-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add method of "editing" a pinned screenshot #3448
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
base: master
Are you sure you want to change the base?
Conversation
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
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. |
|
Thanks for the contribution. I took some time to test this today and the behavior on KDE wayland does not match the video. I also think that this solution is not really the path we should go down for the production implementation. I have always envisioned editing a pinned image as re-opening the pin full screen as if it had not been pinned in the first place. Are you still interested in working on the feature? |
I think so, as long as the direction it's headed down is something that I would still use.
Could you explain that more please? I'm not quite sure I can fully see what that should look like. Once someone has chosen to edit a pin what should happen? When you say full screen, do you mean centering the pinned image on the screen or something? Because the flameshot capture window is already full screen, right? |
|
When an image is pinned and the user hits edit the application goes back to full screen edit mode as if the image was in the original full screen capture mode. This will be possible if the pinned widget saves the entire screen capture. When the user hits Esc or the X button while editing a pinned image, the image is pinned again rather than being discarded. I do not think the position will be restored, because its possible that when the user re-enters edit mode, they adjust the size of the pin. |
|
I see, that's a clear description, thank you! I gotta admit, my initial reaction is that I prefer the current implementation I have where editing the pin only edits the pin, especially how I have it with the flameshot window only on one monitor. It seems to me there are (at least) two motivations for this feature:
For me, I only really care about (2). I just want to draw on the pin. Having a full desktop capture window is actually a negative in this setup, which is why I have a branch to make the capture window only span one screen when editing a pin, I was hoping to raise that PR after this one was merged. For example, in my dual monitor setup I can be editing the pin on one monitor while the other montor is updating live, or I'm scrolling through a page or whatever. Which opens up use cases like having a screen shot as a kind of todo list where you cross off items when done. Having a feature that surfaces a full desktop capture when editing a pin seems like it would prevent going down the path I was hoping for. For me, if I want to re-do a pin I'll just close it and start the capture process over. I can understand that some people may want to preserve the state of the desktop from when they started the pin. I wonder which use case is the most desired though? Looking at #954 to see where people have requested this feature, this comment describes:
Which I think is the way I use it, eg I only want to edit the pinned image, I don't want to "re-do" the capture. I think this issue also describes the same use case, just annotating pins and not needed a whole (old) desktop capture. This comment requests the feature but without a good description. This one, uh, actually seems to want to recover pins closed by mistake, which seems like a different issue (although one I face as well, I have to move my mouse to another screen to remove keyboard focus from new pins). I couldn't find any other comments about this feature. I had a look around and it looks like I could also reproduce my workflow by:
Altogether a lot less convenient than my current setup or right click -> edit pin! So after all that I have a couple of questions:
|
|
@lupoDharkael @borgmanJeremy Any update about this pr. I really need to re edit the pin picture. I sincerely hope that flameshot is not just a screenshot tool, but also a medium for me to re extract and recombine fragmented information from different interfaces. So if we can add grouping and re editing functions to the pin picture, flamespot will have the ability to organize and display information. |
|
@exploman this PR is not representative of how I would like re edit to work. The feature will eventually be added. |
|
@borgmanJeremy Considering that the pinned image is logically smaller than or equal to the original image size we got from the compositor, wouldn't it be easier to have a window for edit mode? Something roughly similar to Ksnip or Spectacle. Basically a normal window with a visible sidebar that has the same tools as This has three main advantages imho:
If I haven't managed to convey what I mean my a "window for editing" through this very rough explanation, I can draw a sketch to illustrate it better 😅 |
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