Auto-copy shared image link to clipboard
ClosedPublic

Authored by ngraham on Jun 12 2018, 1:58 PM.

Details

Summary

Whenever an image is shared and a link is provided, the link is automatically copied to the clipboard.

We also rename the Copy save location checkbox to prevent user confusion with this new feature (and also to better reflect what the Copy save location feature actually does).

FEATURE: 392722
FIXED-IN 18.08.0

Test Plan

Shared an image via Imgur. The link immediately appeared at the top of Klipper's history.

Diff Detail

Repository
R166 Spectacle
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham requested review of this revision.Jun 12 2018, 1:58 PM
ngraham created this revision.
ngraham edited the test plan for this revision. (Show Details)Jun 12 2018, 1:59 PM
ngraham edited the summary of this revision. (Show Details)

Possible avenues for improvement:

  • The text is now very long; perhaps we should come up with a more concise way of expressing this? Or make it a multi-line string?
  • Is it still necessary for the notification to be persistent when the link is copied automatically?
rkflx added a subscriber: rkflx.Jun 17 2018, 11:55 PM

Thanks, preferable over D13221 indeed.

Works great, the only problem I can see is that we have a Copy save location to the clipboard checkbox in the config dialog which is unchecked by default, yet your patch does what it says regardless (ignoring for a moment that "save location" and "link" are technically different things). I think this should respect the setting, as in the other case you can still manually copy the link. I'd be in favour of checking the option by default in another patch, though.

However, see also https://bugs.kde.org/show_bug.cgi?id=390415#c5 and what we recently learned in Gwenview in relation to mimetypes (i.e. GIMP wants a pixmap, copying a local URL is not enough). This gives us three cases which can happen at different points in time:

  1. Take screenshot, trigger Copy to Clipboard. Currently no way to automate this (and even then we'd have the Klipper issue).
  2. Save screenshot, automatically copy URL of file to clipboard if option checked. Would also need to copy pixmap for GIMP to work (but maybe Ctrl+C is used rather than Ctrl+S for this anyway?).
  3. Export to Imgur, automatically copy remote URL. Currently ignores checkbox.

This is all a bit confusing. Can you see a good way to solve this nicely? If not, a workaround for now would be to simply rename the checkbox to emphasize it is strictly about saving an image, e.g. what about Copy file location to clipboard after saving? (← Actually, I'd prefer this over your patch respecting the checkbox and fiddling with pixmaps for GIMP.)


  • The text is now very long; perhaps we should come up with a more concise way of expressing this? Or make it a multi-line string?

Yeah, it's a bit long, in particular and narrow screenshots, where even in English the window gets wider to accomodate the long string. I tried with a linebreak, but that looks odd. Ideas for rewording:

Link to shared image copied to clipboard: https://imgur.com/…

Find the shared image at https://imgur.com/… (link copied to clipboard).

(Or something similar?)

  • Is it still necessary for the notification to be persistent when the link is copied automatically?

Hm, the original motivation to make it persistent does still apply mostly, doesn't it? The message might vanish just when you try to read it. Also, in case a link or button for the "delete" URL is added, we'd still only have a single URL in the clipboard. Also, not everybody is familiar with the concept of a clipboard or how to access it by pasting into a web browser (they only know how to click on links).

I guess the root problem is that the message should not stay around forever, in particular when a new screenshot has been taken and the URL does not match with the preview anymore. Thoughts?

ngraham planned changes to this revision.Jun 18 2018, 2:16 PM

Copy file location to clipboard after saving seems good to me. I realize I'm not actually sure what this feature is even useful for anyway. Wouldn't you almost always want to copy the pixmap instead of the local path to it? Do you know?

I'll think about some more alternative wordings for the text.

On the subject of the shared image notification, perhaps I could make it disappear if and when a new screenshot is taken.

Yeah, the right thing to do in the long term would be to step back and rethink how copying URLs or pixmaps should work from a user/workflow perspective. I tried to start with some peculiarities in my comment above, but obviously this needs more thought. It should be simple, not a myriad of buttons and options, yet support all existing workflows.

As for copying the local path instead of (or in addition to?) the pixmap, I imagine it could be useful:

  • …when trying to paste a reference/link to the file into a notes app or text file
  • …or simply for opening/editing the file in another app afterwards (instead of copying the pixmap and having to save from the app later on).
  • Also, a local path will persist until the file is moved or deleted, while a pixmap is gone as soon as Spectacle is closed.
  • Last but certainly not least, the feature also works for the global shortcuts.
  • BTW, git blame will lead you to https://bugs.kde.org/show_bug.cgi?id=357423.

For now, let's just keep the option as-is, and reword it so it does not conflict too much with your patch.

ngraham updated this revision to Diff 36291.Jun 18 2018, 10:40 PM
ngraham edited the summary of this revision. (Show Details)

Rename "copy save location" checkbox label to reduce user confusion, and also reflect what it actually does

Restricted Application added a project: Documentation. · View Herald TranscriptJun 18 2018, 10:40 PM
Restricted Application added a subscriber: kde-doc-english. · View Herald Transcript
ngraham updated this revision to Diff 36292.Jun 18 2018, 10:46 PM

Also hide the screenshot-specific KMessageWidget when taking a new screenshot

ngraham updated this revision to Diff 36293.Jun 18 2018, 10:54 PM

Shorten and simplify the "image shared; here's the link" text

ngraham edited the summary of this revision. (Show Details)Jun 18 2018, 10:55 PM
ngraham edited the test plan for this revision. (Show Details)
ngraham updated this revision to Diff 36294.Jun 18 2018, 10:58 PM

Respect existing casing style for the changed comment

ngraham edited the summary of this revision. (Show Details)Jun 18 2018, 11:28 PM
ngraham edited the summary of this revision. (Show Details)Jun 19 2018, 3:58 AM
rkflx accepted this revision.Jun 19 2018, 10:21 PM

LGTM, just a comment to keep the commit history spotless ;)

src/Gui/KSMainWindow.cpp
268

That's a nice and simple solution!

Would it make sense to split that out in a separate commit (no need for a Diff), and reference e9218fddfabd in the commit message? That's where MessageDuration::Persistent was introduced, which is not really specific to shared image links.

This revision is now accepted and ready to land.Jun 19 2018, 10:21 PM
ngraham updated this revision to Diff 36363.Jun 19 2018, 10:40 PM
ngraham marked an inline comment as done.

Move the new mMessageWidget->hide(); call to a separate commit

This revision was automatically updated to reflect the committed changes.