Add method of "editing" a pinned screenshot #3448
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add a method to allow editing a pinned screenshot by initiating a new screenshot over top of the pinned image, and then closing the old pinned image on success. This adds a new context menu to pinned screenshots. When selected the PinWidget will call the daemon to do the work. Thus this functionality only works when flameshot is running in daemon mode. The context menu options don't show up when not running in daemon mode (I don't know why they don't show up, other ones like the rotate and opacity ones also don't show up). This is not the ideal method of doing this, it doesn't actually edit the screenshot at all. I call it the "bait and switch" method of editing. The only previous attempt that I've soon to do this got caught up in dealing with screens with different DPI (pull/1565). While it would be amazing to be able to limit the whole "grab" region to be less than a screen, or even just one screen, I think this method of editing a pin in the full desktop cature mode is a step forward. It sure is for my workflow anyhow. I'm not familiar with c++ or this codebase so if there looks to be anything weird in this PR it's probably due to ignorance. I'm calling FlameshotDaemon from PinWidget, I'm not sure if I should be doing that or trying to access the Flameshot singleton directly, I just copied off of another tool which called `FlameshotDaemon::copyToClipboard()`. I'm not sure if doing `.geometry() - .layout()->contentsMargins()` is the correct way to get the image geometry, I was guided by what attributes I could see in GammaRay while inspecting a running flameshot. The method of disconnecting from the signals from within the lambdas is from here: https://stackoverflow.com/questions/14828678/disconnecting-lambda-functions-in-qt5 I contemplated calling the method in Flameshot "replacePin" or "captureFromPin" or something more technically accurate. But I figured the signature could stay a bit optimistic and if people would like the behaviour to be firmed up to match the goal in the future that could be done. Eg add functionality to the CaptureWidget to make it so you can't modify the capture region, change the grab region to be less than the whole desktop etc. Known issues: * you can move/resize the selection region, eg not just edit a pin but create a completely different one of a different region of the screen Relates to: flameshot-org#954
toofar
changed the title
This is a bit of defensive coding that shouldn't normally be needed. I was playing around with making the gui capture window support covering only specific regions and managed to get the capture widget on a different screen to the pin. Then closed the pin before finishing the new capture. Then the existing.close() segfaulted because the reference to existing was invalid. It would have been easier to just do something like `existing.isValid()` or `existing != nullptr` but apparently you can't check if a reference is valid? Seems weird, but hence the extra signal juggling to deal with this case. The new bit is mostly closePinConn and destroyedConn (which just cleans up the former). And I moved the "disconnect everything" to a separate variable to be able to re-use it.
|
I added another commit just to handle an unusual case of the pin widget being closed while the "edit pin" capture was still open. It shouldn't happen normally, but you know how users are. I ran into it while hacking away at making it so the capture window only covers one screen while editing a pin, but that can wait until people have had time to mull over this one. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a method to allow editing a pinned screenshot by initiating a new screenshot over top of the pinned image, and then closing the old pinned image on success.
Apologies for not proposing this method on the linked issue first, I wanted to have a poke at the code today and didn't expect to get this far.
This adds a new context menu to pinned screenshots. When selected the PinWidget will call the daemon to do the work. Thus this functionality only works when flameshot is running in daemon mode. The context menu options don't show up when not running in daemon mode anyway (I don't know why they don't show up, other ones like the rotate and opacity ones also don't show up).
Here is a professional video recording off how it works:
editing_pins-2023-12-27_18.50.05.mp4
This doesn't actually edit the screenshot at all, I call it the "bait and switch" method of editing. The only previous attempt that I've seen to make pins editable got caught up in dealing with screens with different DPI (#1565). While it would be amazing to be able to limit the whole "grab" region to be less than a screen, or even just one screen, I think this method of editing a pin in the full desktop capture mode is a step forward. It sure is for my workflow anyhow.
I'm not familiar with c++ or this codebase so if there looks to be anything weird in this PR it's probably due to ignorance.
I'm calling FlameshotDaemon from PinWidget, I'm not sure if I should be doing that or trying to access the Flameshot singleton directly, I just copied off of another tool which called
FlameshotDaemon::copyToClipboard().I'm not sure if doing
.geometry() - .layout()->contentsMargins()is the correct way to get the image geometry, I was guided by what attributes I could see in GammaRay while inspecting a running flameshot.The method of disconnecting from the signals from within the lambdas is from here: https://stackoverflow.com/questions/14828678/disconnecting-lambda-functions-in-qt5
I contemplated calling the method in Flameshot "replacePin" or "captureFromPin" or something more technically accurate. But I figured the signature could stay a bit optimistic and if people would like the behaviour to be firmed up to match the goal in the future that could be done. Eg add functionality to the CaptureWidget to make it so you can't modify the selection region, change the grab region to be less than the whole desktop etc.
Known issues:
Relates to: #954