Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic copy plot to clipboard #2640

Merged
merged 5 commits into from
Apr 8, 2024
Merged

Basic copy plot to clipboard #2640

merged 5 commits into from
Apr 8, 2024

Conversation

timtmok
Copy link
Contributor

@timtmok timtmok commented Apr 3, 2024

Intent

Address #421

Approach

Adds a Plots pane action button to copy the current plot to the clipboard. This currently writes the same image as shown without any ability to set any options (for dynamic plots).

The clipboard service needed to add support for Electron's clipboard.writeImage API. It can conveniently handle a data URI that we already have from rendering the plot for the pane.

QA Notes

This is basic support for copying to the clipboard for desktop. It's unknown if this works in the web. The web API to access the clipboard does need permission so this may fail until obtaining permission is implemented.

@timtmok timtmok requested review from jmcphers and petetronic April 3, 2024 20:18
// --- Start Positron ---
async writeImage(data: string): Promise<void> {
const blob = new Blob([data], { type: 'image/png' });
navigator.clipboard.write([
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where I think that permission will be required. I haven't been able to test the web version so this will likely fail silently due to permissions.

@@ -4,7 +4,7 @@
*--------------------------------------------------------------------------------------------*/

import { exec } from 'child_process';
import { app, BrowserWindow, clipboard, Display, Menu, MessageBoxOptions, MessageBoxReturnValue, OpenDevToolsOptions, OpenDialogOptions, OpenDialogReturnValue, powerMonitor, SaveDialogOptions, SaveDialogReturnValue, screen, shell } from 'electron';
import { app, BrowserWindow, clipboard, Display, Menu, MessageBoxOptions, MessageBoxReturnValue, nativeImage, OpenDevToolsOptions, OpenDialogOptions, OpenDialogReturnValue, powerMonitor, SaveDialogOptions, SaveDialogReturnValue, screen, shell } from 'electron';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs a Positron code fence. You should add the nativeImage import in a separate import statement to avoid merge conflicts; instructions here: https://connect.posit.it/positron-wiki/overlay-strategy.html#a-note-on-imports

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (plot.lastRender?.uri) {
this._clipboardService.writeImage(plot.lastRender.uri);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should show a little toast notification, or gleam the icon or something, so there's visual feedback that the copy happened.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea. It now returns whether the copy occurred and will toast a notification.

timtmok added 4 commits April 4, 2024 15:53
Implement write image in clipboard service
Only show copy action for supported plots
Add import fence
Remove duplicate copy button
@timtmok timtmok force-pushed the copy-plot-clipboard branch from 221e55f to 1032a79 Compare April 4, 2024 20:20
Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, up to you whether you want to tighten up the error handling before merge

if (copyResult) {
positronPlotsContext.notificationService.info(localize('positronPlotsServiceCopyToClipboard', 'Plot copied to clipboard'));
} else {
positronPlotsContext.notificationService.error(localize('positronPlotsServiceCopyToClipboardError', 'Failed to copy plot to clipboard'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell this can only happen if the plot is unsupported for copying (i.e. a webview plot) so maybe we could make the error message more specific?

Also might be worth adding a try/catch -- if an exception occurs while trying to write the clipboard data, currently nothing will happen (no toast); in this case we should show a toast with the exception text.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm not sure why I did it that way. Definitely fixing this!

@timtmok timtmok merged commit 08917dd into main Apr 8, 2024
1 check passed
@timtmok timtmok deleted the copy-plot-clipboard branch April 8, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants