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
ngraham | |
rkflx |
Gwenview |
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
Use Fill with landscape and portrait pictures with the window in landscape and portrait size.
No Linters Available |
No Unit Test Coverage |
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. ;) |
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... |
lib/documentview/abstractimageview.cpp | ||
---|---|---|
315 | Maybe name this variable "fill" to match the feature it's connected to? |
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. |
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. |
lib/documentview/documentviewcontroller.cpp | ||
---|---|---|
106 |
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:
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.
Gwenview's target user base is very different, I feel Darktable or RawTherapee users are more technical than the average Gwenview user. |
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? |
lib/documentview/documentviewcontroller.cpp | ||
---|---|---|
106 | We should improve our documentation on that, for now see here: https://phabricator.kde.org/D10295#206872 |
doc/index.docbook | ||
---|---|---|
263 | "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 | That's a nice idea, we should implement this someday. I've been looking for better shortcuts for ages, but 1 is already taken… |
lib/documentview/documentviewcontroller.cpp | ||
---|---|---|
106 | What about e.g. "Zoom to fit smaller side" as tooltip? |
lib/documentview/documentviewcontroller.cpp | ||
---|---|---|
106 | Good idea. smaller → narrower? |
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). |
lib/documentview/documentviewcontroller.cpp | ||
---|---|---|
106 | I love your attention to detail ;) How about "Zoom to fill screen by fitting to width or height" |
lib/documentview/documentviewcontroller.cpp | ||
---|---|---|
106 | Can you make that "window" instead of "screen"? (Just like you proposed in the docbook.) |
Thanks, looks really good now. Any more changes planned? Otherwise I'll land it.
In case you need ideas for follow-up patches:
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 🙂...