Add magnifier for rectangle mode, it can show 5x of the mouse pointer
Depends on D11598
FEATURE: 392482
rkflx | |
ngraham | |
alexeymin |
Spectacle |
Add magnifier for rectangle mode, it can show 5x of the mouse pointer
Depends on D11598
FEATURE: 392482
In rectangle mode:
No Linters Available |
No Unit Test Coverage |
Thanks for the video. Horizontal and vertical offset don't seem to be equal there, not sure I like that. Also, you don't show what happens when moving the cursor near a border of the screen ;) Anyway, I see where you are coming from, even if it does not fit my taste.
Nevertheless, now the magnifier obscures the view of the to-be-screenshotted area again when grabbing the top left corner. We are kinda back were we began, but of all versions we iterated through, this one is probably the best yet.
While I think a static magnifier or my mockup might be preferable, I'd be also fine with another mitigation: magOffset: 32 would still be near the cursor, but leave a gap large enough so you could still get an unmagnified view of the area around the cursor. Could you agree to that?
Okay, cool. Just like you implemented it.
Just checked with Qt 5.6.2, it looks exactly like in the screenshot above.
Could you share a screenshot (i.e. no photo) of how it looks for you, ideally on a white background? You can use spectacle -b -d 4000 to start a second instance capturing how you hold the mouse button for the magnifier (or just use a VM).
I currently lack the time to debug this for you in-depth, sorry for that. All I could see so far was that removing the crossBackground element made the black lines go away completely. Could you simply try to add a border by yourself? Otherwise when showing e.g. a white magnifier background on a white screen background you won't know where the magnifier actually is…
src/QuickEditor/EditorRoot.qml | ||
---|---|---|
286 | @ngraham When changing to Item, for me the background of the magnifier got transparent instead of white when operating near the edges of the screen. Is there a better way you have in mind? @guotao I guess you should add a comment to that effect to the code, so future readers will also understand why you chose a Rectangle. |
FWIW, this fixes the black border issue for me:
diff --git a/src/QuickEditor/EditorRoot.qml b/src/QuickEditor/EditorRoot.qml index e267592..11d1b7b 100644 --- a/src/QuickEditor/EditorRoot.qml +++ b/src/QuickEditor/EditorRoot.qml @@ -282,7 +282,8 @@ Item { height: ( magPixels * 2 + 1) * magZoom; width: ( magPixels * 2 + 1) * magZoom; - border.width: 0; + border.width: 1; + border.color: black visible: false; clip: true
With this change, it would actually make sense to be using a Rectangle too.
Like @rkflx, I would prefer for the magnifier to not always be down-and-to-the-right, but I think I could live with the current design.
This feature does need a way to turn it on and off though. Let's put that in the Configure window for now, pending an overhaul of the rectangle-mode-specific settings UI.
Are you sure you pasted the correct diff? Yours has an Item in it, and I don't think you can just use black, because I get ReferenceError: black is not defined. Besides that, it's not working for me, because this border is underneath the image, and even then the border is not symmetrical, because it is in addition to the already existing black lines. I think there might be something else going on.
You're right, that was the wrong diff. Try changing crossMagnifier's height and width to the following:
height: ((magPixels * 2 + 1) * magZoom) -1; width: ((magPixels * 2 + 1) * magZoom) -1;
Cool, that works, and even with HiDPI. I kinda tried the same before, just forgot the brackets…
different result on my side:
archlinux with qt5 5.10.1
regarding the magnifier behaviour, I think we can improve it in later commits.
There are only three users currently, maybe we need more user suggestions after release.
Thanks, I think this solved the mystery! I inspected your screenshot with KMag, and noticed that the lines in the magnifier where transparent, while for me they are not. Also, your blue handles have some antialiasing, while for me they are very blocky. This all means that there are some rendering issues with Qt in relation to QML/OpenGL/graphics driver, which unfortunately are still common compared to a traditional QWidgets based interface (besides the horrible performance upon resizing the selection…).
If I change renderTarget to Image, I get the same rendering like you are showing in the screenshot, which confirms my theory. I suspect Nate experienced the same problem, because he probably just used a VM to test like I did here. For now, don't worry about it, no need for the -1. I'll still have to investigate some more, and then I'll either send a separate patch to change renderTarget, or we'll have to live with sub-par rendering for some graphics configurations until the underlying stack has been improved.
regarding the magnifier behaviour, I think we can improve it in later commits.
There are only three users currently, maybe we need more user suggestions after release.
Yup, we should definitely get more feedback later on. Won't happen automatically though, because not that many people are running Spectacle from Git master. OTOH this means that we need to provide an option to turn the feature off for any release we ship to users.
One more thing I noticed: The magnifier now hides the size label in some cases, perhaps the size label should be moved elsewhere. (Probably best to work on this in a follow-up patch, to keep the Diff understandable…)
I can confirm that when running Spectacle from git master with this patch on bare metal (not in a VM), the issue goes away. Could not reproduce the issue with any rendering backend chosen in the compositor KCM.
So that's nice. @taoguo, can you add a setting to turn this feature on and off in Spectacle's Configure window? I feel like we're getting pretty close to the finish line here!
Thanks for adding the toggle! But the main window isn't an appropriate place for the new Show Magnifier checkbox, because it only affects one capture mode and isn't the kind of thing that users are likely to toggle on and off for different screenshots. Also, five checkboxes is getting towards the high end of what's acceptable to put on the main window.
This setting should be in the Config window on the General page, inside the "Rectangular Region" box, which is currently where all the other options affecting Rectangular Region live.
I tested this, and it looks cool and works fine!
src/QuickEditor/EditorRoot.qml | ||
---|---|---|
36 | can these magZoom, magPixels, magOffset have type int instead of var? Just asking, why var? | |
59 | This function setShowMagnifier() is probably not needed at all. You can set context property directly from C++, without invokeMethod, see below | |
src/QuickEditor/QuickEditor.cpp | ||
122 ↗ | (On Diff #32248) | It is considered bad practice to directly manupulate QML from C++. Good practice: instead of connecting to signals from QML to C++, just expose slots from C++ objects into QML. From QML, call those slots to notify about events. Do not set properties manually, just expose them from C++ objects as Q_PROPERTY so QML code can use them directly. Global application settings should be exposed into QML with context properties (d->mQuickView->rootContext()->setContextProperty()). In this case you could directly modify showMagnifier property, instead of invoking a method by QMetaObject::invokeMethod(), if you already have root item: rootItem->setProperty("showMagnifier", true); I know, this how it was done in existing code already, but do not write new code like this example in Spectacle. It is bad. Perhaps, the whole QuickEditor thing in Specatcle (and its corresponding QML) should be rewritten, but this is unrelated to this diff... |
src/QuickEditor/EditorRoot.qml | ||
---|---|---|
202 | Javascript only has var type for variables. Not a property declaration here ( |
src/QuickEditor/EditorRoot.qml | ||
---|---|---|
202 | @alexeymin, so it's doesn't need change here, right? | |
src/QuickEditor/SelectionRectangle.qml | ||
31 | In HiDPI mode, It's not int |
@guotao Thanks for the updates! The config settings are working great now, just some final code polishing to be done.
test magnifier on multi-screen
Found a small issue: When both screens have a different height, the magnifier sometimes does not "flip up", but gets obscured.
Would be great if you could tackle this afterwards… ;)
src/QuickEditor/EditorRoot.qml | ||
---|---|---|
265 | Maybe there's a reason you have to add this, but currently I don't see it. Could you explain? | |
284 | Why not use height here? | |
293 | Are you sure false is the right choice here? The docs say "at the expense of small 'ui element' images", but I don't see any of those. IIUC this is about caching the image on the GPU, which to me sounds sensible in this case… | |
297 | Is this needed? | |
src/QuickEditor/SelectionRectangle.qml | ||
30–31 | I don't really understand why those properties are called zx and zy. Could you come up with a more descriptive name? If not, at least add a comment here describing their purpose. | |
31 | Yup, see https://doc.qt.io/qt-5/qml-real.html for what is allowed. Using double seems to work for me in HiDPI mode (as opposed to int). |
Update diff
src/QuickEditor/EditorRoot.qml | ||
---|---|---|
265 | I change this because the help text show and hide very quickly. | |
284 | sorry, what you mean? | |
293 | I just follow the exist code. | |
297 | It seems my debug code but not removed. |
Great idea to add this to Spectacle too, and works nicely!
I had to read the source code to know where to look for it. What about something like this (inspired by Gwenview):
src/Gui/SettingsDialog/GeneralOptionsPage.cpp | ||
---|---|---|
63 ↗ | (On Diff #32360) | I'd use ⇧ in the text here (even though it already works with every modifier…) , as it is more commonly connected with modifying state (e.g. "Preserve aspect ratio"): Hold the Shift key to temporarily toggle the magnifier when adjusting the selection rectangle. (Also reworded a bit, to make it more clear that the magnifier only appears once you press down a mouse button.) |
src/QuickEditor/EditorRoot.qml | ||
76 | For me this triggers for every key on the keyboard. Could you make it so that only ⇧+Alt+Ctrl affect the magnifier? | |
201–202 | You could be more DRY here: var magWidth = crossMagnifier.width; var magHeight = crossMagnifier.height; | |
265 | Ah, I see, makes sense. I was looking for an explanation connected to the magnifier, but apparently there is none to find ;) Could you split out this change into a separate Diff, then? (Could even go to the Applications/18.04 branch, so users will get the fix earlier than the magnifier, which would have to wait until 18.08…) | |
284 | You are duplicating the calculation for height also for width. As you simply want both to be the same, you could write: width: height; | |
293 | Okay. But if it works well for you on real hardware, I'd tend to go with enabling the cache. Let me investigate this more and possibly change it in the other place too. For crossBackground, you can remove the line for now, because the default value is already true. | |
src/QuickEditor/SelectionRectangle.qml | ||
30–31 | Thank you, much better ;) |
src/QuickEditor/EditorRoot.qml | ||
---|---|---|
76 | my bad, wrong logic |
Another idea would be to add this to the on-screen help text of the rectangle selection (but it is already quite crowded…).
src/QuickEditor/EditorRoot.qml | ||
---|---|---|
293 | Why is this marked as "Done" when the line is still there? |
This is looking great! +1 for adding a Gwenview-style inline message about being able to toggle it with shift.
Sure, do it in a separate Diff, we'll wait with submitting this one until it's done. Let us know if you need any help or have specific questions.
As Henrik said...
While you have the mag2 branch checked out, you can use arc feature to base the next patch on this one and automatically have it marked as a dependency. See more at https://community.kde.org/Infrastructure/Phabricator#If_the_patches_are_all_for_the_same_project