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
