Hide file preview when icon is too small
ClosedPublic

Authored by anemeth on Apr 18 2018, 5:55 PM.

Details

Summary

In the filepicker dialog hide the previews when the icons are too small.

Test Plan

Diff Detail

Repository
R241 KIO
Branch
conditional_preview (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes

Fabulous. I agree with @rkflx that before committing this, we should also prepare a similar patch for Dolphin to unify the behaviors.

That would be great, as this would solve bug 350932 & keep the behavior consistent across applications.

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

rkflx added a comment.Apr 19 2018, 8:08 PM

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.

anemeth updated this revision to Diff 32872.EditedMon, Apr 23, 10:20 AM

Change tooltip when preview button is disabled/enabled.

markg requested changes to this revision.Mon, Apr 23, 11:03 AM
markg added a subscriber: markg.

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.

This revision now requires changes to proceed.Mon, Apr 23, 11:03 AM
rkflx added a comment.EditedMon, Apr 23, 11:12 AM

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.

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?

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 already fixed that 2 diffs ago.

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.

That's why the tooltip changes when that is the case.

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 already fixed that 2 diffs ago.

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.

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.

That's why the tooltip changes when that is the case.

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.

That is unexpected behavior.

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?

rkflx added a comment.EditedMon, Apr 23, 11:51 AM

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

That is unexpected behavior.

I disagree, adapting the interface dynamically is good design.

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.

rkflx added a comment.EditedMon, Apr 23, 12:16 PM

I agree but not for a button that the user controls.

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.

I agree but not for a button that the user controls.

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.

rkflx added a comment.EditedMon, Apr 23, 12:31 PM

I agree but not for a button that the user controls.

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.

I agree but not for a button that the user controls.

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.

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)?

rkflx added a comment.EditedMon, Apr 23, 12:57 PM

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?

rkflx added a comment.Mon, Apr 23, 1:00 PM

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!

Also, as you may have noticed, the file dialog does not show previews of SVGs for any size, making your point moot.

Actually with D12389 applied it will:

ngraham added a comment.EditedMon, Apr 23, 1:06 PM

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.

markg added a comment.Mon, Apr 23, 2:03 PM

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?

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.

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?

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

Browse... grid view... small size... previews... icons... so you want to be able to do this?

markg added a comment.Mon, Apr 23, 2:40 PM

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?

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

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

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?

rkflx added a comment.Mon, Apr 23, 3:31 PM

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.

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.

markg added a comment.Mon, Apr 23, 3:36 PM

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.

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?

markg added a comment.Mon, Apr 23, 3:38 PM

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?

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.

rkflx added a comment.Mon, Apr 23, 3:40 PM

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?

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.

rkflx added a comment.EditedMon, Apr 23, 3:43 PM

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.

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

markg added a comment.Mon, Apr 23, 5:04 PM

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.

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.

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?

rkflx added a comment.Mon, Apr 23, 8:12 PM

I've not tried this patch nor the svg patch.

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.

rkflx requested changes to this revision.Wed, Apr 25, 6:50 PM

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

rkflx added a comment.Wed, Apr 25, 6:51 PM

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

anemeth updated this revision to Diff 33098.Wed, Apr 25, 7:24 PM

Implement changes suggested by @rkflx

anemeth marked 8 inline comments as done.Wed, Apr 25, 7:25 PM
anemeth marked an inline comment as done.Wed, Apr 25, 7:28 PM
anemeth added inline comments.
src/filewidgets/kdiroperator.cpp
2589–2591

Without this the previews are always disabled when opening the dialog. This was added to enable it and then disable it right after if the read config was disabled in the k_toggleInlinePreviews function.
This will be fixed with D12328

anemeth marked 2 inline comments as done.Wed, Apr 25, 7:30 PM
rkflx added a comment.Wed, Apr 25, 7:59 PM

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 ContentsHistoryWhitespace ChangesShow All).

anemeth updated this revision to Diff 33101.Wed, Apr 25, 8:29 PM
  • Rebase on master
  • Remember preview state when dialog is closed
  • Remove trailing whitespaces left in
anemeth marked 2 inline comments as done.Wed, Apr 25, 8:31 PM
rkflx accepted this revision.Wed, Apr 25, 8:39 PM
rkflx added a subscriber: elvisangelaccio.

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 ;)

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

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.

src/filewidgets/kdiroperator.cpp
2589

qobject_cast

2595

Maybe we can also add an hint about how to actually show the previews? (i.e. the fact that the user needs to increase the icon size).

@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 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 :)

anemeth updated this revision to Diff 33330.Mon, Apr 30, 1:09 PM
anemeth marked an inline comment as done.Mon, Apr 30, 1:11 PM
anemeth added inline comments.
src/filewidgets/kdiroperator.cpp
2595

Do you have a suggestion?
I think it is self explanatory.
@ngraham what do you think?

ngraham added inline comments.Mon, Apr 30, 1:12 PM
src/filewidgets/kdiroperator.cpp
2595

@anemeth I'm inclined to agree, though there's always room for improvement, and more clarity is usually better. How about this for the tooltip text?

"Automatically disabled for small icons; increase icon size to see previews"

anemeth updated this revision to Diff 33331.Mon, Apr 30, 1:21 PM
  • Changed tooltip for disabled preview button
markg resigned from this revision.Mon, Apr 30, 3:08 PM

I guess it's time for me to resign from this one.
Good luck folks :)

This revision is now accepted and ready to land.Mon, Apr 30, 3:08 PM
ngraham closed this revision.Tue, May 1, 1:36 AM
rkflx added a comment.Tue, May 1, 8:57 PM

This breaks the string freeze. Please revert.

See https://community.kde.org/Schedules/Frameworks.

rkflx added a comment.Tue, May 1, 9:00 PM

Cool, LGTM now. Due to the string change this will have to wait for 5.47

...and I even added a warning :/

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.

ngraham reopened this revision.Tue, May 1, 9:13 PM

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.

This revision is now accepted and ready to land.Tue, May 1, 9:13 PM
rkflx added a comment.Tue, May 1, 9:15 PM

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

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.

I'll rather wait.
5/6 as in May 6th or June 5th?

rkflx added a comment.Tue, May 1, 9:18 PM

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.

I'll rather wait.
5/6 as in May 6th or June 5th?

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 ;)

5.46 has been tagged; this can land now.

anemeth closed this revision.Sun, May 6, 4:11 PM