Fix Advanced settings crop toolbar sometimes cut off
ClosedPublic

Authored by muhlenpfordt on Aug 18 2018, 8:36 AM.

Details

Summary

When the Crop tool is started in standard mode and then enabling
Advanced settings the window width is not updated, which can result
in a cut off toolbar.
This is caused by the design of the image tools which are not embedded
by a layout to the image view and therefore not automatically
exchanging layout/resize events.
To solve this SlideContainer::eventFilter() now handles
LayoutRequest events of the content widgets.
Updating the geometry for these events makes some layout problems in
SaveBar visible which is also derived from SlideContainer.
The message and link labels need to be adjusted to be still shrinkable
and don't force the SaveBar getting too wide.

FIXED-IN: 18.08.1

Test Plan
  • Open Crop without checked Advanced settings option
  • Set Advanced settings and check if window width is adjusted to show the whole toolbar
  • Modify 3 or more images to enable SaveBar and show all possible parts (buttons, info and links)

Diff Detail

Repository
R260 Gwenview
Branch
fix-crop-toggle-advanced (branched from Applications/18.08)
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 1964
Build 1982: arc lint + arc unit
muhlenpfordt requested review of this revision.Aug 18 2018, 8:36 AM
muhlenpfordt created this revision.

I tried to keep the appearance of the SaveBar. Maybe there are better suggestions to change the layout?

There's also a (normally invisible) second line in the SaveBar with a memory warning. I used this hack to check it (after the 2nd image change):

diff --git a/app/savebar.cpp b/app/savebar.cpp
index cce25769..52869c33 100644
--- a/app/savebar.cpp
+++ b/app/savebar.cpp
@@ -163,7 +163,7 @@ struct SaveBarPrivate
             memoryUsage += doc->memoryUsage();
         }
 
-        mTooManyChangesFrame->setVisible(memoryUsage > maxMemoryUsage);
+        mTooManyChangesFrame->setVisible(list.size() > 1);
     }
 
     void updateTopRowWidget(const QList<QUrl>& lst)

Removed unneeded leftover header

rkflx added a subscriber: rkflx.Aug 18 2018, 10:54 PM

Wow, you fixed it already! This also solves the problem of not being able to make the window smaller after disabling Advanced settings again.

The labels need to be adjusted to be shrinkable and don't force the SaveBar getting too wide.

I see where you are coming from, but I'm afraid now the common case of modifying a single image will cut off the Current image modified label much earlier, since there is now a gap between both button groups which does not get compressed. Would this be easy to fix?


As for a wide top bar in general, maybe we can improve it later. Ideas:

  • Given that even in the worst case the top bar is still narrower than the advanced Crop bar, I wonder if we could live with the window getting wider?
  • Add a FlowLayout.
  • Elide all three labels to the right. (← probably the best solution, to avoid jumping between a single and a double row layout while navigating)
muhlenpfordt edited the summary of this revision. (Show Details)

Set message label to QSizePolicy::Fixed

muhlenpfordt added a comment.EditedAug 19 2018, 11:55 AM

I see where you are coming from, but I'm afraid now the common case of modifying a single image will cut off the Current image modified label much earlier, since there is now a gap between both button groups which does not get compressed. Would this be easy to fix?

Setting the message label to QSizePolicy::Fixed prevents from shrinking but it still adjusts to the current width hint. The links will only appear if there is enough space. I think this is a good way.
Another possibility would be to add rowLayout->setStretchFactor(d->mMessageLabel, 1);. This will show the still shrinkable message label in first place and only after it's fully visible the links.


As for a wide top bar in general, maybe we can improve it later. Ideas:

  • Elide all three labels to the right. (← probably the best solution, to avoid jumping between a single and a double row layout while navigating)

I think the current layout with the fixed message label is not too big, but if it should get even narrower this would the the best way.

rkflx accepted this revision.Aug 19 2018, 8:51 PM

Setting the message label to QSizePolicy::Fixed prevents from shrinking but it still adjusts to the current width hint.

There is still an edge case where we now introduce wiggling: Make the sidebar as wide as possible, modify an image and then jump back and forth between images. Now the sidebar changes size everytime you reach the modified image. For Crop that's fine, but for the top bar which remains visible that's not so nice.

Another possibility would be to add rowLayout->setStretchFactor(d->mMessageLabel, 1);. This will show the still shrinkable message label in first place and only after it's fully visible the links.

It took me a while until I realized this referred to the previous version, i.e. with Ignored and setMaximumWidth still in place. That's exactly what I'd like to see! In my testing this reduces most of the wiggling (starting to hide buttons would probably be overkill), but still doesn't show the empty gap between the button groups. Nice!

With that change the patch can land ;)


  • Elide all three labels to the right. (← probably the best solution, to avoid jumping between a single and a double row layout while navigating)

I think the current layout with the fixed message label is not too big, but if it should get even narrower this would the the best way.

Yup. Even for the most extreme situations a user can reach via resizing, Gwenview should behave nicely. As such eliding is more elegant than simply cutting off the label…

This revision is now accepted and ready to land.Aug 19 2018, 8:51 PM
muhlenpfordt edited the summary of this revision. (Show Details)

Use stretch factor instead of fixed policy for message label

rkflx accepted this revision.Aug 20 2018, 11:16 AM
This revision was automatically updated to reflect the committed changes.
Restricted Application added a project: Gwenview. · View Herald TranscriptAug 21 2018, 6:42 AM

Yup. Even for the most extreme situations a user can reach via resizing, Gwenview should behave nicely. As such eliding is more elegant than simply cutting off the label…

(Implementing a floating layout for the crop bar is still up for grabs, though, and I don't have plans to work on that in the near future.

Note that VDG considers to roughly triple the width of each spinbox and we have 4 of them, and they want to increase font size to 11, all of which would totally destroy the toolbar layout ATM. At the same time they are praising the new Plasma Pinebook with only 1366 px horizontal resolution which we apparently now have to support – it's a bit depressing…)