Added magnifier for more precise selections

[SilasDo]

related to #328 and #390

I was missing a magnifier so I decided to add this feature.
not really happy about the way I integrated the magnifierwidget into the capture process but its good enough for my use case.
(for example: right now it does not show the magnifier when using the move tool)

I'm open to feedback.
If this is something you wold like to see in the Flameshot I'm willing to improve the implementation.

demo:

mag.mp4

btw not sure if you already know this but the selection is bugged in v11 beta, for example the selection is no longer constrained to the screen (you can simply drag the selection offscreeen).

selection_offscreen.mp4

another bug i encountered in v11 is that the first pixel you selected does not end up in the selection, an easy way to test this is to select your entire screen top-left to bottom-right (you would expect the selection to equal your screen resolution but it does not)

I compiled the v10.2 version and had neither of those issues.

[veracioux]

@SilasDo I appreciate you taking the time to implement this. But you should have first discussed this via issues (the devs only agreed that this feature is useful but not how it should be executed). Right off the bat I'd like to say that you should take what I say with a grain of salt because @borgmanJeremy and @mmahmoudian might disagree with me.

We were looking at a solution that reuses the color grab zoom widget I implemented in #1869, in order to prevent unnecessary code duplication.
Would you be willing to combine your effort with my implementation so that we use the same code for both functionalities? You could adapt my code to reuse yours, or you can adapt your code to reuse mine. I personally prefer the second option, obviously because I am biased (that's why @borgmanJeremy should give his opinion), but I can also list the following reasons:

• It already basically implements the magnifier widget, it just needs a crosshair and a round variant
• It implements the color grabber, so it doesn't require any adaptation on this front
• It is decoupled from CaptureWidget. Basically the recent refactoring trend is to move functionality outside of CaptureWidget::mouseMoveEvent and elsewhere

Some criticism

• May be personal preference, but I don't see a need to include both a circular and a square magnifier. I really don't like including the "Square shaped magnifier" config option. It strikes me as arbitrary, inelegant, and bloats the config and corresponding GUI (@borgmanJeremy might disagree with me on this)
• The magnifier should IMO be toggleable using the keyboard and/or mouse
• See the following recording, which happens when the FLAMESHOT_DEBUG_CAPTURE option is ON (this is what I always use when debugging flameshot, so I'd like it to work properly):
  

If we collectively decide that you should merge this with my implementation, then please hold off on working on it until I fix a bug on MacOS regarding the ColorGrabWidget (I'll inform you when it's done). But in any case, this feature should not be included in the v11 release.

P.S. I was able to reproduce the existing bugs you mentioned, and I've started working on them. Thanks for testing out the beta!

[mmahmoudian]

@SilasDo thank you for taking the time to implement such feature. It's good to know there are users who can contribute to the codebase.

My suggestion is to have them both using the same code as it would increase the maintainability.

Regarding the shape, I myself love to see the crosshairs to show the center pixel, plus I feel like the circle shape is more smoother to the eye and intuitive as IRL magnifiers are typically circular.

Also regarding in which version to release this in, I would also go with the idea that let's not delay the v11 but add this to the next minor/major version.

So to sum up, I completely agree with the points @veracioux made.

[borgmanJeremy]

@SilasDo now that v11 is calming down we have bandwidth to help get this integrated into v11. I think @veracioux has good points on merging the implementation with the existing magnifier feature. Are you willing to take on that work?

If not we can take your code and port it over. Overall I really like the demo.

[SilasDo]

I'm currently quite busy with another project, i could probably find some time in about a week.

i took a quick look at the colorgrabwidget (really like the hold left click for precise selection feature)

but I'm not really sure how to best merge both implementations since they are currently not very similar.
e.g: magnifier on top of cursor vs at an offset

things like that should be agreed upon in order for me to merge features.

that being said If someone else has time to implement this, that would be totally great as well!

[mmahmoudian]

but I'm not really sure how to best merge both implementations since they are currently not very similar.
e.g: magnifier on top of cursor vs at an offset

I think the one with offset is good for when user wants to find the region of interest, and then the rightist can freeze the zoom window do the precise pixel can be selected. For me the hardest part is to find the region, for instance in the current implementation if I want to get the color of a green tongue in 🤑, I should move the mouse quite a bit to find the region of the tongue, so the offset can help because it doesn't cover my field of view.

[veracioux]

@SilasDo Then I'm going to merge the implementations.
@mmahmoudian Point taken.

[borgmanJeremy]

@veracioux when you say "merge the implementations" do you mean you want this PR merged as is?

Or are you going to merge this PR to another branch, combine the zoom classes, and then submit a new PR?

[veracioux]

@borgmanJeremy I meant the latter. But you could also merge this PR into master if you think it is mature enough, so flameshot-git users don't miss out on it. And then I can make my changes in another branch.

[borgmanJeremy]

@veracioux okay I think the best way to approach this will be to merge as is and iteratively improve it.