Add magnifier for rectangle mode
ClosedPublic

Authored by guotao on Mar 23 2018, 8:41 AM.

Details

Summary

Add magnifier for rectangle mode, it can show 5x of the mouse pointer

Depends on D11598

FEATURE: 392482

Test Plan

In rectangle mode:

  1. select rectangle, check magnifier
  2. adjust selection, check magnifier
  3. change the selection to any size and check magnifier
  4. test magnifier on multi-screen
  5. test magnifier in HiDPI mode

Diff Detail

Repository
R166 Spectacle
Branch
mag2 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes

Update diff, restore the magnifier behavior so that it's same as the video

attach video

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?

append video for right bottom case

guotao updated this revision to Diff 31964.Apr 12 2018, 11:28 AM

update diff

@rkflx how about the border issue on your side? do you have any idea?

rkflx added a comment.Apr 12 2018, 1:16 PM

append video for right bottom case

Okay, cool. Just like you implemented it.

  • The border of the magnifier still looks a bit odd, because only on the top and on the left there is a black line, but this line is missing on the bottom and on the right.

It's strange, because on my side, it's ok

Strange indeed, for me it looks like that with Qt 5.10.0:

Just checked with Qt 5.6.2, it looks exactly like in the screenshot above.

@rkflx how about the border issue on your side? do you have any idea?

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…

rkflx edited the summary of this revision. (Show Details)Apr 12 2018, 1:18 PM
rkflx added inline comments.Apr 12 2018, 1:27 PM
src/QuickEditor/EditorRoot.qml
287

@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.

rkflx added a comment.Apr 12 2018, 6:44 PM

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

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.

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

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;
rkflx added a comment.Apr 12 2018, 7:04 PM

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.

rkflx added a comment.EditedApr 13 2018, 11:18 AM

different result on my side:

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!

guotao updated this revision to Diff 32244.Apr 16 2018, 4:13 AM

Add option to enable/disable

guotao updated this revision to Diff 32245.Apr 16 2018, 4:16 AM

Update diff

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.

guotao updated this revision to Diff 32248.Apr 16 2018, 5:20 AM

I see, update diff

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?

60

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

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...

guotao updated this revision to Diff 32271.Apr 16 2018, 11:39 AM
guotao marked an inline comment as done.

Update diff

guotao marked 4 inline comments as done.Apr 16 2018, 11:41 AM
guotao edited the test plan for this revision. (Show Details)
ngraham added inline comments.Apr 16 2018, 1:18 PM
src/QuickEditor/EditorRoot.qml
203

Make all of these ints, please. vars are much slower.

src/QuickEditor/SelectionRectangle.qml
31

ints please.

alexeymin added inline comments.Apr 16 2018, 1:31 PM
src/QuickEditor/EditorRoot.qml
203

Javascript only has var type for variables. Not a property declaration here (

guotao added inline comments.Apr 16 2018, 1:52 PM
src/QuickEditor/EditorRoot.qml
203

@alexeymin, so it's doesn't need change here, right?

src/QuickEditor/SelectionRectangle.qml
31

In HiDPI mode, It's not int

alexeymin added inline comments.Apr 16 2018, 2:24 PM
src/QuickEditor/EditorRoot.qml
203

Yes, in property declarations you can say property int x, property double y, but variable declarations inside JS code can only use var or let.

double offsetX

will produce unexpected token errors here.

src/QuickEditor/SelectionRectangle.qml
31

maybe double then?

alexeymin accepted this revision as: alexeymin.Apr 16 2018, 6:20 PM
ngraham accepted this revision as: ngraham.Apr 16 2018, 6:40 PM

@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.


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…)

Would be great if you could tackle this afterwards… ;)

src/QuickEditor/EditorRoot.qml
266

Maybe there's a reason you have to add this, but currently I don't see it. Could you explain?

285

Why not use height here?

294

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…

298

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).

guotao updated this revision to Diff 32354.Apr 17 2018, 2:43 AM

Update diff

src/QuickEditor/EditorRoot.qml
266

I change this because the help text show and hide very quickly.
I think the default value should be false.

285

sorry, what you mean?

294

I just follow the exist code.
After set to true, it seem works well.

298

It seems my debug code but not removed.
after remove, works well.

guotao updated this revision to Diff 32355.Apr 17 2018, 2:47 AM
guotao marked 8 inline comments as done.

Update diff

guotao marked 5 inline comments as done.Apr 17 2018, 2:48 AM
guotao updated this revision to Diff 32357.Apr 17 2018, 2:51 AM

Update diff

guotao updated this revision to Diff 32359.Apr 17 2018, 4:00 AM

@rkflx I add maigc key, so that we can toggle magnifier on/off use alt key

guotao updated this revision to Diff 32360.Apr 17 2018, 4:40 AM

Please also review my tool tip.

rkflx added a comment.Apr 17 2018, 8:19 AM

@rkflx I add maigc key, so that we can toggle magnifier on/off use alt key

Great idea to add this to Spectacle too, and works nicely!

Please also review my tool tip.

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

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
77

For me this triggers for every key on the keyboard. Could you make it so that only +Alt+Ctrl affect the magnifier?

202–203

You could be more DRY here:

var magWidth = crossMagnifier.width;
var magHeight = crossMagnifier.height;
266

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…)

285

You are duplicating the calculation for height also for width. As you simply want both to be the same, you could write:

width: height;
294

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 ;)

guotao updated this revision to Diff 32386.Apr 17 2018, 1:09 PM
guotao marked 11 inline comments as done.

Update diff

guotao added inline comments.Apr 17 2018, 1:10 PM
src/QuickEditor/EditorRoot.qml
77

my bad, wrong logic

guotao marked 2 inline comments as done.Apr 17 2018, 1:11 PM
rkflx added a comment.Apr 17 2018, 9:54 PM

Please also review my tool tip.

I had to read the source code to know where to look for it. What about something like this (inspired by Gwenview):

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
294

Why is this marked as "Done" when the line is still there?

guotao updated this revision to Diff 32438.Apr 18 2018, 4:36 AM

update diff

guotao marked an inline comment as done.Apr 18 2018, 4:36 AM

This is looking great! +1 for adding a Gwenview-style inline message about being able to toggle it with shift.

@rkflx @ngraham It's a big change to implement Gwenview style inline tool tip, I think we can change it in later commit.

rkflx added a comment.EditedApr 19 2018, 12:42 PM

@rkflx @ngraham It's a big change to implement Gwenview style inline tool tip, I think we can change it in later commit.

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.

Can we land this change first?

As Henrik said...

we'll wait with submitting [this patch] one until [the other patch you proposed] is done. Let us know if you need any help or have specific questions.

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

rkflx added a comment.Apr 30 2018, 5:00 PM

Please also review my tool tip.

Another idea would be to add this to the on-screen help text of the rectangle selection (but it is already quite crowded…).

Let's do just that in D12617, and clean up the on-screen text while we are at it.

rkflx accepted this revision.May 1 2018, 8:04 PM
This revision is now accepted and ready to land.May 1 2018, 8:04 PM
ngraham accepted this revision.May 1 2018, 8:08 PM

Ship it!

This revision was automatically updated to reflect the committed changes.