Improve grid icon layout in filepicker dialog
ClosedPublic

Authored by anemeth on Apr 18 2018, 12:17 PM.

Details

Summary

Icon grid layout now changes depending on the dialog window size.

As a side effect, this patch improves the grid spacing in icons-on-top mode by making it looser for small icons (which gives the labels more space) and tighter for large icons (which allows more to be seen at once).

BUG: 334099
Fixed-IN: 5.46

Test Plan

Before:

After:


Works just as well when scaling is used:

Diff Detail

Repository
R241 KIO
Branch
grid_layout (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
anemeth created this revision.Apr 18 2018, 12:17 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 18 2018, 12:17 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
anemeth requested review of this revision.Apr 18 2018, 12:17 PM
anemeth retitled this revision from fp to Filepicker dialog proper grid icon layout.Apr 18 2018, 12:24 PM
anemeth edited the summary of this revision. (Show Details)
anemeth edited the test plan for this revision. (Show Details)
anemeth added reviewers: Frameworks, VDG.
anemeth updated this revision to Diff 32465.Apr 18 2018, 12:26 PM

Removed debug commands

anemeth edited the test plan for this revision. (Show Details)Apr 18 2018, 12:34 PM
rkflx added subscribers: ngraham, rkflx.EditedApr 18 2018, 1:05 PM

Nice! Make sure to talk to @ngraham in D12149: Improve grid spacing in icons-on-top mode for open/save dialogs, where he worked on something similar (but also set a larger text width for small icons). We might want to pick parts from both patches, but currently they are conflicting.

However, there is one way I immediately could break your patch. Set a very large icon size, then move the places splitter all the way to the right:

Floating point exception (core dumped)

I guess that's because dividing by zero rows is not really recommended…

src/filewidgets/kdiroperator.cpp
2595
Thread 1 "kdialog" received signal SIGFPE, Arithmetic exception.
anemeth updated this revision to Diff 32470.Apr 18 2018, 1:12 PM

Fixed crash when viewport size is too small.

anemeth marked an inline comment as done.Apr 18 2018, 1:14 PM

Yes, this patch is kind of based on @ngraham 's patch.
I'll look into the text width part.

src/filewidgets/kdiroperator.cpp
2595

Thanks, fixed.

anemeth marked 2 inline comments as done.Apr 18 2018, 1:14 PM
anemeth updated this revision to Diff 32472.Apr 18 2018, 1:28 PM

Fixed icon spacing when icons are small.

rkflx added a comment.EditedApr 18 2018, 3:50 PM

Cool! Can you also tweak the vertical spacing a bit? Currently Nate displays 2 rows of text (left), while your patch shows 3 (right), resulting in fewer items visible at once. Of course the best course of action would be to make this adapt dynamically to the filename like in Dolphin, but if that is too complicated for now, I think we should go with 2 rows, because that seems to fit most situations:

Edit: And for large icons, it seems the other way round, i.e. you only display 1 row of text, but perhaps it would be better to show 2.


One more thing:

Do you plan to submit more patches to KDE in the coming months?

@rkflx I'm not sure...

I think you should definitely think about applying for a developer account ;)

ngraham edited the summary of this revision. (Show Details)Apr 18 2018, 3:54 PM
abetts added a subscriber: abetts.Apr 18 2018, 3:59 PM

I also vote for a little more vertical spacing. Maybe in similar ways to what Marco Martin did recently for spacing in System Settings.

I also vote for a little more vertical spacing. Maybe in similar ways to what Marco Martin did recently for spacing in System Settings.

Did you mean a little less? @rkflx is objecting that the vertical spacing is too high in this patch compared to mine. I rather agree, and think that 2 rows for the filename is just fine--especially for reasonable icon sizes (icons-on-top mode is only really useful with large icons and previews enabled).

anemeth updated this revision to Diff 32488.Apr 18 2018, 5:14 PM

Reduced vertical spacing.

This is great! Much better than my patch. I tested it six ways to Sunday and couldn't break it. One comment:

src/filewidgets/kdiroperator.cpp
2253

Why is this needed?

anemeth added inline comments.Apr 18 2018, 6:50 PM
src/filewidgets/kdiroperator.cpp
2253

Without this the icon grid gets only updated when the zoom slider is moved, and it needs to get updated when the window is resized as well.

ngraham accepted this revision.Apr 18 2018, 6:51 PM
This revision is now accepted and ready to land.Apr 18 2018, 6:51 PM

Thanks, works great for me, apart from two minor inline comments.

Resizing is still a bit less smooth than in Dolphin because of the immediate updates, but I think that's okay for now.

Anybody from Frameworks having any additional objections on code or behaviour?

src/filewidgets/kdiroperator.cpp
2592

The + 1 looks a bit weird. Taking it out results in much flickering, so I guess we have to keep it. However, it should get a comment and/or possibly be moved elsewhere (as it is not about the scrollBarWidth).

2593

There seems to be an issues with the Oxygen style, where upon resizing the window an additional scrollbar (?) width is added on the right, resulting in the items not being centered anymore.

You can look into it, but if there is no easy solution (or there is a bug in Oxygen), I'm also okay with the current state.

anemeth added inline comments.Apr 19 2018, 4:44 PM
src/filewidgets/kdiroperator.cpp
2593

I tried other widget styles too.
They all share this issue where the space for the scrollbar seems to be reserved at all times.
This is a workaround for it.
Maybe this is a QListView thing?
I really hope the VDG is OK about the little gap at the right side.

ngraham added inline comments.Apr 19 2018, 4:46 PM
src/filewidgets/kdiroperator.cpp
2593

I think it's OK if avoiding it is a nightmare.

anemeth updated this revision to Diff 32575.Apr 19 2018, 4:48 PM

Add comment to the workaround

rkflx added a comment.Apr 19 2018, 5:00 PM

Are we talking about the same thing? For me the issue is only with Oxygen, but not with Breeze or Fusion (look at the gap on the right which is not there upon start, but after resizing does not go away anymore):

As far as I see the +1 is about a different issue?

anemeth updated this revision to Diff 32588.Apr 19 2018, 6:11 PM

Fix flickering for Oxygen and spacing for very narrow window.

I see what you are talking now.
The reason Oxygen has this issue but Breeze (or others ) not is because QListView::contentsRect() is reporting the wrong size.
At first I used QListView::viewport()::width() to get the size, but it was unreliable and sometimes jumps around randomly (Qt bug?).
I tried other size getter functions and contentsRect() was the only one that worked consistently... for Breeze at least... not for Oxygen.
This causes flickering when Oxygen is used, I don't know if you noticed it.
The good news is that I found a workaround. The bad news that it further increases the gap at the right by 4 pixels.
I also found a fix for the issue in the video.

rkflx added a comment.Apr 19 2018, 9:18 PM

Sorry for the back and forth, it seems this is all a bit tricky to get right…

Now I get a horizontal scrollbar for small window sizes and big icons (independent from the widget style in use) :(

The 5 * devicePixelRatioF sounds odd, and adding an additional scrollBarWidth to width looks a bit fishy, too, because this is not only an issue for 1 column of items, but happens with every width upon resize. No hacks, please (at least no ugly ones ;)

Let's go with the version we had before. IMO it's better to be a bit off-center for Oxygen after resizing, than to get a scrollbar everywhere and have this ugly code. I guess this has to be fixed in Oxygen eventually, and not worked around in KDirOperator.

For the + 1 in const in scrollBarWidth, please move it to const int viewPortWidth (i.e. it becomes -1 there), and as a comment add Subtract 1 px to prevent flickering when resizing the window. This way future readers of the code will know what this is about. (The scrollbar width is reported correctly, so it should be left alone.)


There is one more issue: For Oxygen, re-showing the dialog results in one column missing from displaying, it only shows up after the window has been resized. We are in a dilemma here:

  • We cannot ship the file dialog without it working on Oxygen. This is no cosmetic issue, but a serious bug.
  • We should not add a hack (i.e. 5 * itemView->devicePixelRatioF()) for every style, to work around a bug which only happens with Oxygen.

For now we have no choice but to add this hack (for me, 4 instead of 5 was enough). Nevertheless, please document it properly (see my suggestion below).

Also, please open a bug (and add a link here) or get otherwise in contact with Hugo (maintainer of Oxygen), so he can share his wisdom and give some tips.


Patch:

diff --git a/src/filewidgets/kdiroperator.cpp b/src/filewidgets/kdiroperator.cpp
index db07a311..ea6373dc 100644
--- a/src/filewidgets/kdiroperator.cpp
+++ b/src/filewidgets/kdiroperator.cpp
@@ -2589,13 +2589,16 @@ void KDirOperator::Private::updateListViewGrid()
         const int height = itemView->iconSize().height() + metrics.height() * 2.5;
         const int minWidth = qMax(height, metrics.height() * 5);
 
-        // this is needed for a workaround to an issue where the scrollbar area seems to be reserved at all times
-        const int scrollBarWidth = itemView->verticalScrollBar()->sizeHint().width() + (5 * itemView->devicePixelRatioF());
+        const int scrollBarWidth = itemView->verticalScrollBar()->sizeHint().width();
 
-        const int viewPortWidth = itemView->contentsRect().width() - scrollBarWidth;
+        // Subtract 1 px to prevent flickering when resizing the window
+        // For Oxygen a column is missing after showing the dialog without resizing it,
+        // therefore subtract 3 more (scaled) pixels
+        const int viewPortWidth = itemView->contentsRect().width() - scrollBarWidth
+                                  - 1 - 3 * itemView->devicePixelRatioF() ;
         const int itemsInRow = qMax(1, viewPortWidth / minWidth);
         const int remainingWidth = viewPortWidth - (minWidth * itemsInRow);
-        const int width = minWidth + (remainingWidth / itemsInRow) + ( itemsInRow == 1 ? scrollBarWidth : 0);
+        const int width = minWidth + (remainingWidth / itemsInRow);
 
         const QSize itemSize(width, height);
rkflx requested changes to this revision.Apr 19 2018, 9:26 PM
This revision now requires changes to proceed.Apr 19 2018, 9:26 PM
anemeth updated this revision to Diff 32614.Apr 19 2018, 10:02 PM

Move scrollbar width correction to viewPortWidth.

@rkflx the 3px is causing flickering on Oxygen, 4px does not.
Also I can't reproduce the missing last column issue for Oxygen.
The itemsInRow == 1 part is for when there is one column only it should be centered, so I left
that in.

@rkflx the 3px is causing flickering on Oxygen, 4px does not.

I don't see this, but I'm also fine with 4px.

Also I can't reproduce the missing last column issue for Oxygen.

Video for missing column (4 * removed):

Video for horizontal scrollbars:

The itemsInRow == 1 part is for when there is one column only it should be centered, so I left
that in.

No, please remove it. It's wrong because:

  • It only affects Oxygen (but the problem should be fixed there, not worked around).
  • Even for Oxygen it's not working right, because why would it be centered only for one column, while the items are off-centered for all other column counts?
  • It causes horizontal scrollbars for every widget style (see above).
rkflx added inline comments.Apr 19 2018, 10:26 PM
src/filewidgets/kdiroperator.cpp
2594–2595

You got the reasoning wrong, please see my patch again and switch it around. 1px is against flickering, 4 px against the column problem (and include "Oxygen"!).

Video for the flickering I'm talking about (recorded with no "hacks", adding -1 will solve it):

anemeth updated this revision to Diff 32680.Apr 20 2018, 9:50 PM

Revert centered icons when there is only one column, fix comment

Ooh, centering when there's only one column is a nice touch.

rkflx added a comment.EditedApr 21 2018, 6:18 AM

Ooh, centering when there's only one column is a nice touch.

Not sure what you mean? The whole patch is about centering everything (unless there are too few items, of course), that's still happening. The revert was only about a bug affecting Oxygen (small offset after changing window width), which made it worse for every other style.

The same thing with the scrollbar area constantly reserved can be reproduced on Windows in a standalone Qt app using QListView, so it is definitely a Qt issue.
It could be fixed by creating a new widget for this, but I'm not up for that task.

I think the current solution is "good enough".

Me too. Let's not let the perfect be the enemy of the good here. @rkflx, are you satisfied with this now?

rkflx accepted this revision.Apr 22 2018, 8:24 PM

Thanks, my patch from above has been applied, so LGTM now.

Any additional objections or comments from Frameworks? If not, I'll commit this on Wednesday on behalf of @anemeth.


@anemeth Will review https://phabricator.kde.org/D12328 and https://phabricator.kde.org/D12321 in the next days, sorry for the delay…

This revision is now accepted and ready to land.Apr 22 2018, 8:24 PM
rkflx edited the summary of this revision. (Show Details)Apr 22 2018, 8:26 PM
rkflx retitled this revision from Filepicker dialog proper grid icon layout to Improve grid icon layout in filepicker dialog.
This revision was automatically updated to reflect the committed changes.