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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
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
45

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
253–254

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

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

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

200–201

Please also wrap single lines with {}:

f (_items.empty()) {
    return x + currentLineWidth + r;
}
203

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

204

const

206

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.

207

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

211

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

214

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

-> const int itemHeight

283

{ on same line as for, please.

kdevplatform/sublime/idealtoolbutton.cpp
76

Linebreak sneaked in?

103

Please document the magic number 4 :)

113

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
253–254

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
253–254

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
269

smallItemCount

271

const

283

QLayoutItem * -> QLayoutItem*`

kdevplatform/sublime/idealtoolbutton.cpp
19

Accidentally added newline? :)

45

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?

90–96

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

Q_UNUSED(event);

so the compiler knows it should not complain.

101

Please wrap the single line body with {}:

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

Please wrap the single line branch body with {}:

if (toolButtonStyle() != Qt::ToolButtonTextOnly && !option.icon.isNull()) {
    iconHeight = option.iconSize.height();
}
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
45

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
207

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
207

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 41690

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...