In the filepicker dialog hide the previews when the icons are too small.
Details
- Reviewers
ngraham rkflx elvisangelaccio markg - Group Reviewers
VDG Frameworks Dolphin - Maniphest Tasks
- T8552: Polish Open/Save dialogs
- Commits
- R241:2559a2873962: Hide file preview when icon is too small
R241:f472f5490125: Hide file preview when icon is too small
Diff Detail
- Repository
- R241 KIO
- Branch
- conditional_preview (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage
In fact, perhaps we should consider this new automatically-disable-previews-at-small-sizes behavior to be a replacement for Dolphin's clunky independent-sliders-for-previews-and-non-previews feature. I suspect it was implemented that way to allay a similar concern (previews being useless at small sizes) but IMHO the solution in this patch is much more elegant and user-friendly
Not sure I'd agree to that. Staying in a view mode and having the preview toggle make the items larger for previews, and smaller for regular icons, is quite neat! If at all this feature should be ported to the file dialog too…
Nonetheless, both functionalities are independent of each other. Remembering a different slider position per preview toggle state (which applies to all kinds of icons sizes) has nothing to do with the fact that for small icon sizes it does not make sense to show thumbnails.
That looks really fancy! :)
Yet i have to give it a -1..
You are overwriting a setting that the user had explicitly set (show preview). That will result in a "huh, why is the preview off all of a sudden?" responses which will lead to bug reports.
I would consider it a bug if the view is going to decide for me when i want to see previews and when not.
Also, i really would not base this logic on fonts. I can have smaller fonts just because i might like that (or bigger, same argument) which would then determine when the preview is switched off.
I get that this is a tricky area, but it would be much better if you can somehow determine if the preview is going to be smaller then - lets say - 1 by 1 centimeters (in physical size, therefore you need to know the scaling ratio and the dpi) or in inch it would be 0.39 by 0.39. That would be a more logical decision to turn on/off previews.
This logic breaks in really high resolution displays (take smartphones for example) where a 1x1 cm block can (and should!) easily show you an image. But lets lets consider that a out-of-scope issue as dolphin isn't on the smartphone (that i'm aware of), just something that might need to be considered in the future.
That being said, having an "auto preview" (aka, the view knows best, your implementation) would be awesome! But not as overridden user setting.
Perhaps the preview button needs to become a tri-state button:
- Off
- On (always a preview, the user knows best)
- Auto (the view decides when you see a preview)
I have no clue how a tri-state button would look in dolphin though :) Or even if the Breeze theme has a graphical representation for it.
That's simply a bug with the patch: Enable previews, set small icon set, click Cancel, reopen dialog, set large icon size. Previews should still be enabled, but they are not. I guess this needs to track separately what the user set vs. what the button says (as for small sizes the thumbnails are turned off by the slider, not by the user) when writing to the config.
This logic breaks in really high resolution displays (take smartphones for example) where a 1x1 cm block can (and should!) easily show you an image. But lets lets consider that a out-of-scope issue as dolphin isn't on the smartphone (that i'm aware of), just something that might need to be considered in the future.
The patch is basing the cut-off point on the logical size, not on the physical size, and as such should work just fine in HiDPI mode. Also, in general the font size is a good approximation on what sizes users prefer. There is no need to make every single aspect of our UI's behaviour configurable, because we should pick good defaults.
That being said, having an "auto preview" (aka, the view knows best, your implementation) would be awesome! But not as overridden user setting.
Perhaps the preview button needs to become a tri-state button:
- Off
- On (always a preview, the user knows best)
- Auto (the view decides when you see a preview)
I think this was discussed before, and rejected? @ngraham Can you remember?
Indeed, it's working now. Sorry for the confusion. I guess I was testing D12328 then. Normally due to the dependent revision it should pick up the change, but because of the rebase gone wrong, it did not work apparently.
Let me rephrase it then :)
The user explicitly clicks on the preview button. If the user then zooms in/out that same preview button might change to what the view thinks is best.
That is unexpected behavior.
A tri-state would prevent that.
For the record, if it becomes a tri-state i would set it to auto (this implementation, the view knows best) by default.
I disagree, adapting the interface dynamically is good design.
A tri-state would prevent that.
For the record, if it becomes a tri-state i would set it to auto (this implementation, the view knows best) by default.
Suppose previews are on. Now the user clicks on the button, wanting to turn previews off. Sadly, nothing will happen, because the user just switched into "automatic" mode. That's not something we should implement!
Besides that, tristate is only a property of QCheckBox, but not of QToolButton. (And see also confusion here: https://www.reddit.com/r/kde/comments/8c5ael/there_are_several_settings_in_kde_that_have_a/.)
Thus this would need to be a separate config option, making everything even more complicated. I don't get why you want to see those tiny previews? Or is it simply out of principle that you want to control the thumbnails, even if they are not useful?
@markg One more thing: This patch is a prerequisite for D12328: Enable preview by default in the filepicker dialog, because otherwise for small icons the previews would also be turned on by default, which we don't want obviously (even you said by default for tiny icons thumbnails don't make any sense).
D12328 again is required for the vision outlined T8552: Polish Open/Save dialogs, i.e. having a details tree view mode with tiny icons, and a visual text-under-icons mode with large icons and previews.
If you think we should abandon the dialog overhaul in it's entirety, please comment on that task, and maybe add a suggestion on what we should work on instead for the chosen focus goal T6831: Top-notch usability and productivity for basic software.
I agree but not for a button that the user controls.
It should either be a tri-state or a "auto preview" button.
You should never change states that users had set explicitly (at least, that's my opinion).
It will only cause confusion and becomes a nightmare when saving settings as you then already need to maintain if a button was changed by the user or code logic.
In fact, the current patch already adds class-wide bools to check for this thus internally it already needs a tri state.
A tri-state would prevent that.
For the record, if it becomes a tri-state i would set it to auto (this implementation, the view knows best) by default.Suppose previews are on. Now the user clicks on the button, wanting to turn previews off. Sadly, nothing will happen, because the user just switched into "automatic" mode. That's not something we should implement!
Besides that, tristate is only a property of QCheckBox, but not of QToolButton. (And see also confusion here: https://www.reddit.com/r/kde/comments/8c5ael/there_are_several_settings_in_kde_that_have_a/.)
Thus this would need to be a separate config option, making everything even more complicated. I don't get why you want to see those tiny previews? Or is it simply out of principle that you want to control the thumbnails, even if they are not useful?
With a two-state button you either give me control or you don't, anything in between is potentially unexpected behavior.
If you want smarter ways then two-states then it should either be represented in a button that can handle it (a tri-state) or a new button altogether with the downside that you get two (at first sight seemingly the same) buttons.
As for the vision, i'm perfectly fine with that :)
Just as long as it doesn't cause unexpected behavior which i think this one as implemented now will cause. That doesn't make the vision wrong, it merely means the technical execution of some parts might need a little tweaking.
We already disable options where they do not make sense, see the Icon position setting which is only available in select view modes.
The ability to preview could similarly be disabled where it does not make sense. I highly doubt this will cause confusion for users.
Ohh, but i'm not against that :)
I merely don't want one button to be changed by both the user and some code logic.
It's the same for Icon position: Even if the user does not change from Above filename to Next to filename the code logic when changing the radio button (which would equal moving the zoom slider) to another view mode will change the icon position anyway, because for Detailed View you cannot have icons above the filename.
This automatic logic is already there. You are simply objecting to not allowing previews for small icons. Yes, we will remove this possibility which was there before. No, I don't think this is bad, unless you can come up with reasons why anyone would want totally undecipherable previews.
Edit:
a new button altogether
It does not make any sense to add a button controlling something which is not useful, giving the space constraints in toolbars and menus. We should only have meaningful and essential options, and not a spaceship cockpit.
Then you make it impossible (ultimately, not with this patch though) to for instance browse through folders with small icons (say icon sets).
That cannot possibly be desired...
What file browser should people then use to view small icons?
Or should they just never do that and be forced to use gwenview (or some alternative)?
Then you make it impossible (ultimately, not with this patch though) to for instance browse through folders with small icons (say icon sets).
We have an explicit icon chooser dialog for this task, using the file dialog is not the recommended way for apps to select an icon. Also, as you may have noticed, the file dialog does not show previews of SVGs for any size, making your point moot.
As for choosing PNG icons, you can simply set the icon size large enough (38px for me) to see the previews anyway. This workaround is good enough, and should not prevent us from improving the handling for all other situations. Let's be honest here: How often do you want to select icons with <38px size, but cannot increase the size temporarily?
simply set the icon size large enough (38px for me)
To add to that: The icon will be shown at its native size, the "icon size" in this case is only about the grid spacing!
Sorry Mark, I can respect your opinion, but I just don't see the practical impact.
First of all, SVG previews for icons didn't even work at all until yesterday, when Alex fixed it in D12389: Filepicker reads thumbs preview from Dolphin settings.
Second of all, if you're browsing icons and you have previews on, without this patch the previews are unusably small:
For people who have this use case, switching to large icons view or increasing the size makes much more sense than attempting to distinguish previews at 16px. Furthermore, browsing SVG icons is such a developer-specific task that I don't think it makes sense to optimize around it. Again, remember that what you're worried about not being possible was already impossible until Alex fixed it yesterday. I think this line of objection is a tempest in a teapot.
I think you mis-interpreted what i said which then caused @ngraham to reply with apparently that in mind which is also not as i intended :)
I intended specifically what i said.
browse a folder with lots of icons like an icon pack/theme. And yes, that is - sometimes - very handy to have!
I said nothing about the view mode (i meant the grid view though, not the list view).
In those cases where you just browse through a gazillion icons (nothing with an icon picker or selecting icons, i didn't say any of that) becomes impossible in your future patch.
This patch makes it slightly more "inconvenient" to browse folders like that.
As for SVG.. Not my words. png, jpg, any format really.
Browse... grid view... small size... previews... icons... so you want to be able to do this?
Sometimes, yes.
Like when looking for which icon to use for another button in the application i'm making for my job for instance....
Are such tiny icons actually useful and distinguishable? Wouldn't it actually improve your productivity to increase the size of the icons and the window, such that you saw this instead?
Why torture yourself with tiny icons in a tiny window?
What "future patch" are you referring to, though?
As far as I've understood, we don't plan to prevent people from viewing thumbnails for tiny icons anywhere (as long as they select a reasonable/medium grid size).
In fact, with D12306: Improve grid icon layout in filepicker dialog there probably won't be much of a difference between slider position with regard to grid spacing anyway, as "this patch improves the grid spacing in icons-on-top mode by making it looser for small icons", to give more room for the filename label.
You said:
This automatic logic is already there. You are simply objecting to not allowing previews for small icons. Yes, we will remove this possibility which was there before.
Which i interpreted as "previews for small images/icons won't be possible anymore in the future".
Did i interpret that wrong?
Not always. Sometimes you want to see how they look at the actual size. How they are represented.
The blown up way of looking at it (resized version) does not give you an accurate representation of how it will look like in the native size. Granted, this is very developer specific and what i usually do is first look at the blown up version. Then single out a few i might like followed by looking at the real sizes to determine which one to use. But regardless of that, it's great to be able to do that, i see no reason why we should lose that ability.
Indeed, we have to be careful with the terminology here:
- "Icon size" mainly refers to the "Icon size" slider, which chooses how much space is available in the dialog per item/cell/file.
- The size of an image, e.g. a photo, an icon or a word document, has nothing to do with this patch.
For example, with 38px grid "icon size", you'll be able to see a 4000x3000 photo as well as a 4x3 icon. With 12px grid "icon size", you won't see previews for either the photo or the tiny icon.
Have you actually tried the patch? Raster icons are not blown up, they are shown at their native size as long as it is smaller than the grid spacing (and the grid spacing is large enough to enable showing of previews).
I've not tried this patch nor the svg patch.
The perfect video of this patch clearly shows me what i need to see (big pros for that btw!).
The svg patch unlikely influences my png icons ;)
My comment was based on what i see in an icon folder right in front of me at this very moment.
16x16 icons are scaled to whatever the zoom level is (which is fine imho). These icons are png ones.
And the 16x16 png icons with previews are actually distinguishable and useful? Can you share a screenshot?
Maybe you should, to get a feel for how nice this works!
My comment was based on what i see in an icon folder right in front of me at this very moment.
16x16 icons are scaled to whatever the zoom level is (which is fine imho). These icons are png ones.
Are you sure? For me small icons do not scale up, both without and with the patch:
Mark, how can we move forward here? We'd like to address your concerns if we can.
The underlying goal here is to turn on previews by default so that the large icons view gets them, but we don't irritate people using very small icon sizes who generally don't benefit from previews since the icons are too small to distinguish anything. Do you see any other ways to achieve this goal? Or are you willing to put up with no previews for your tiny icons, and just increase their sizes a bit? :) Keep in mind that once the grid spacing patch goes in (today, per @rkflx) you'll be able to see more icons at larger sizes than you can today. So once all is said and done, you may actually come out ahead compared to what you have available today.
@anemeth Finally got to the code review part. First of all, thanks for implementing the disabled/turned-off behaviour I suggested, works really great now and surely makes the feature much more pleasant to use.
Some small comments regarding the code (better naming of variables and cleanup, mostly), but otherwise looks pretty good.
src/filewidgets/kdiroperator.cpp | ||
---|---|---|
232 | I'd name this updatePreviewActionState to keep the terminology somewhat consistent (otherwise the code is a bit hard to follow, at least I felt this way when reviewing the patch). | |
286 | calledFromUpdatePreviewActionState | |
287 | showPreviewsConfigEntry | |
1646–1647 | Unrelated? | |
2129 | That's a totally unrelated change (and a change which I would object to). Show Preview is about the preview sidebar you can toggle with F11. | |
2587 | Please drop the is in the variable name, as this would only be used for the corresponding getter function normally. | |
2588 | Unrelated change. | |
2589–2591 | Could you explain why you need to change showPreviewsEnabledBeforeZoom here? As far as I can see this variable just caches the config value, and thus should only be set in _k_toggleInlinePreviews? Do you have an example where something would break if we would remove this code block? | |
2599 | Wording: "for preview" → "for showing previews" (Or just use @ngraham's wording instead of inventing your own, because as a native speaker he often gets it quite right: "Previews are automatically disabled for icons smaller than {threshold]", or simply "Previews are automatically disabled for small icon sizes.".) | |
2611 | Unrelated change. |
@markg I just read the whole thing again. As far as I can see, your main concerns were:
- Not being able to show previews for icon sets of small PNG files.
- Confused users when an option is not available in some situations.
For the first point, we were able to show that small PNG files can be shown as before. For the second point, we brought an example where this is already the case, with no confused users hunting us on Bugzilla. Various other questions also turned out to be non-issues (e.g. HiDPI support, remembering the user-set value etc.).
Furthermore we have explained why your proposed solution to make this configurable is not a sensible path forward.
We can still keep this Diff open for discussion for a couple of days, but at some point we'll have to make some progress. Your comments were really helpful in making us reflect even more use cases and situations, but in the end it turned out the patch should be able to handle all that just fine. Please let us know how to go forward from here.
In any case we plan to also discuss this with Dolphin before landing.
Thanks. Still not 100% there, though ;) (And please rebase on current master, while you are at it…)
src/filewidgets/kdiroperator.cpp | ||
---|---|---|
2164 | Don't remove that, otherwise your reviewer will be totally confused and wonder why your patch is suddenly totally broken! :D (I suspect you already removed what Nate wanted in an earlier revision, but the rest should be kept, of course…) | |
2626 | You still got an unrelated whitespace change here (check on Phabricator with Revision Contents → History → Whitespace Changes → Show All). |
- Rebase on master
- Remember preview state when dialog is closed
- Remove trailing whitespaces left in
Cool, LGTM now. Due to the string change this will have to wait for 5.47, and we have to find a general consensus in Frameworks about the nature of the patch first, of course.
@elvisangelaccio We'd love to hear your opinion on the feature and on the code, and we'd be interested whether the same change would be a good idea for Dolphin ;)
I initially said nothing about PNG. You assumed SVG so i explicitly said PNG, but it's any non-raster format. Don't make this format specific.
- Confused users when an option is not available in some situations.
For the first point, we were able to show that small PNG files can be shown as before. For the second point, we brought an example where this is already the case, with no confused users hunting us on Bugzilla. Various other questions also turned out to be non-issues (e.g. HiDPI support, remembering the user-set value etc.).
And i have to admit that i was testing the wrong thing.
If i take a grid view (in dolphin) and do that on a folder with lots of small icons i see them being resized.
Apparently the behavior in the file open dialog is different in this regard.
So i guess i'm 50% wrong there ;)
Furthermore we have explained why your proposed solution to make this configurable is not a sensible path forward.
I know and i thank you (and the rest) for the constructive discussion!
We can still keep this Diff open for discussion for a couple of days, but at some point we'll have to make some progress. Your comments were really helpful in making us reflect even more use cases and situations, but in the end it turned out the patch should be able to handle all that just fine. Please let us know how to go forward from here.
In any case we plan to also discuss this with Dolphin before landing.
Please let me stress that i'm very much in favor of having this! No mistake about that.
I'm just quite firmly against a "in your face" icon changing state "automagically". If i press "show previews" then i expect them to be shown, however small they are. It's a choice i made and apparently want to have, A 3-state button is imho still the way to go. It could for instance be a button with a diagonal line in it where half of the button is disabled and the other half is enabled. Quite graphically showing that "both" states are active (the auto state, the view knows best).
Anyhow, i don't think we're going to agree on this. That's fine :)
You folks have the vision and you've been doing a great job thus far so i ultimately have no reason to stop you from making progress, even if i don't like it.
I will resign from this review in a few days to let it progress to wherever it wants to go.
I do feel like I get Mark's point and sympathize with the impulse behind it: there's the potential for user frustration when a feature is unexpectedly unavailable, or a button doesn't do anything when you click it. It's like the Fit and Fill buttons in Gwenview when "Enlarge Images" is turned off: you click on them, nothing happens, and you don't know why and feel irritated about it. Now, it's better here since we're actually disabling the button when its feature can't be used, and we show a helpful tooltip on hover that explains why. We might consider doing those for Gwenview too.
If and when this feature lands in both the open/save dialogs and also Dolphin, I don't really expect any user complaints, but if they do arise, we can investigate adding a setting to control the "disable previews at small sizes" with a setting in Dolphin's Previews tab (until we can relocate those settings to somewhere in System Settings). I'm not a fan of tri-state buttons, as they're terribly baffling to most users.
I don't have a strong opinion on this feature. Code is a bit complex, but looks correct. If this gets in, it'd be a good idea to do the same in Dolphin :)
- Merge branch 'master' of https://github.com/KDE/kio into conditional_preview
- change static_cast to qobject_cast
Darn, my bad. Could we revert just the tooltip string change and land that for 5.47, or would you prefer to hold the whole feature until 5.47?
Well, I'll revert and then we can discuss whether or not we can land a more limited version without the tooltip change.
Reverted. Alex, now that you have a fancy contributor account, you can land this yourself on 5/6/18, or else offer up a version of this without the tooltip change and petition to get that in for 5.46.
Well, the tooltip was added for a reason. The reason is still there, so the tooltip still has to be there.
Maybe later on we won't need the tooltip when removing the button, but until that's done (and agreed upon – I did not see any agreement from others, yet you already started on the implementation…), it should stay. Let's not rush this please, it only leads to more work in the end.
Please read the link I gave and understand what a string freeze is. As you have commit access now, that's your job to get right ;)