Added CopyToClipboard parameter to dbus RectangularRegion and set up shortcut keys
Needs ReviewPublic

Authored by jatinsharma on Mar 29 2020, 3:01 AM.

Details

Reviewers
None
Group Reviewers
Spectacle
Summary

Added a copyToClipboard boolean parameter to the dbus function RectanguhlarRegion
Also set up an extra shortcut key (adding Ctrl to the existing one) to copy the rectangular screenshot to clipboard rather than save to file.

Diff Detail

Repository
R166 Spectacle
Branch
feature/copyToClipDBus
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25632
Build 25650: arc lint + arc unit
jatinsharma created this revision.Mar 29 2020, 3:01 AM
Restricted Application added a project: Spectacle. · View Herald TranscriptMar 29 2020, 3:01 AM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
jatinsharma requested review of this revision.Mar 29 2020, 3:01 AM

Thanks for your patch.
However, I think you shouldn't change the existing method signature.

I don't think this is the right approach. You are just changing it for one method. Maybe we could make it configurable in settings,
Currently we have

After taking a screenshot: - Save
                                                  - Copy

That only applies when the gui is running Maybe we could find a solution to have it also apply when the ui is not up? Something like the above and a setting what should happen if you press shortcuts? What do you think @ngraham?

jatinsharma added a comment.EditedMar 30 2020, 12:04 PM

I did this only for a single function to get feedback on whether this approach was acceptable.

I had multiple ways in mind and this was the least nice but fastest to do, so I went ahead with it to get the discussion rolling.

Regarding the method signature, the methods already take in one or two parameters which are properties (e.g. includeWindowDecorations, bool includeMousePointer), or settings the screenshot should have.
The "should it be copied to the clipboard" seemed like a property similar to these so I thought it made sense to add another parameter. This was also what was suggested to me by 'PointiestStick' (reddit) aka @ngraham.

Also, these 2 to 3 boolean values/properties are passed around throughout the codebase. Personally I would prefer that these were packed in a struct which is then passed around.
This would make it more flexible making adding any future such 'properties' easier and would not affect the method signature (in the future).

That only applies when the gui is running Maybe we could find a solution to have it also apply when the ui is not up? Something like the above and a setting what should happen if you press shortcuts?

I think the discussion on T11418 is quite relevant to this.

Any updates on this?

Sorry, I'll try to get to this soon.

I don't think this is the right approach. You are just changing it for one method. Maybe we could make it configurable in settings,
Currently we have

After taking a screenshot: - Save
                                                  - Copy

That only applies when the gui is running Maybe we could find a solution to have it also apply when the ui is not up? Something like the above and a setting what should happen if you press shortcuts? What do you think @ngraham?

I think we need new DBus activation modes that basically do the same as:

  • spectacle -bcf (full screen)
  • spectacle -bcr (rectangular region)
  • ...and so on.

I agree that there's no need to change the existing method, like Kai and David said. If I nidicated something different before, then oops.

Does that make sense?

We have five modes for taking the screenshot.

  1. Fullscreen
  2. CurrentScreen
  3. ActiveWindow
  4. WindowUnderCursor
  5. RectangularRegion

Shouldn't each of these have an option for the screenshot to be copied to the clipboard? Why should only two of these get the feature?
If we are not to change function signatures that would mean 5 extra functions.

Also, I just wanted to know, is the rationale behind letting the signatures remain intact is to not disturb places where these are already being used?

Right, so all five would need new functions. And yes, the rationale is to avoid changing existing function signatures which could break people and software already using them.

Okay, Should it also include the functionality to be able to save to file AND copy to clipboard for the same screenshot?

Nah, I think for these new ones, just putting it on the clipboard is fine. The point after all is just to quickly paste an image into like a chat window or something.

Hello, Sorry about the mess. I have accidentally created a new revision, but I wanted to update this. I can't seem to figure out how to revert this.

Added 5 dbus method corresponding to the five modes of taking a screenshot.
They copy the screenshot to clipboard without saving them to a file.

Great work! Do you think you could also add new entries to desktop/org.kde.spectacle.desktop.cmake so that these are actually exposed in the UI and usable? That would be fantastic, and sufficient to fix https://bugs.kde.org/show_bug.cgi?id=416106.

.gitignore
23

Unrelated. Please remove this from your patch, and I'll make this change myself.

Thank you! I didn't add the shortcuts in this commit. I just wanted to confirm/ask some things before implementing them.

So we'll be adding three new keyboard shortcuts, for FullScreen, ActiveWindow and RectangularRegion.

  1. What should be the new shortcut keys? Like D9117 I was thinking of
a. Ctrl+Shift+Print - FullScreen Clipboard  
b. Ctrl+Meta+Print  - ActiveWindow Clipboard  
c. Ctrl+Print       - RectangularRegion Clipboard
  1. What all changes do I need to make? The ones I see are,
    1. desktop/org.kde.spectacle.desktop.cmake
    2. src/SpectacleCore.cpp:141
    3. src/ShortcutActions.cpp (There seems to be a lot of duplication between this and desktop/MigrateShortcuts.cpp)
    4. desktop/MigrateShortcuts.cpp (I'm not sure what all exactly to add here.)

We seem to be writing the same shortcut key combinations in at least three places in A, C and D above.

  1. In desktop/org.kde.spectacle.desktop.cmake There are a lot of languages which require a Name[lang]. Do I just leave them for now?
aleasto added a subscriber: aleasto.May 3 2020, 2:38 PM

Nah, I think for these new ones, just putting it on the clipboard is fine. The point after all is just to quickly paste an image into like a chat window or something.

I would definitely have preferred if it could both save to file and copy to clipboard. Especially since the Copy button on the notification of file saved copies the file path rather than the image itself.