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
Details
Details
- Reviewers
rkflx abetts - Group Reviewers
Frameworks VDG
Tested with a new user account and default settings. Open Gwenview, Choose File → Open, then Settings button → Icon Position → Above file name
Before, default tiny icons:
After, default tiny icons, default 10pt Noto Sans:
After, default tiny icons, 9pt Noto Sans:
After, default tiny icons, 11pt Noto Sans:
Before: large icons:
After: large icons:
Before: huge icons:
After, huge icons:
Before, resizing:
After: resizing:
Note: the default dialog size is 2 pixels too narrow to properly accommodate an additional row of icons at their largest size both with and without this patch. I'll be submitting another patch to improve that.
No change for any other view modes
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Branch
- arcpatch-D12149
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
New version looks much better, no doubt. The only thing you may be asked about here is magical number 120, what is it? Some minimum width for item?
Comment Actions
Yes, I'm afraid it's a magic number: the minimum width of each cell. I'm open to better approaches.
Comment Actions
FWIW I wrestled with the issue prior to submitting the patch, but couldn't come up with anything better. The issue here that we really do need a minimum width of at least 110px or else there's simply never enough room for titles with small icons, but there's a lack of granularity in the provided icon sizes for large values: KIconLoader::SizeEnormous is 128px--which is just a bit too big--but the next-smallest size is KIconLoader::SizeHuge--which is only 64px and not big enough.
I'm not sure it would be worth it to try to programmatically calculate something in between (e.g. ceil(KIconLoader::SizeEnormous * 0.85) because that would just be using a different magic value.
FWIW, the existing code had a magic value for the width too: height * 1.5. Maybe we can live with the different 120px magic value here?
| src/filewidgets/kdiroperator.cpp | ||
|---|---|---|
| 2590 | If you need a minimum width to accomodate a minimum number of characters, you could use a value proportional to font size, e.g. "8 * metrics.height()" will give you room for about 15 characters (they are usually twice as tall as wide). | |
| src/filewidgets/kdiroperator.cpp | ||
|---|---|---|
| 2590 | Excellent idea! If we have a magic value, it should definitely be tied to the font size. | |
Comment Actions
Can we also create another patch that can add more vertical spacing between the items and the top edge of the field?
Comment Actions
@ngraham Before I look at this in detail: Did you aim for a spacing consistent with Dolphin? Because on a short try with different icon sizes in both Dolphin and the file dialog, sometimes one was wider and sometimes the other…
Edit: I'm not saying the spacing is bad or anything, it's just something I noticed. Maybe it's because Dolphin has more advanced calculations, so your approach for the dialog is fine for now. (I'm not talking about how Dolphin auto-adapts to the window size.)
Comment Actions
Ok, forget about Dolphin, which for small sizes IMO could be much more dense than it is now.
However, can we still tweak the file dialog a bit?
My issue is that between 16px and 72px there is absolutely no change in both the number of characters displayed, as well as the number of columns for a given window width (best tested with a abcdefghijklmnopqrstuvwxyz1234567890 folder name). The purpose of the slider is not to only control the vertical density and the icons size, but also be able to change the horizontal density.
Obviously the old behaviour led to too many linebreaks for small sizes, but a better compromise for me would be to use * 5.5 instead of * 6. This way you'll lose roughly 3 characters, but move the limit down to 64px. As a nice side effect, you'll fit 5 columns instead of 4 for the default window size and with 32px icons.
Apart from that, I really tried to break it, but failed. Well done!
Code mostly LGTM.
| src/filewidgets/kdiroperator.cpp | ||
|---|---|---|
| 2591 | You could avoid repeating QSize: const QSize itemSize(qMax(minWidth, width), height); Alternatively use auto. | |