Allow to double-click to accelerate crop and red-eye modifications
ClosedPublic

Authored by rkflx on Aug 12 2018, 6:27 AM.

Details

Summary

Currently double-clicking will switch to fullscreen when a tool like
Crop or Reduce Red Eye is used. It would be handy to allow
to accept the modifications without having to tediously position the
cursor over the corresponding button each time.

By using double-click to accept just like it's done for Spectacle's
Rectangular Region capture mode, users can improve the efficiency of
their primarily mouse-based workflow. In particular fixing multiple red
eyes, e.g. in a group picture, will become much faster. In GIMP's
Crop, clicking also accepts (albeit a single click is enough).

Triggering fullscreen can still be done before opening the tool, with a
shortcut or with the toolbutton, and for Crop when clicking
outside the selection. Furthermore, there is the possibility to
Undo if the double-click was unintentional.

To make users aware of the feature, for Reduce Red Eye the inline
help text is changed (at the same time removing the to-be-avoided "you"
in the string). For Crop the docbook will be adapted, since there
is not enough space in the UI for a subtle inline pointer (in a later
patch).

Test Plan

Double-click in the Crop and Reduce Red Eye tools after
modifying the image. Gwenview should apply the changes immediately.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rkflx requested review of this revision.Aug 12 2018, 6:27 AM
rkflx created this revision.
rkflx added a comment.Aug 12 2018, 6:28 AM

(I'm aware that the size slider for Reduce Red Eye now is quite long. There will be a follow-up fixing that along with a bunch of other issues.)

Very nice improvement. Seems to work perfectly in my testing. I have a string change request below:

Looks good to me. Works as stated and I can't find any issues, also zooming by Ctrl+DoubleClick still works.

Very nice improvement. Seems to work perfectly in my testing. I have a string change request below:

Hm - for me the rest of your comment seems to got lost. ;)


First I thought your patch mysteriously does nothing - but it was just a changed path and I started the wrong version.
Since b9aee44dfbd8 the Gwenview binary output moved from build/app/gwenviewto build/bin/gwenview (along with all other binaries).

lib/crop/croptool.cpp
411

Empty line here but not in RedEyeReductionTool? ;)

lib/documentview/abstractrasterimageviewtool.cpp
57–61

This is not needed since both derived classes override mouseDoubleClickEvent().
But maybe we should explicitly set a default for all event functions here (of course in another patch). So I think it's good to keep it.

ngraham added inline comments.Aug 13 2018, 10:53 AM
lib/redeyereduction/redeyereductionwidget.ui
64

This is a comma splice.

Aside from that, how about phrasing this in a slightly different way that makes it clearer which clicks do what? I'm thinking about something like this:

"Click on the red eye to correct it and then adjust the size of the correction; double-click to correct it immediately."

...But maybe that's too long?

rkflx marked an inline comment as done.Aug 13 2018, 11:19 AM

Looks good to me. Works as stated and I can't find any issues, also zooming by Ctrl+DoubleClick still works.

Nice, I did not think of testing that ;)

Since b9aee44dfbd8 the Gwenview binary output moved from build/app/gwenviewto build/bin/gwenview (along with all other binaries).

Interesting, I was not aware of the sideeffect of that unreviewed patch (probably affects the test binaries too). But I think it's fine, it's what most other repos are doing nowadays.

lib/documentview/abstractrasterimageviewtool.cpp
57–61

Yes, this was the intention. We should avoid having to submit something like D14286 again.

Let's fix the rest too (whoever gets around to it first).

lib/redeyereduction/redeyereductionwidget.ui
64

This is a comma splice.

Ah, thanks for the hint.

...But maybe that's too long?

Obviously too long ;) How about:

"Click on [a] red eye to correct it. Double-click to correct it immediately."

ngraham added inline comments.Aug 13 2018, 3:00 PM
lib/redeyereduction/redeyereductionwidget.ui
64

I think with that, the distinction between "correct it" and "correct it immediately" is not obvious. How about:

"Single-click to correct a red eye interactively; double-click to correct it immediately."

rkflx marked an inline comment as done.Aug 13 2018, 8:35 PM
rkflx added inline comments.
lib/redeyereduction/redeyereductionwidget.ui
64

Okay, let's step back a bit because I think it's getting more convoluted with each iteration. Problems with the last proposal:

  • You can also double click after you corrected interactively, and double-clicking is already kind of interactive, there is no real distinction between both modes.
  • With that wording users don't know where to click anymore.
  • "Single-click to correct" implies the fix will be applied at once, but you actually have to confirm the correction later on.

Basically I wanna convey two points:

  • You have to click on an eye at least once (because it's not yet automatic).
  • Besides clicking on "OK" (which I plan to rename to "Reduce Red Eye" in a later patch), there is also the option to double-click.

Let's keep it short and sweet:

Click on a red eye and choose a size, or double-click to confirm instantly.

The operator precedence between "and" and "or" is a bit vague and ambiguous on purpose, so it nicely fits both cases. Would this be acceptable?

rkflx updated this revision to Diff 39732.Aug 14 2018, 4:01 PM
rkflx marked 5 inline comments as done.
  • Update wording with my suggestion from yesterday.
  • Remove empty line.
ngraham added inline comments.Aug 14 2018, 8:42 PM
lib/redeyereduction/redeyereductionwidget.ui
64

Sorry for the delayed response. Very busy at Akademy this week. I think your wording is almost perfect, but could we change "confirm" to "correct"? "Confirm" implies a thing to be confirmed, but that thing has not already been established.

rkflx updated this revision to Diff 39750.Aug 14 2018, 8:45 PM

confirm -> correct

ngraham accepted this revision.Aug 14 2018, 8:50 PM

+1 ship it!

This revision is now accepted and ready to land.Aug 14 2018, 8:50 PM
This revision was automatically updated to reflect the committed changes.