D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth closed this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a comment.


  5.46 has been tagged; this can land now.

REPOSITORY
  R241 KIO

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#256797 , @anemeth wrote:
  
  > In D12321#256791 , @ngraham 
wrote:
  >
  > > 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 ;)

REPOSITORY
  R241 KIO

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a comment.


  May 6th.

REPOSITORY
  R241 KIO

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth added a comment.


  In D12321#256791 , @ngraham wrote:
  
  > 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?

REPOSITORY
  R241 KIO

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#256786 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R241 KIO

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham reopened this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  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.

REPOSITORY
  R241 KIO

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a comment.


  Well, I'll revert and then we can discuss whether or not we can land a more 
limited version without the tooltip change.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#253903 , @rkflx wrote:
  
  > Cool, LGTM now. Due to the string change this will have to wait for 5.47
  
  
  ...and I even added a warning :/

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  This breaks the string freeze. Please revert.
  
  See https://community.kde.org/Schedules/Frameworks.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham closed this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Elvis Angelaccio]

elvisangelaccio accepted this revision as: elvisangelaccio.

REPOSITORY
  R241 KIO

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, elvisangelaccio
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg resigned from this revision.
markg added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  R241 KIO

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth updated this revision to Diff 1.
anemeth added a comment.


  - Changed tooltip for disabled preview button

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12321?vs=0=1

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in kdiroperator.cpp: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).

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth marked an inline comment as done.
anemeth added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in kdiroperator.cpp: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).

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth updated this revision to Diff 0.
anemeth added a comment.


  - Merge branch 'master' of https://github.com/KDE/kio into conditional_preview
  - change static_cast to qobject_cast

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12321?vs=33101=0

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D12321#253903 , @rkflx wrote:
  
  > @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 :)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> kdiroperator.cpp:2589
> +
> +KToggleAction *previewAction = static_cast *>(actionCollection->action(QStringLiteral("inline preview")));
> +previewAction->setEnabled(iconSizeBigEnoughForPreview);

`qobject_cast`

> kdiroperator.cpp:2595
> +} else {
> +previewAction->setToolTip(i18n("Previews are automatically disabled 
> for small icon sizes"));
> +}

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#253839 , @rkflx wrote:
  
  > @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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a subscriber: elvisangelaccio.
rkflx added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: elvisangelaccio, markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, 
michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth marked 2 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth updated this revision to Diff 33101.
anemeth added a comment.


  - Rebase on master
  - Remember preview state when dialog is closed
  - Remove trailing whitespaces left in

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12321?vs=33098=33101

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  Thanks. Still not 100% there, though ;) (And please rebase on current master, 
while you are at it…)

INLINE COMMENTS

> kdiroperator.cpp:2622
>  } else {
> -const QFontMetrics metrics(itemView->viewport()->font());
> +const QFontMetrics metrics(itemView->viewport()->font());
>  int size = itemView->iconSize().height() + metrics.height() * 2;

You still got an unrelated whitespace change here (check on Phabricator with 
Revision Contents > History > Whitespace Changes > Show All).

> kdiroperator.cpp:2166
>  d->showPreviews = configGroup.readEntry(QStringLiteral("Previews"), 
> false);
> +d->showPreviewsEnabledBeforeZoom = d->showPreviews;
>  }

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth marked 2 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth marked an inline comment as done.
anemeth added inline comments.

INLINE COMMENTS

> rkflx wrote in kdiroperator.cpp: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?

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 

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth marked 8 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth updated this revision to Diff 33098.
anemeth added a comment.


  Implement changes suggested by @rkflx

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12321?vs=32872=33098

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx requested changes to this revision.
rkflx added a comment.


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

INLINE COMMENTS

> kdiroperator.cpp:232
>  void updateListViewGrid();
> +void updateIconPreviewsState();
>  int iconSizeForViewType(QAbstractItemView *itemView) const;

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

> kdiroperator.cpp:286
>  bool showPreviews;
> +bool calledByPreviewStateUpdate;
> +bool showPreviewsEnabledBeforeZoom;

`calledFromUpdatePreviewActionState`

> kdiroperator.cpp:287
> +bool calledByPreviewStateUpdate;
> +bool showPreviewsEnabledBeforeZoom;
> +

`showPreviewsConfigEntry`

> kdiroperator.cpp:1646-1647
>  
> +// set grid and previews
> +d->updateListViewGrid();
> +

Unrelated?

> kdiroperator.cpp:2131
>  //}
> -if (configGroup.readEntry(QStringLiteral("Show Preview"), false)) {
> +if (configGroup.readEntry(QStringLiteral("Show Preview"), true)) {
>  d->defaultView |= KFile::PreviewContents;

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

> kdiroperator.cpp:2585
>  } else {
> -const QFontMetrics metrics(itemView->viewport()->font());
>  int size = itemView->iconSize().height() + metrics.height() * 2;

Unrelated change.

> kdiroperator.cpp:2587
> +// hide icon previews when they are too small
> +const bool isIconSizeBigEnoughForPreview = itemView->iconSize().height() 
> > metrics.height() * 2;
> +

Please drop the `is` in the variable name, as this would only be used for the 
corresponding getter function normally.

> kdiroperator.cpp:2589-2591
> +if (showPreviews && isIconSizeBigEnoughForPreview) {
> +showPreviewsEnabledBeforeZoom = true;
> +}

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?

> kdiroperator.cpp:2599
> +} else {
> +previewAction->setToolTip(i18n("Icon size is too small for 
> preview"));
> +}

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

> kdiroperator.cpp:2615
> +
> +const QFontMetrics metrics(itemView->viewport()->font());
> +

Unrelated change.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#252421 , @markg wrote:
  
  > 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:
  
  F5819647: KIO-16px-icons-before-after.webm 


REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a comment.


  In D12321#252421 , @markg wrote:
  
  > 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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252393 , @rkflx wrote:
  
  > In D12321#252374 , @markg wrote:
  >
  > > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#252374 , @markg wrote:
  
  > 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, the 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).

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#252372 , @markg wrote:
  
  > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252366 , @ngraham wrote:
  
  > In D12321#252365 , @markg wrote:
  >
  > > > Browse... grid view... small size... previews... icons... so you want 
to be able to do this?
  > > > 
  > > > F5819296: Small, not useful.png 
  > >
  > > 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?
  >
  > F5819375: Large, more useful.png 
  >
  > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252369 , @rkflx wrote:
  
  > In D12321#252337 , @markg wrote:
  >
  > > 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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#252337 , @markg wrote:
  
  > 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: Filepicker dialog proper grid icon layout 
 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a comment.


  In D12321#252365 , @markg wrote:
  
  > > Browse... grid view... small size... previews... icons... so you want to 
be able to do this?
  > > 
  > > F5819296: Small, not useful.png 
  >
  > 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?
  
  F5819375: Large, more useful.png 
  
  Why torture yourself with tiny icons in a tiny window?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252346 , @ngraham wrote:
  
  > In D12321#252337 , @markg wrote:
  >
  > > In D12321#252299 , @rkflx 
wrote:
  > >
  > > > > 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?
  >
  > F5819296: Small, not useful.png 
  
  
  Sometimes, yes.
  Like when looking for which icon to use for another button in the application 
i'm making for my job for instance

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a comment.


  In D12321#252337 , @markg wrote:
  
  > In D12321#252299 , @rkflx wrote:
  >
  > > > 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?
  
  F5819296: Small, not useful.png 

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252299 , @rkflx wrote:
  
  > > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a comment.


  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:
  
  F5819200: Not very useful.png 
  
  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. I think this line of objection is a tempest in a teapot.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth added a comment.


  In D12321#252299 , @rkflx wrote:
  
  > 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:
  
  F5819197: svg_prev.PNG 

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  > 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 (and make the icon large enough to be 
visible in the first place, I might add). 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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252285 , @rkflx wrote:
  
  > In D12321#252283 , @markg wrote:
  >
  > > In D12321#252282 , @rkflx 
wrote:
  > >
  > > > In D12321#252281 , @markg 
wrote:
  > > >
  > > > > 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)?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#252283 , @markg wrote:
  
  > In D12321#252282 , @rkflx wrote:
  >
  > > In D12321#252281 , @markg 
wrote:
  > >
  > > > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252282 , @rkflx wrote:
  
  > In D12321#252281 , @markg wrote:
  >
  > > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#252281 , @markg wrote:
  
  > 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 confusion for users.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252264 , @rkflx wrote:
  
  > In D12321#252261 , @markg wrote:
  >
  > > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  @markg One more thing: This patch is a prerequisite for D12321: Hide file 
preview when icon is too small <https://phabricator.kde.org/D12321>, 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).
  
  D12321 <https://phabricator.kde.org/D12321> again is required for the vision 
outlined T8552 <https://phabricator.kde.org/T8552>, 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#252261 , @markg wrote:
  
  > 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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg added a comment.


  In D12321#252259 , @anemeth wrote:
  
  > In D12321#252238 , @markg wrote:
  >
  > > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#252258 , @anemeth wrote:
  
  > In D12321#252245 , @rkflx wrote:
  >
  > > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth added a comment.


  In D12321#252238 , @markg wrote:
  
  > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth added a comment.


  In D12321#252245 , @rkflx wrote:
  
  > 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.
  
  F5819083: 2018-04-23 13-15-44.webm 

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#252238 , @markg wrote:
  
  > 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 user 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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Mark Gaiser]

markg requested changes to this revision.
markg added a comment.
This revision now requires changes to proceed.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin, markg
Cc: markg, xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth updated this revision to Diff 32872.
anemeth added a comment.


  Change tooltip when preview button is disabled/enabled.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12321?vs=32602=32872

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#250031 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a comment.


  In D12321#24 , @xyquadrat 
wrote:
  
  > In D12321#249981 , @ngraham 
wrote:
  >
  > > 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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth updated this revision to Diff 32602.
anemeth added a comment.


  Remove left in debug

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12321?vs=32601=32602

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth updated this revision to Diff 32601.
anemeth added a comment.


  - Hide previews when icons are too small and disable the preview pushbutTO
  - Remember preview state after closing window

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12321?vs=32596=32601

BRANCH
  conditional_preview (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12321

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Julian Schraner]

xyquadrat added a comment.


  In D12321#249981 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: xyquadrat, sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a comment.


  Fabulous. I agree with @rkflx that before committing this, we should also 
prepare a similar patch for Dolphin to unify the behaviors.
  
  One quality-of-life enhancement I could see would be to change the tooltip 
for the preview button to say "Previews are automatically disabled for icons 
smaller than {threshold]" when it's disabled. Not a need-to-have, but a 
nice-to-have.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth marked 2 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth updated this revision to Diff 32596.
anemeth added a comment.


  Remove unintentional changes from D12328 

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12321?vs=32546=32596

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12321

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a comment.


  Very nice! I agree with @sharvey: very classy.

INLINE COMMENTS

> kdiroperator.cpp:2165
>  if (d->inlinePreviewState == Private::NotForced) {
> -d->showPreviews = configGroup.readEntry(QStringLiteral("Previews"), 
> false);
> +d->showPreviews = configGroup.readEntry(QStringLiteral("Preview 
> Thumbnails"), true);
> +d->showPreviewsEnabledBeforeZoom = d->showPreviews;

Unintentional changes from D12328 

> kdiroperator.cpp:2230
>  if (d->inlinePreviewState == Private::NotForced) {
> -configGroup.writeEntry(QStringLiteral("Previews"), d->showPreviews);
> +configGroup.writeEntry(QStringLiteral("Preview Thumbnails"), 
> d->showPreviews);
>  if (qobject_cast(d->itemView)) {

Unintentional changes from D12328 

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  @anemeth Thanks for the update, I'll test it out later.
  
  For now, could you see if some unrelated commit slipped into your patch? I 
see a bunch of "Preview Thumbnails", which belong to another patch really. 
Please revert the unrelated changes, and rebase D12328 
 on D12321 
 instead. See 
https://community.kde.org/Infrastructure/Phabricator#Marking_patches_as_dependent_on_other_patches
 for how your branch setup should look like (i.e. a separate branch per Diff, 
set parent branch as upstream tracking branch, e.g. retroactively with `git 
branch --set-upstream-to=`).

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Scott Harvey]

sharvey added a comment.


  In D12321#249568 , @anemeth wrote:
  
  > (effective screencast)
  
  
  +1 from me, and you beat me to it.   I was just looking through the code 
when Thunderbird chimed that I had new mail.
  
  For the sake of clarification, the reason this works and my attempt in D12094 
 did not, is because this `KToggleAction` 
is, for lack of a better term, unique. There's no second, competing Preview 
action to confuse the issue and be duplicated in Toolbar Preferences. You 
**can** bend a `KToggleAction` to your will; that's pretty straightforward. As 
you can see in @anemeth's patch, toggling it on or off (or disabling it 
entirely) is just a matter of setting the right flags, under the desired 
conditions. My work in D12094  does much of 
the same, but because we were trying to implement two closely-related Find 
actions, we needed two action entries. Here, we only need one. And because 
there's just one to worry about, Alex can make it do any trick he wants, 
without worrying about interfering with or getting confused with another action 
item.
  
  I think this effect is subtle but very classy and polished. We show previews 
at higher sizes by default, but they can be toggled on or off at any time. The 
Preview button is disabled at tiny sizes, because it makes no sense otherwise. 
Looks good.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth added a comment.


  F5812773: 2018-04-19 13-00-42.webm 

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth updated this revision to Diff 32546.
anemeth added a comment.


  Disable preview button when icons are too small.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12321?vs=32500=32546

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12321

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx added a comment.


  In D12321#249441 , @ngraham wrote:
  
  > In D12321#249367 , @rkflx wrote:
  >
  > > The relation between the Preview and the Zoom slider is now a bit weird: 
You can override with the button, but once you move the slider the setting will 
be gone again. Also, when showing the dialog after closing it, the slider has 
preference over the button. In addition, even though the button says "enabled", 
users will wonder why no thumbnails are showing!
  > >
  > > Would it make sense to both:
  > >
  > > - Set the button state to "off" once the icons are too small, to make it 
clear that no previews are to be expected.
  > > - Disable the button, so manually toggling thumbnails only works if icons 
are big enough.
  >
  >
  > Something like this might be nice, but as you observed, it'll get real 
complicated real fast. And as @sharvey discovered in D12094 
, trying to coerce a KToggleAction into 
doing something it doesn't want to do can be daunting.
  
  
  As far as I can see, those are two totally separate issues. Here we should be 
able to disable the complete action (second point), and toggle the thumbnails 
via the action instead of sidestepping it (first point).
  
  > My vote gots to disabling the button at small sizes, if that's easily 
possible.
  
  There is nothing to vote on. Both points should be implemented.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a reviewer: Dolphin.
ngraham added a subscriber: sharvey.
ngraham added a comment.


  In D12321#249367 , @rkflx wrote:
  
  > The relation between the Preview and the Zoom slider is now a bit weird: 
You can override with the button, but once you move the slider the setting will 
be gone again. Also, when showing the dialog after closing it, the slider has 
preference over the button. In addition, even though the button says "enabled", 
users will wonder why no thumbnails are showing!
  >
  > Would it make sense to both:
  >
  > - Set the button state to "off" once the icons are too small, to make it 
clear that no previews are to be expected.
  > - Disable the button, so manually toggling thumbnails only works if icons 
are big enough.
  
  
  Something like this might be nice, but as you observed, it'll get real 
complicated real fast. And as @sharvey discovered in D12094 
, trying to coerce a KToggleAction into 
doing something it doesn't want to do can be daunting. My vote gots to 
disabling the button at small sizes, if that's easily possible.
  
  In D12321#249367 , @rkflx wrote:
  
  > Also, before this goes in I'd like to see a similar patch for Dolphin. Not 
so much because of any implemention issue, but to get the (not quite 
uncontroversial) idea more exposure and to agree on a common vision for how 
thumbnails should behave everywhere.
  
  
  Agreed. Let's also invite #dolphin  
into this discussion too. It's a shame the same view widget isn't used in 
both...

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx, #dolphin
Cc: sharvey, rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Henrik Fehlauer]

rkflx requested changes to this revision.
rkflx added a comment.
This revision now requires changes to proceed.


  The relation between the Preview and the Zoom slider is now a bit weird: You 
can override with the button, but once you move the slider the setting will be 
gone again. Also, when showing the dialog after closing it, the slider has 
preference over the button. In addition, even though the button says "enabled", 
users will wonder why no thumbnails are showing!
  
  Would it make sense to both:
  
  - Set the button state to "off" once the icons are too small, to make it 
clear that no previews are to be expected.
  - Disable the button, so manually toggling thumbnails only works if icons are 
big enough.
  
  Allowing to override even for small images would quickly lead to some kind of 
manual vs. automatic setting, but this would be even more confusing, so I'd 
rather not go in that direction.
  
  ---
  
  Also, before this goes in I'd like to see a similar patch for Dolphin. Not so 
much because of any implemention issue, but to get the (not quite 
uncontroversial) idea more exposure and to agree on a common vision for how 
thumbnails should behave everywhere.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx
Cc: rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth added a dependent revision: D12328: Enable preview by default in the 
filepicker dialog.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx
Cc: rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a comment.


  (BTW the visible flickering when you resize the icons using the slider is a 
pre-existing issue. We should fix that though. Something else to add to the 
workboard...)

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx
Cc: rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham added a task: T8552: Polish Open/Save dialogs.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx
Cc: rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a reviewer: rkflx.
ngraham added a subscriber: rkflx.
ngraham added a comment.
This revision is now accepted and ready to land.


  Very nice, works great for me. Once this goes in, I think we can safely turn 
on previews by default, because we won't regress anything for people who prefer 
small icons who don't need (and probably don't like) previews. Adding @rkflx as 
a reviewer since I know he feels strongly about previews. :)
  
  In fact, we should really do this for Dolphin too, as currently previews are 
on by default but uselessly apply to tiny icons just like in the file dialogs 
before this patch. Wanna have a crack at that?

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks, ngraham, rkflx
Cc: rkflx, ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth updated this revision to Diff 32500.
anemeth added a comment.


  Apply to all type of views

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12321?vs=32495=32500

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12321

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp

To: anemeth, #vdg, #frameworks
Cc: ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth edited the summary of this revision.
anemeth edited the test plan for this revision.
anemeth added reviewers: VDG, Frameworks.
anemeth added a subscriber: ngraham.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12321

To: anemeth, #vdg, #frameworks
Cc: ngraham, #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth updated this revision to Diff 32495.
anemeth added a comment.


  Remove accidental white space

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12321?vs=32494=32495

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12321

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp

To: anemeth
Cc: #frameworks, michaelh, bruns

D12321: Hide file preview when icon is too small

[Alex Nemeth]

anemeth created this revision.
anemeth added a project: Frameworks.
anemeth requested review of this revision.

REVISION SUMMARY
  file preview

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12321

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp

To: anemeth
Cc: #frameworks, michaelh, bruns