D12321: Hide file preview when icon is too small

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

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,

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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,

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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,

D12321: Hide file preview when icon is too small

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,

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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:

D12321: Hide file preview when icon is too small

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

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,

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

elvisangelaccio added inline comments. INLINE COMMENTS > kdiroperator.cpp:2589 > + > +KToggleAction *previewAction = static_cast *>(actionCollection->action(QStringLiteral("inline preview"))); > +previewAction->setEnabled(iconSizeBigEnoughForPreview); `qobject_cast` >

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

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

D12321: Hide file preview when icon is too small

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

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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.

D12321: Hide file preview when icon is too small

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.

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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 >

D12321: Hide file preview when icon is too small

markg added a comment. In D12321#252346 , @ngraham wrote: > In D12321#252337 , @markg wrote: > > > In D12321#252299 , @rkflx wrote: > > > > > > Then

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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:

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

markg added a comment. In D12321#252285 , @rkflx wrote: > In D12321#252283 , @markg wrote: > > > In D12321#252282 , @rkflx wrote: > > > > > In

D12321: Hide file preview when icon is too small

rkflx added a comment. In D12321#252283 , @markg wrote: > In D12321#252282 , @rkflx wrote: > > > In D12321#252281 , @markg wrote: > > > > > I agree but

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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 (ev

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

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

D12321: Hide file preview when icon is too small

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 =

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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

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:

D12321: Hide file preview when icon is too small

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

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

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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,

D12321: Hide file preview when icon is too small

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

D12321: Hide file preview when icon is too small

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: