Differential D15450

Sublime: Fix window growing larger by making tool view buttons shrinkable
ClosedPublic
Actions

Authored by amhndu on Sep 12 2018, 12:23 PM.

Details

Reviewers
kossebau
Group Reviewers
KDevelop
Commits
R32:b95bc1c34525: Sublime: Fix window growing larger by making tool view buttons shrinkable
Summary

Items will be shrinked in the IdealButtonBarLayout, instead of
overflowing and forcing the window larger.
Geometries are calculated so that only the bigger will be contracted
while items smaller than a relative threshold won't contract.
Consequently, the window will no longer grow when changing from Debug to Code.

IdealToolButtons: Instead of simply truncating text, they will elide text while painting.

IdealButtonBarWidget: Previously the layout attached to the object was being used to add
buttons, which for the bottom bar wrongly meant a super-layout while the
IdealButtonBarLayout added to this super-layout was being ignored, introduced a new
member to use the proper layout for all orientations.

BUG: 384711

Diff Detail

Repository
R32 KDevelop
Branch
buttonbar-shrink
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2798
Build 2816: arc lint + arc unit
amhndu created this revision.Sep 12 2018, 12:23 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptSep 12 2018, 12:23 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
amhndu requested review of this revision.Sep 12 2018, 12:23 PM
amhndu added inline comments.Sep 12 2018, 12:59 PM
kdevplatform/sublime/idealtoolbutton.cpp
46 ↗(On Diff #41470)

Although I've added this, it doesn't seem to work.

kossebau added a subscriber: kossebau.Sep 13 2018, 10:13 AM

Very clean implementation, I like this :) And from what I tested this works nicely.

Some code style feedback for now, will think through the logic tonight, but so far looks fine.

kdevplatform/sublime/idealbuttonbarwidget.cpp
255 ↗(On Diff #41470)

Why did you remove the variant for the bottom docker? I have no idea why that exists :) but then I also do not see why it should be removed, as it changes behaviour people have got used to, no?

kdevplatform/sublime/idealbuttonbarwidget.h
97 ↗(On Diff #41470)

Even while not really needed for the API used, let's use the final type IdealButtonBarLayout, otherwise the code reader could assume that potentially different subclasses of QLayout might be used with this member.

kdevplatform/sublime/ideallayout.cpp
25 ↗(On Diff #41470)

Please move to be the last include. While inconsistent throughout kdevelop codebase, the recommended order is own->kf->qt->std

201 ↗(On Diff #41470)

Please also wrap single lines with {}:

f (_items.empty()) {
    return x + currentLineWidth + r;
}
204 ↗(On Diff #41470)

Make const, or perhaps avoid intermediate var and use directly in estimation of shrink.

205 ↗(On Diff #41470)

const

207 ↗(On Diff #41470)

please explicit type definition for each variable on its own line, const for those which will not be changed later.
Same for rest of code.

208 ↗(On Diff #41470)

surplus is only used inside the if(shrink), so move declaration of it there,

212 ↗(On Diff #41470)

{ please in same line as if(), here and below

215 ↗(On Diff #41470)

Please tag as const to make clear this will not be changed, use decriptive variable name, not single char:

-> const int itemHeight

288 ↗(On Diff #41470)

{ on same line as for, please.

kdevplatform/sublime/idealtoolbutton.cpp
89 ↗(On Diff #41470)

Linebreak sneaked in?

115 ↗(On Diff #41470)

Please document the magic number 4 :)

123 ↗(On Diff #41470)

const

amhndu updated this revision to Diff 41533.Sep 13 2018, 11:46 AM

Updated patch with style suggestions.

amhndu marked 13 inline comments as done.Sep 13 2018, 11:48 AM
amhndu added inline comments.
kdevplatform/sublime/idealbuttonbarwidget.cpp
255 ↗(On Diff #41470)

Previously, the buttons in the bottom dock were added to a RightToLeft Box Layout, now they're added to the IdealButtonBarLayout, which works from left to right.
I did this to ensure consistency.

amhndu added inline comments.Sep 13 2018, 11:56 AM
kdevplatform/sublime/idealbuttonbarwidget.cpp
255 ↗(On Diff #41470)

Without it, new tools will be added to the far left of the bar, instead of the previous behavior of adding it to the right.

kossebau accepted this revision.Sep 14 2018, 11:14 PM

Some more nitpicks, but otherwise happy with this patch.
So with the nitpicks resolved, I vote for adding this.

kdevplatform/sublime/ideallayout.cpp
211

-> smallItemCount, 'count' makes it clear the variable is not a list of items.

213

int h -> const int itemHeight or const int h

222

Please add a comment to what is done here, at least sleepy code reader does not instantly get what is done here:

// distribute surplus height over large items
268

smallItemCount

270

const

282

QLayoutItem * -> QLayoutItem*`

kdevplatform/sublime/idealtoolbutton.cpp
1

Accidentally added newline? :)

103

Now that the argument event is no longer used, please add a line

Q_UNUSED(event);

so the compiler knows it should not complain.

112

Please wrap the single line body with {}:

if (toolButtonStyle() != Qt::ToolButtonTextOnly && !option.icon.isNull()) {
    iconWidth = option.iconSize.width();
}
121

Please wrap the single line branch body with {}:

if (toolButtonStyle() != Qt::ToolButtonTextOnly && !option.icon.isNull()) {
    iconHeight = option.iconSize.height();
}
46 ↗(On Diff #41470)

What exactly was the intent with this code? Given the layout code in the IdealButtonBarLayout class does not take the size policy or minimum size into account in the current logic, this would not work.

So unless the button is going to be inserted elsewhere, this added code can be removed again, or?

This revision is now accepted and ready to land.Sep 14 2018, 11:14 PM
kossebau added a comment.Sep 14 2018, 11:59 PM

And IMHO this can be considered a bug fix, which should go to the 5.3 branch (also has no new UI strings, so not breaking the string freeze).

amhndu updated this revision to Diff 41681.Sep 15 2018, 6:22 AM
amhndu marked 12 inline comments as done.

Final styling fixes.

amhndu added inline comments.Sep 15 2018, 6:27 AM
kdevplatform/sublime/idealtoolbutton.cpp
46 ↗(On Diff #41470)

You're right, IdealButtonBarLayout does not consider this, but I was initially testing with a BoxLayout, since that's what the bottom bar used, I'll remove it.

amhndu updated this revision to Diff 41682.Sep 15 2018, 6:38 AM

Remove stray new lines (hopefully final amend)

kossebau added a comment.Sep 15 2018, 8:49 AM

Time to land this as well :)

Please see https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22 for instructions how to land this patch on kdevelop's "5.3" branch (so where the example has "Applications/18.04", you would use "5.3", but I assume you got that, just playing safe :) ).

amhndu retitled this revision from Make sublime tool bar buttons shrinkable and elide text to Sublime: Fix window growing larger by making toolbar buttons shrinkable.Sep 15 2018, 10:43 AM
amhndu edited the summary of this revision. (Show Details)
amhndu retitled this revision from Sublime: Fix window growing larger by making toolbar buttons shrinkable to Sublime: Fix window growing larger by making tool view buttons shrinkable.Sep 15 2018, 10:51 AM
amhndu edited the summary of this revision. (Show Details)
Closed by commit R32:b95bc1c34525: Sublime: Fix window growing larger by making tool view buttons shrinkable (authored by amhndu). · Explain WhySep 15 2018, 10:55 AM
This revision was automatically updated to reflect the committed changes.
amhndu added a comment.Sep 18 2018, 11:51 AM

Oh, my bad.
I see you haven't pushed a fix. Should I make a new revision then ?

rjvbb added a comment.Sep 18 2018, 12:08 PM

No, I haven't pushed a fix, but I have one which seems to work just fine. Feel free to clean it up as you see fit (I see an accidental indentation change) and push it yourself.

diff --git kdevplatform/sublime/ideallayout.cpp kdevplatform/sublime/ideallayout.cpp
index 8bc43479197dfb1b018b780c116034c3f1fc7e69..5976a2c5b31aef60b9f7e49ea5560330a0531cba 100644
--- kdevplatform/sublime/ideallayout.cpp
+++ kdevplatform/sublime/ideallayout.cpp
@@ -202,12 +202,12 @@ int IdealButtonBarLayout::doVerticalLayout(const QRect &rect, bool updateGeometr
         return x + currentLineWidth + r;
     }
 
-    const bool shrink = rect.height() < sizeHint().height();
+    bool shrink = rect.height() < sizeHint().height();
 
     const int maximumHeight = rect.height() / _items.size();
     int shrinkedHeight = -1;
 
-    if (shrink) {
+    if (shrink ) {
         int smallItemCount = 0;
         const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumHeight, &smallItemCount](int acc, QLayoutItem* item) {
             const int itemHeight = item->sizeHint().height();
@@ -218,9 +218,13 @@ int IdealButtonBarLayout::doVerticalLayout(const QRect &rect, bool updateGeometr
             return acc;
         });
 
-        Q_ASSERT(_items.size() != smallItemCount); // should be true since rect.width != sizeHint.width
-        // evenly distribute surplus height over large items
-        shrinkedHeight = maximumHeight + surplus / (_items.size() - smallItemCount);
+        if (_items.size() != smallItemCount) {
+           // evenly distribute surplus height over large items
+           shrinkedHeight = maximumHeight + surplus / (_items.size() - smallItemCount);
+        } else {
+           // Only small items, no shrinkage needed
+           shrink = false;
+        }
     }
 
     for (QLayoutItem* item : _items) {
@@ -260,7 +264,7 @@ int IdealButtonBarLayout::doHorizontalLayout(const QRect &rect, bool updateGeome
         return y + currentLineHeight + b;
     }
 
-    const bool shrink = rect.width() < sizeHint().width();
+    bool shrink = rect.width() < sizeHint().width();
 
     const int maximumWidth = rect.width() / _items.size();
     int shrinkedWidth = -1;
@@ -276,9 +280,12 @@ int IdealButtonBarLayout::doHorizontalLayout(const QRect &rect, bool updateGeome
             return acc;
         });
 
-        Q_ASSERT(_items.size() != smallItemCount); // should be true since rect.width != sizeHint.width
-        // evenly distribute surplus width on the large items
-        shrinkedWidth = maximumWidth + surplus / (_items.size() - smallItemCount);
+        if (_items.size() != smallItemCount) {
+           // evenly distribute surplus height over large items
+           shrinkedWidth = maximumWidth + surplus / (_items.size() - smallItemCount);
+        } else {
+           shrink = false;
+        }
     }
 
     for (QLayoutItem* item : _items) {
kossebau added a comment.Sep 18 2018, 12:13 PM

I would like to first try to find the reason why the condition bool shrink = rect.width() < sizeHint().width(); seems to still lead us here into a situation where only "smallItems" exists?
I would suspect some rounding errors.

The patch as proposed might make the bug disappear, but let's catch it at the root :)

kossebau added inline comments.Sep 18 2018, 12:34 PM
kdevplatform/sublime/ideallayout.cpp
224 ↗(On Diff #41470)

This here misses to take buttonSpacing into account. Which for me with the Breeze style is 0, but possibly is >0 with other styles, which might also explain the assert triggering for @rjvbb

kossebau added inline comments.Sep 18 2018, 12:36 PM
kdevplatform/sublime/ideallayout.cpp
224 ↗(On Diff #41470)

Well, or rather the divide by 0 without assert :)

rjvbb added a comment.Sep 18 2018, 2:19 PM

seems to still lead us here into a situation where only "smallItems" exists?

I would suspect some rounding errors.

Possibly, but items because of any number of reasons, including the fact they are in fact small, no? You're not wrong that it's a good idea to figure out if the number of small items is determined correctly but I fear (strongly suspect) that it will be necessary anyway to introduce a check if by chance and because of some unforeseen reason there are only small items. Can you guarantee for instance that this will never be the case when the user did a style (or font) change - in my experience there can be artefacts when changing the those properties at runtime?

let's catch it at the root :)

I'd call this the root, and any thing that causes a miscalculation of the number of small items is just a trigger ;)
After all, there can be 0 small items, but why would it be impossible that there be only small items?

Revision Contents
Changeset List

Diff 41533

View Options

kdevplatform/sublime/idealbuttonbarwidget.h

Loading...
View Options

kdevplatform/sublime/idealbuttonbarwidget.cpp

Loading...
View Options

kdevplatform/sublime/ideallayout.cpp

Loading...
View Options

kdevplatform/sublime/idealtoolbutton.cpp

Loading...