Differential D14846

Polish Reduce Red Eye UI
ClosedPublic
Actions

Authored by rkflx on Aug 14 2018, 4:06 PM.

Details

Reviewers
muhlenpfordt
Group Reviewers
Gwenview
Commits
R260:0c678376131d: Polish Reduce Red Eye UI
Summary

The bottom toolbar of the Reduce Red Eye tool sports a couple of
glitches and inconsistencies:

  • The vertical height is too large compared to the Crop tool.
  • After Size the colon is missing.
  • For some languages the slider can become quite long.
  • After accepting, sometimes the left text flickers, since it is updated before the layout changes back.
  • The OK button is missing a more descriptive text and more fitting icon as featured by the Crop tool.
  • Crop uses the standard Cancel wording, but here a confusing Close button is placed next to OK.
  • When proceeding to fix the second eye after clicking on OK, the Close button suddenly gains focus and thus turns blue, which was not the case for the first eye.
  • When pressing Enter without having clicked first, invalid rect is printed.

Let's fix all of those issues.

Test Plan

Issues are gone, no functional or visual regressions when playing around
with the tool.

Before:


After:

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 14 2018, 4:06 PM
rkflx created this revision.
muhlenpfordt accepted this revision.Aug 16 2018, 9:42 AM
muhlenpfordt added a subscriber: muhlenpfordt.

Code and functionality looks good to me. The described issues are all fixed.

In D14846, @rkflx wrote:
  • After accepting, sometimes the left text flickers, since it is updated before the layout changes back.

Wow, you have eyes like a hawk. ;)
I need at least 50 tries to spot this once.

  • When proceeding to fix the second eye, the Close button suddenly gains focus and thus turns blue, which was not the case for the first eye.

It took me a while to confirm this - it happens only if you accept by click on Ok/Reduce Red Eye not by pressing Return. Maybe add a hint?

lib/redeyereduction/redeyereductiontool.cpp
50

28754fa2d95e added QStringLiteral for crop here. Should we use this too?

This revision is now accepted and ready to land.Aug 16 2018, 9:42 AM
rkflx marked an inline comment as done.Aug 16 2018, 7:49 PM

Thanks for the review. I have more small patches like that slowly making it through the pipeline, I hope you don't mind me requesting yet another review every other day ;) (Still fighting with the cursor issues though, sorry for the wait in that Diff of yours…)

you have eyes like a hawk

Haha, I guess my machine is just too slow.

lib/redeyereduction/redeyereductiontool.cpp
50

Good catch, I'll add it.

It's not really important right now, but what we should do eventually is to set -DQT_USE_QSTRINGBUILDER (or rather include the corresponding ECM module) to enforce this and thus get compilation errors when someone forgets it. There are still more places where this is missing, though (in case you wanna work on that).

rkflx edited the summary of this revision. (Show Details)Aug 16 2018, 7:50 PM
rkflx updated this revision to Diff 39880.Aug 16 2018, 7:50 PM
rkflx marked an inline comment as done.
  • rebase
  • use QStringLiteral
Closed by commit R260:0c678376131d: Polish Reduce Red Eye UI (authored by rkflx). · Explain WhyAug 16 2018, 7:52 PM
This revision was automatically updated to reflect the committed changes.
ngraham added a subscriber: ngraham.Aug 16 2018, 9:35 PM

Very nice improvements.

Revision Contents
Changeset List

Diff 39881

View Options

lib/redeyereduction/redeyereductiontool.h

Loading...
View Options

lib/redeyereduction/redeyereductiontool.cpp

Loading...
View Options

lib/redeyereduction/redeyereductionwidget.ui

Loading...