D12306: Filepicker dialog proper grid icon layout

rkflx edited the summary of this revision. REPOSITORY R241 KIO BRANCH grid_layout (branched from master) REVISION DETAIL https://phabricator.kde.org/D12306 To: anemeth, #frameworks, #vdg, ngraham, rkflx Cc: abetts, rkflx, ngraham, #frameworks, michaelh, bruns

D12306: Filepicker dialog proper grid icon layout

rkflx accepted this revision. rkflx added a comment. This revision is now accepted and ready to land. Thanks, my patch from above has been applied, so LGTM now. Any additional objections or comments from #frameworks ? If not, I'll commit this

D12306: Filepicker dialog proper grid icon layout

ngraham added a comment. Me too. Let's not let the perfect be the enemy of the good here. @rkflx, are you satisfied with this now? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12306 To: anemeth, #frameworks, #vdg, ngraham, rkflx Cc: abetts, rkflx, ngraham,

D12306: Filepicker dialog proper grid icon layout

anemeth added a comment. 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. F5816535:

D12306: Filepicker dialog proper grid icon layout

rkflx added a comment. In D12306#250724 , @ngraham wrote: > Ooh, centering when there's only one column is a nice touch. Not sure what you mean? The whole patch is about centering everything, that's still happening obviously. The revert

D12306: Filepicker dialog proper grid icon layout

ngraham added a comment. Ooh, centering when there's only one column is a nice touch. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12306 To: anemeth, #frameworks, #vdg, ngraham, rkflx Cc: abetts, rkflx, ngraham, #frameworks, michaelh, bruns

D12306: Filepicker dialog proper grid icon layout

anemeth updated this revision to Diff 32680. anemeth added a comment. Revert centered icons when there is only one column, fix comment REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12306?vs=32614=32680 BRANCH grid_layout (branched from master) REVISION

D12306: Filepicker dialog proper grid icon layout

rkflx added a comment. Video for the flickering I'm talking about (recorded with no "hacks", adding `-1` will solve it): F5813789: KIO-flickering.webm REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12306 To: anemeth,

D12306: Filepicker dialog proper grid icon layout

rkflx added inline comments. INLINE COMMENTS > kdiroperator.cpp:2594-2595 > + > +// Subtract 1 px to prevent the last column from missing > +// Reduce 4 more (scaled) pixels to prevent flickering when resizing > the window > +const int viewPortWidth =

D12306: Filepicker dialog proper grid icon layout

rkflx added a comment. In D12306#250116 , @anemeth wrote: > @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

D12306: Filepicker dialog proper grid icon layout

anemeth updated this revision to Diff 32614. anemeth added a comment. 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

D12306: Filepicker dialog proper grid icon layout

rkflx requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12306 To: anemeth, #frameworks, #vdg, ngraham, rkflx Cc: abetts, rkflx, ngraham, #frameworks, michaelh, bruns

D12306: Filepicker dialog proper grid icon layout

rkflx added a comment. 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

D12306: Filepicker dialog proper grid icon layout

anemeth added a comment. 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

D12306: Filepicker dialog proper grid icon layout

anemeth updated this revision to Diff 32588. anemeth added a comment. Fix flickering for Oxygen and spacing for very narrow window. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12306?vs=32575=32588 BRANCH master REVISION DETAIL

D12306: Filepicker dialog proper grid icon layout

rkflx added a comment. 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): F5813179: KIO-resizing.webm

D12306: Filepicker dialog proper grid icon layout

anemeth updated this revision to Diff 32575. anemeth added a comment. Add comment to the workaround REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12306?vs=32488=32575 BRANCH master REVISION DETAIL https://phabricator.kde.org/D12306 AFFECTED FILES

D12306: Filepicker dialog proper grid icon layout

ngraham added inline comments. INLINE COMMENTS > anemeth wrote in 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

D12306: Filepicker dialog proper grid icon layout

anemeth added inline comments. INLINE COMMENTS > rkflx wrote in kdiroperator.cpp: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

D12306: Filepicker dialog proper grid icon layout

rkflx added a comment. 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

D12306: Filepicker dialog proper grid icon layout

ngraham added a task: T8552: Polish Open/Save dialogs. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D12306 To: anemeth, #frameworks, #vdg, ngraham Cc: abetts, rkflx, ngraham, #frameworks, michaelh, bruns

D12306: Filepicker dialog proper grid icon layout

ngraham added a dependent revision: D12326: [WIP/open dependencies] In Short View, display icons on top and increase icon size. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D12306 To: anemeth, #frameworks, #vdg, ngraham Cc: abetts, rkflx, ngraham,

D12306: Filepicker dialog proper grid icon layout

ngraham accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D12306 To: anemeth, #frameworks, #vdg, ngraham Cc: abetts, rkflx, ngraham, #frameworks, michaelh, bruns

D12306: Filepicker dialog proper grid icon layout

anemeth added inline comments. INLINE COMMENTS > ngraham wrote in kdiroperator.cpp:2253 > Why is this needed? 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. REPOSITORY R241 KIO REVISION DETAIL

D12306: Filepicker dialog proper grid icon layout

ngraham added a comment. This is great! Much better than my patch. I tested it six ways to Sunday and couldn't break it. One comment: INLINE COMMENTS > kdiroperator.cpp:2253 > + > +d->updateListViewGrid(); > } Why is this needed? REPOSITORY R241 KIO REVISION DETAIL

D12306: Filepicker dialog proper grid icon layout

anemeth added a comment. F5811818: a1.PNG REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12306 To: anemeth, #frameworks, #vdg Cc: abetts, rkflx, ngraham, #frameworks, michaelh, bruns

D12306: Filepicker dialog proper grid icon layout

anemeth updated this revision to Diff 32488. anemeth added a comment. Reduced vertical spacing. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12306?vs=32472=32488 BRANCH master REVISION DETAIL https://phabricator.kde.org/D12306 AFFECTED FILES

D12306: Filepicker dialog proper grid icon layout

ngraham added a comment. In D12306#248869 , @abetts wrote: > 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

D12306: Filepicker dialog proper grid icon layout

abetts added a comment. I also vote for a little more vertical spacing. Maybe in similar ways to what Marco Martin did recently for spacing in System Settings. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12306 To: anemeth, #frameworks, #vdg Cc: abetts, rkflx,

D12306: Filepicker dialog proper grid icon layout

ngraham edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12306 To: anemeth, #frameworks, #vdg Cc: rkflx, ngraham, #frameworks, michaelh, bruns

D12306: Filepicker dialog proper grid icon layout

rkflx added a comment. 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

D12306: Filepicker dialog proper grid icon layout

anemeth added a comment. F5811675: tiny_icons.webm REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12306 To: anemeth, #frameworks, #vdg Cc: rkflx, ngraham, #frameworks, michaelh, bruns

D12306: Filepicker dialog proper grid icon layout

anemeth updated this revision to Diff 32472. anemeth added a comment. Fixed icon spacing when icons are small. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12306?vs=32470=32472 BRANCH master REVISION DETAIL https://phabricator.kde.org/D12306 AFFECTED

D12306: Filepicker dialog proper grid icon layout

anemeth marked 2 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12306 To: anemeth, #frameworks, #vdg Cc: rkflx, ngraham, #frameworks, michaelh, bruns

D12306: Filepicker dialog proper grid icon layout

anemeth marked an inline comment as done. anemeth added a comment. Yes, this patch is kind of based on @ngraham 's patch. I'll look into the text width part. INLINE COMMENTS > rkflx wrote in kdiroperator.cpp:2595 > Thread 1 "kdialog" received signal SIGFPE, Arithmetic exception. Thanks,

D12306: Filepicker dialog proper grid icon layout

anemeth updated this revision to Diff 32470. anemeth added a comment. Fixed crash when viewport size is too small. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12306?vs=32465=32470 BRANCH master REVISION DETAIL https://phabricator.kde.org/D12306

D12306: Filepicker dialog proper grid icon layout

rkflx added subscribers: ngraham, rkflx. rkflx added a comment. 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

D12306: Filepicker dialog proper grid icon layout

anemeth edited the test plan for this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12306 To: anemeth, #frameworks, #vdg Cc: #frameworks, michaelh, bruns

D12306: Filepicker dialog proper grid icon layout

anemeth updated this revision to Diff 32465. anemeth added a comment. Removed debug commands REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12306?vs=32463=32465 BRANCH master REVISION DETAIL https://phabricator.kde.org/D12306 AFFECTED FILES

D12306: Filepicker dialog proper grid icon layout

anemeth retitled this revision from "fp" to "Filepicker dialog proper grid icon layout". anemeth edited the summary of this revision. anemeth edited the test plan for this revision. anemeth added reviewers: Frameworks, VDG. REPOSITORY R241 KIO REVISION DETAIL