Replace "Fit Width" feature with "Fill"
ClosedPublic

Authored by slenz on Feb 25 2018, 11:38 PM.

Details

Summary

Fit width is arguably less useful than a more general Fill feature. This replaces "Fit Width" with "Fill", fitting width or height, depending on what fills the window.

FEATURE: 195579

Test Plan

Use Fill with landscape and portrait pictures with the window in landscape and portrait size.

Diff Detail

Repository
R260 Gwenview
Branch
fitwidthtotfill (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
slenz requested review of this revision.Feb 25 2018, 11:38 PM
slenz created this revision.
slenz added inline comments.
lib/documentview/documentview.cpp
458

Was there a reason this was split into if-else?

lib/zoomwidget.cpp
157

This switches D8306 to the "equal width" option, as Fill should be about as short as Fit, weighing the the space vs symmetry decision towards symmetry (unless some language is an exception, in which case we have to switch back...).

Works as described and I can't spot any technical issues.
Just a hint to the BUG-keyword: maybe it works but policy says uppercase.

lib/documentview/documentview.cpp
458

Introduced while adding "FitToWidth" in 3e10699ac37c but I can't see any reason too.

lib/zoomwidget.cpp
157

Is there a more legible way for this? Sorry, don't have a concrete proposal just wondering. ;)

slenz edited the summary of this revision. (Show Details)Feb 27 2018, 9:59 AM
slenz added inline comments.Feb 27 2018, 10:29 AM
lib/zoomwidget.cpp
157

We could use c++11 features and do std::max({x,y,z}). But as far as i can tell we still support Qt 5.6, which doesn't require a C++11 compiler so this is probably the best way...

ngraham edited the summary of this revision. (Show Details)Feb 27 2018, 5:39 PM

Works great, seems like an improvement to me. One comment below.

ngraham added inline comments.Feb 27 2018, 6:05 PM
lib/documentview/abstractimageview.cpp
315

Maybe name this variable "fill" to match the feature it's connected to?

slenz updated this revision to Diff 28210.Feb 27 2018, 7:05 PM
  • Rename fill ratio variable
ngraham accepted this revision as: ngraham.Feb 27 2018, 7:47 PM
ngraham added a subscriber: rkflx.

Looks and works great. Let's see what @rkflx has to say before we land this.

This revision is now accepted and ready to land.Feb 27 2018, 7:47 PM
rkflx requested changes to this revision.Feb 27 2018, 10:41 PM

Thanks for the patch, works really great. Amazing how many files this touches. Some minor comments (as usual ;).

lib/documentview/documentview.cpp
458

There are more spots to DRY like this. Material for a follow-up patch!?

lib/documentview/documentviewcontroller.cpp
106

Could you try to come up with a better tooltip, more akin to the one for 100%?

106

Also: While 3e10699ac37c did not add anything to doc/index.docbook (grep for slider at the bottom), I think it might be worth explaining what the button does, as it may not be obvious to every user unless they are adventurous enough to Rotate when experimenting.

108

Perhaps add (fit width or height) after to fill?

lib/zoomwidget.cpp
157

Haha, that's exactly how I implemented it for my screenshot back in October. I tried it with GCC 4.8 + Qt 5.6 and found that CMake turns on C++11 mode for us anyway. AFAIK Qt is much more conservative than we are. Let's try the initializer list approach for now.

This revision now requires changes to proceed.Feb 27 2018, 10:41 PM
slenz added inline comments.Feb 27 2018, 11:25 PM
lib/documentview/documentviewcontroller.cpp
106

Would it not be better to change all tooltips to something like Zoom to fit/fill screen and Zoom to actual size (Lightroom actually uses "Zoom to 100%")? I can't come up with something good that is similar to the one for 100%.

Slightly off topic, but I don't think anyone actually thinks of themselves as viewing "documents" in an image viewer, so that standard text suits us quite badly anyways.

rkflx added inline comments.Feb 27 2018, 11:38 PM
lib/documentview/documentviewcontroller.cpp
106

documents

Yeah, that's not ideal. What I meant was not the text, but the fact the explanation is longer.

IMO the tooltips serve multiple purposes:

  • Mention that the button will "Zoom", because there is no space for that on the buttom. (That's achieved already.)
  • Clarify what "Fit" and "Fill" actually are, because not every user might understand the concepts and some are scared to break something if they click on it. Experts obviously won't bother with the tooltips at all. (That's where we can improve.)

Let's keep it for now as is, perhaps later we'll have a better idea for the wording. I don't want to only repeat what's on the button, but phrase it differently.

Lightroom

Gwenview's target user base is very different, I feel Darktable or RawTherapee users are more technical than the average Gwenview user.

slenz updated this revision to Diff 28228.Feb 27 2018, 11:48 PM
  • Update docbook with new button
  • Improve description for icon text
  • Switch to c++11 max function
slenz marked 9 inline comments as done.Feb 27 2018, 11:56 PM
slenz added inline comments.
lib/documentview/documentviewcontroller.cpp
106

Updated but untested. I can't get khelpcenter to actually use my new documentation instead. Perhaps some shell variables I'm missing?

rkflx added inline comments.Feb 28 2018, 12:22 AM
lib/documentview/documentviewcontroller.cpp
106

We should improve our documentation on that, for now see here: https://phabricator.kde.org/D10295#206872

rkflx added inline comments.Feb 28 2018, 12:30 AM
doc/index.docbook
263 ↗(On Diff #28228)

"Cropping" is not ideal, as it is not the Crop tool. Some phrasing from https://bugs.kde.org/show_bug.cgi?id=195579#c0, perhaps?

neccecary → necessary

263 ↗(On Diff #28228)

That's a nice idea, we should implement this someday. I've been looking for better shortcuts for ages, but 1 is already taken…

muhlenpfordt added inline comments.Feb 28 2018, 8:24 AM
lib/documentview/documentviewcontroller.cpp
106

What about e.g. "Zoom to fit smaller side" as tooltip?

rkflx added inline comments.Feb 28 2018, 8:30 AM
lib/documentview/documentviewcontroller.cpp
106

Good idea.

smaller → narrower?

slenz added inline comments.Feb 28 2018, 8:41 AM
lib/documentview/documentviewcontroller.cpp
106

Not really true? Would depend on the current combination of window aspect ratio to image aspect ratio (4:3 image on a 16:9 window does fit width even if that is bigger).

rkflx added inline comments.Feb 28 2018, 8:49 AM
lib/documentview/documentviewcontroller.cpp
106

I love your attention to detail ;)

How about "Zoom to fill screen by fitting to width or height"

slenz updated this revision to Diff 28246.Feb 28 2018, 9:02 AM
  • Update documentation to avoid crop confusion
  • More elaborate tooltip
  • Use proper capitalization for tooltips
rkflx added inline comments.Feb 28 2018, 9:05 AM
lib/documentview/documentviewcontroller.cpp
106

Can you make that "window" instead of "screen"? (Just like you proposed in the docbook.)

slenz updated this revision to Diff 28247.Feb 28 2018, 9:08 AM
  • Screen>Window
slenz edited the summary of this revision. (Show Details)Feb 28 2018, 9:13 AM
slenz edited the summary of this revision. (Show Details)
rkflx accepted this revision.Feb 28 2018, 9:16 AM

Thanks, looks really good now. Any more changes planned? Otherwise I'll land it.

In case you need ideas for follow-up patches:

  • Dry ifs (as mentioned).
  • Add Fill to the View menu.
  • 100% should not be active anymore after changing zoom again, i.e. make the behaviour consistent with Fit and Fill.
This revision is now accepted and ready to land.Feb 28 2018, 9:16 AM
slenz added a comment.Feb 28 2018, 9:19 AM

Thanks, looks really good now. Any more changes planned? Otherwise I'll land it.

I'm done!

In case you need ideas for follow-up patches:

  • Dry ifs (as mentioned).
  • Add Fill to the View menu.
  • 100% should not be active anymore after changing zoom again, i.e. make the behaviour consistent with Fit and Fill.

Got an exam coming up now, but maybe later 🙂...

This revision was automatically updated to reflect the committed changes.
Restricted Application added a subscriber: kde-doc-english. · View Herald TranscriptMar 12 2018, 6:57 PM