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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180425/fa6c308e/attachment.html>