Differential D12321 Diff 32872 src/filewidgets/kdiroperator.cpp

Changeset View

Standalone View

View Options

src/filewidgets/kdiroperator.cpp

Show First 20 Lines • Show All 223 Lines • ▼ Show 20 Line(s)172public:
224 void _k_assureVisibleSelection(); 224​ void _k_assureVisibleSelection();
225 void _k_synchronizeSortingState(int, Qt::SortOrder); 225​ void _k_synchronizeSortingState(int, Qt::SortOrder);
226 void _k_slotChangeDecorationPosition(); 226​ void _k_slotChangeDecorationPosition();
227 void _k_slotExpandToUrl(const QModelIndex &); 227​ void _k_slotExpandToUrl(const QModelIndex &);
228 void _k_slotItemsChanged(); 228​ void _k_slotItemsChanged();
229 void _k_slotDirectoryCreated(const QUrl &); 229​ void _k_slotDirectoryCreated(const QUrl &);
230 230​
231 void updateListViewGrid(); 231​ void updateListViewGrid();
232​ void updateIconPreviewsState();
rkflxUnsubmitted
Done

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

rkflx: I'd name this `updatePreviewActionState` to keep the terminology somewhat consistent (otherwise…
232 int iconSizeForViewType(QAbstractItemView *itemView) const; 233​ int iconSizeForViewType(QAbstractItemView *itemView) const;
233 234​
234 // private members 235​ // private members
235 KDirOperator *parent; 236​ KDirOperator *parent;
236 QStack<QUrl *> backStack; ///< Contains all URLs you can reach with the back button. 237​ QStack<QUrl *> backStack; ///< Contains all URLs you can reach with the back button.
237 QStack<QUrl *> forwardStack; ///< Contains all URLs you can reach with the forward button. 238​ QStack<QUrl *> forwardStack; ///< Contains all URLs you can reach with the forward button.
238 239​
239 QModelIndex lastHoveredIndex; 240​ QModelIndex lastHoveredIndex;
Show All 37 Lines
277 278​
278 KNewFileMenu *newFileMenu; 279​ KNewFileMenu *newFileMenu;
279 280​
280 KConfigGroup *configGroup; 281​ KConfigGroup *configGroup;
281 282​
282 KFilePreviewGenerator *previewGenerator; 283​ KFilePreviewGenerator *previewGenerator;
283 284​
284 bool showPreviews; 285​ bool showPreviews;
286​ bool calledByPreviewStateUpdate;
rkflxUnsubmitted
Done

calledFromUpdatePreviewActionState

rkflx: `calledFromUpdatePreviewActionState`
287​ bool showPreviewsEnabledBeforeZoom;
rkflxUnsubmitted
Done

showPreviewsConfigEntry

rkflx: `showPreviewsConfigEntry`
288​
285 int iconsZoom; 289​ int iconsZoom;
286 290​
287 bool isSaving; 291​ bool isSaving;
288 292​
289 KActionMenu *decorationMenu; 293​ KActionMenu *decorationMenu;
290 KToggleAction *leftAction; 294​ KToggleAction *leftAction;
291 QList<QUrl> itemsToBeSetAsCurrent; 295​ QList<QUrl> itemsToBeSetAsCurrent;
292 bool shouldFetchForItems; 296​ bool shouldFetchForItems;
Show All 18 Lines301KDirOperator::Private::Private(KDirOperator *_parent) :
311 progressDelayTimer(nullptr), 315​ progressDelayTimer(nullptr),
312 dropOptions(0), 316​ dropOptions(0),
313 actionMenu(nullptr), 317​ actionMenu(nullptr),
314 actionCollection(nullptr), 318​ actionCollection(nullptr),
315 newFileMenu(nullptr), 319​ newFileMenu(nullptr),
316 configGroup(nullptr), 320​ configGroup(nullptr),
317 previewGenerator(nullptr), 321​ previewGenerator(nullptr),
318 showPreviews(false), 322​ showPreviews(false),
323​ calledByPreviewStateUpdate(false),
324​ showPreviewsEnabledBeforeZoom(false),
319 iconsZoom(0), 325​ iconsZoom(0),
320 isSaving(false), 326​ isSaving(false),
321 decorationMenu(nullptr), 327​ decorationMenu(nullptr),
322 leftAction(nullptr), 328​ leftAction(nullptr),
323 shouldFetchForItems(false), 329​ shouldFetchForItems(false),
324 inlinePreviewState(NotForced) 330​ inlinePreviewState(NotForced)
325{ 331​{
326} 332​}
▲ Show 20 Lines • Show All 322 Lines • ▼ Show 20 Line(s)
649void KDirOperator::Private::_k_toggleInlinePreviews(bool show) 655​void KDirOperator::Private::_k_toggleInlinePreviews(bool show)
650{ 656​{
651 if (showPreviews == show) { 657​ if (showPreviews == show) {
652 return; 658​ return;
653 } 659​ }
654 660​
655 showPreviews = show; 661​ showPreviews = show;
656 662​
663​ if (!calledByPreviewStateUpdate) {
664​ showPreviewsEnabledBeforeZoom = show;
665​ }
666​
657 if (!previewGenerator) { 667​ if (!previewGenerator) {
658 return; 668​ return;
659 } 669​ }
660 670​
661 previewGenerator->setPreviewShown(show); 671​ previewGenerator->setPreviewShown(show);
662} 672​}
663 673​
664void KDirOperator::Private::_k_slotOpenFileManager() 674​void KDirOperator::Private::_k_slotOpenFileManager()
▲ Show 20 Lines • Show All 963 Lines • ▼ Show 20 Line(s)1548{
1628 const bool previewShown = d->inlinePreviewState == Private::NotForced ? d->showPreviews : previewForcedToTrue; 1638​ const bool previewShown = d->inlinePreviewState == Private::NotForced ? d->showPreviews : previewForcedToTrue;
1629 d->previewGenerator = new KFilePreviewGenerator(d->itemView); 1639​ d->previewGenerator = new KFilePreviewGenerator(d->itemView);
1630 const int maxSize = KIconLoader::SizeEnormous - KIconLoader::SizeSmall; 1640​ const int maxSize = KIconLoader::SizeEnormous - KIconLoader::SizeSmall;
1631 const int val = (maxSize * d->iconsZoom / 100) + KIconLoader::SizeSmall; 1641​ const int val = (maxSize * d->iconsZoom / 100) + KIconLoader::SizeSmall;
1632 d->itemView->setIconSize(previewForcedToTrue ? QSize(KIconLoader::SizeHuge, KIconLoader::SizeHuge) : QSize(val, val)); 1642​ d->itemView->setIconSize(previewForcedToTrue ? QSize(KIconLoader::SizeHuge, KIconLoader::SizeHuge) : QSize(val, val));
1633 d->previewGenerator->setPreviewShown(previewShown); 1643​ d->previewGenerator->setPreviewShown(previewShown);
1634 d->actionCollection->action(QStringLiteral("inline preview"))->setChecked(previewShown); 1644​ d->actionCollection->action(QStringLiteral("inline preview"))->setChecked(previewShown);
1635 1645​
1646​ // set grid and previews
1647​ d->updateListViewGrid();
rkflxUnsubmitted
Done

Unrelated?

rkflx: Unrelated?
1648​
1636 // ensure we change everything needed 1649​ // ensure we change everything needed
1637 d->_k_slotChangeDecorationPosition(); 1650​ d->_k_slotChangeDecorationPosition();
1638 1651​
1639 emit viewChanged(view); 1652​ emit viewChanged(view);
1640 1653​
1641 const int zoom = previewForcedToTrue ? (KIconLoader::SizeHuge - KIconLoader::SizeSmall + 1) * 100 / maxSize : d->iconSizeForViewType(view); 1654​ const int zoom = previewForcedToTrue ? (KIconLoader::SizeHuge - KIconLoader::SizeSmall + 1) * 100 / maxSize : d->iconSizeForViewType(view);
1642 1655​
1643 // this will make d->iconsZoom be updated, since setIconsZoom slot will be called 1656​ // this will make d->iconsZoom be updated, since setIconsZoom slot will be called
▲ Show 20 Lines • Show All 466 Lines • ▼ Show 20 Line(s)2122 } else if (viewStyle == QLatin1String("DetailTree")) {
2110 d->defaultView |= KFile::DetailTree; 2123​ d->defaultView |= KFile::DetailTree;
2111 } else { 2124​ } else {
2112 d->defaultView |= KFile::Simple; 2125​ d->defaultView |= KFile::Simple;
2113 } 2126​ }
2114 //if (configGroup.readEntry(QLatin1String("Separate Directories"), 2127​ //if (configGroup.readEntry(QLatin1String("Separate Directories"),
2115 // DefaultMixDirsAndFiles)) { 2128​ // DefaultMixDirsAndFiles)) {
2116 // d->defaultView |= KFile::SeparateDirs; 2129​ // d->defaultView |= KFile::SeparateDirs;
2117 //} 2130​ //}
2118 if (configGroup.readEntry(QStringLiteral("Show Preview"), false)) { 2131​ if (configGroup.readEntry(QStringLiteral("Show Preview"), true)) {
rkflxUnsubmitted
Done

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.

rkflx: That's a totally unrelated change (and a change which I would object to). `Show Preview` is…
2119 d->defaultView |= KFile::PreviewContents; 2132​ d->defaultView |= KFile::PreviewContents;
2120 } 2133​ }
2121 2134​
2122 d->previewWidth = configGroup.readEntry(QStringLiteral("Preview Width"), 100); 2135​ d->previewWidth = configGroup.readEntry(QStringLiteral("Preview Width"), 100);
2123 2136​
2124 if (configGroup.readEntry(QStringLiteral("Show hidden files"), 2137​ if (configGroup.readEntry(QStringLiteral("Show hidden files"),
2125 DefaultShowHidden)) { 2138​ DefaultShowHidden)) {
2126 d->actionCollection->action(QStringLiteral("show hidden"))->setChecked(true); 2139​ d->actionCollection->action(QStringLiteral("show hidden"))->setChecked(true);
Show All 17 Lines2156 } else if (sortBy == QLatin1String("Type")) {
2144 sorting |= QDir::Type; 2157​ sorting |= QDir::Type;
2145 } 2158​ }
2146 if (configGroup.readEntry(QStringLiteral("Sort reversed"), DefaultSortReversed)) { 2159​ if (configGroup.readEntry(QStringLiteral("Sort reversed"), DefaultSortReversed)) {
2147 sorting |= QDir::Reversed; 2160​ sorting |= QDir::Reversed;
2148 } 2161​ }
2149 d->updateSorting(sorting); 2162​ d->updateSorting(sorting);
2150 2163​
2151 if (d->inlinePreviewState == Private::NotForced) { 2164​ if (d->inlinePreviewState == Private::NotForced) {
2152 d->showPreviews = configGroup.readEntry(QStringLiteral("Previews"), false); 2165​ d->showPreviews = configGroup.readEntry(QStringLiteral("Previews"), false);
2166​ d->showPreviewsEnabledBeforeZoom = d->showPreviews;
ngrahamUnsubmitted
Done

Unintentional changes from D12328

ngraham: Unintentional changes from D12328
rkflxUnsubmitted
Done

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

rkflx: Don't remove that, otherwise your reviewer will be totally confused and wonder why your patch…
2153 } 2167​ }
2154 QStyleOptionViewItem::Position pos = (QStyleOptionViewItem::Position) configGroup.readEntry(QStringLiteral("Decoration position"), (int) QStyleOptionViewItem::Left); 2168​ QStyleOptionViewItem::Position pos = (QStyleOptionViewItem::Position) configGroup.readEntry(QStringLiteral("Decoration position"), (int) QStyleOptionViewItem::Left);
2155 setDecorationPosition(pos); 2169​ setDecorationPosition(pos);
2156} 2170​}
2157 2171​
2158void KDirOperator::writeConfig(KConfigGroup &configGroup) 2172​void KDirOperator::writeConfig(KConfigGroup &configGroup)
2159{ 2173​{
2160 QString sortBy = QStringLiteral("Name"); 2174​ QString sortBy = QStringLiteral("Name");
▲ Show 20 Lines • Show All 47 Lines • ▼ Show 20 Line(s)
2208 } else if (KFile::isTreeView(fv)) { 2222​ } else if (KFile::isTreeView(fv)) {
2209 style = QStringLiteral("Tree"); 2223​ style = QStringLiteral("Tree");
2210 } else if (KFile::isDetailTreeView(fv)) { 2224​ } else if (KFile::isDetailTreeView(fv)) {
2211 style = QStringLiteral("DetailTree"); 2225​ style = QStringLiteral("DetailTree");
2212 } 2226​ }
2213 configGroup.writeEntry(QStringLiteral("View Style"), style); 2227​ configGroup.writeEntry(QStringLiteral("View Style"), style);
2214 2228​
2215 if (d->inlinePreviewState == Private::NotForced) { 2229​ if (d->inlinePreviewState == Private::NotForced) {
2216 configGroup.writeEntry(QStringLiteral("Previews"), d->showPreviews); 2230​ configGroup.writeEntry(QStringLiteral("Previews"), d->showPreviewsEnabledBeforeZoom);
ngrahamUnsubmitted
Done

Unintentional changes from D12328

ngraham: Unintentional changes from D12328
2217 if (qobject_cast<QListView *>(d->itemView)) { 2231​ if (qobject_cast<QListView *>(d->itemView)) {
2218 configGroup.writeEntry(QStringLiteral("listViewIconSize"), d->iconsZoom); 2232​ configGroup.writeEntry(QStringLiteral("listViewIconSize"), d->iconsZoom);
2219 } else { 2233​ } else {
2220 configGroup.writeEntry(QStringLiteral("detailedViewIconSize"), d->iconsZoom); 2234​ configGroup.writeEntry(QStringLiteral("detailedViewIconSize"), d->iconsZoom);
2221 } 2235​ }
2222 } 2236​ }
2223 2237​
2224 configGroup.writeEntry(QStringLiteral("Decoration position"), (int) d->decorationPosition); 2238​ configGroup.writeEntry(QStringLiteral("Decoration position"), (int) d->decorationPosition);
▲ Show 20 Lines • Show All 331 Lines • ▼ Show 20 Line(s)2532{
2556 } 2570​ }
2557} 2571​}
2558 2572​
2559void KDirOperator::Private::_k_slotItemsChanged() 2573​void KDirOperator::Private::_k_slotItemsChanged()
2560{ 2574​{
2561 completeListDirty = true; 2575​ completeListDirty = true;
2562} 2576​}
2563 2577​
2578​void KDirOperator::Private::updateIconPreviewsState()
2579​{
2580​ if (!itemView) {
2581​ return;
2582​ }
2583​
2584​ const QFontMetrics metrics(itemView->viewport()->font());
2585​
2586​ // hide icon previews when they are too small
2587​ const bool isIconSizeBigEnoughForPreview = itemView->iconSize().height() > metrics.height() * 2;
rkflxUnsubmitted
Done

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

rkflx: Please drop the `is` in the variable name, as this would only be used for the corresponding…
2588​
2589​ if (showPreviews && isIconSizeBigEnoughForPreview) {
elvisangelaccioUnsubmitted
Done

qobject_cast

elvisangelaccio: `qobject_cast`
2590​ showPreviewsEnabledBeforeZoom = true;
2591​ }
rkflxUnsubmitted
Done

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?

rkflx: Could you explain why you need to change `showPreviewsEnabledBeforeZoom` here? As far as I can…
anemethAuthorUnsubmitted
Not Done

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

anemeth: Without this the previews are always disabled when opening the dialog. This was added to enable…
2592​
2593​ KToggleAction *previewAction = static_cast<KToggleAction *>(actionCollection->action(QStringLiteral("inline preview")));
2594​ previewAction->setEnabled(isIconSizeBigEnoughForPreview);
2595​
elvisangelaccioUnsubmitted
Not Done

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

elvisangelaccio: Maybe we can also add an hint about how to actually show the previews? (i.e. the fact that the…
anemethAuthorUnsubmitted
Not Done

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

anemeth: Do you have a suggestion? I think it is self explanatory. @ngraham what do you think?
ngrahamUnsubmitted
Not Done

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

ngraham: @anemeth I'm inclined to agree, though there's always room for improvement, and more clarity is…
2596​ if (isIconSizeBigEnoughForPreview) {
2597​ previewAction->setToolTip(i18n("Show Preview"));
2598​ } else {
2599​ previewAction->setToolTip(i18n("Icon size is too small for preview"));
rkflxUnsubmitted
Done

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

rkflx: Wording: "for preview" → "for showing previews" (Or just use @ngraham's wording instead of…
2600​ }
2601​
2602​ calledByPreviewStateUpdate = true;
2603​ previewAction->setChecked(isIconSizeBigEnoughForPreview && showPreviewsEnabledBeforeZoom);
2604​ calledByPreviewStateUpdate = false;
2605​}
2606​
2564void KDirOperator::Private::updateListViewGrid() 2607​void KDirOperator::Private::updateListViewGrid()
2565{ 2608​{
2566 if (!itemView) { 2609​ if (!itemView) {
2567 return; 2610​ return;
2568 } 2611​ }
2569 2612​
2613​ updateIconPreviewsState();
2614​
2615​ const QFontMetrics metrics(itemView->viewport()->font());
rkflxUnsubmitted
Done

Unrelated change.

rkflx: Unrelated change.
2616​
2570 QListView *view = qobject_cast<QListView *>(itemView); 2617​ QListView *view = qobject_cast<QListView *>(itemView);
2571 2618​
2572 if (!view) { 2619​ if (!view) {
2573 return; 2620​ return;
2574 } 2621​ }
2575 2622​
2576 const bool leftChecked = actionCollection->action(QStringLiteral("decorationAtLeft"))->isChecked(); 2623​ const bool leftChecked = actionCollection->action(QStringLiteral("decorationAtLeft"))->isChecked();
2577 2624​
2578 if (leftChecked) { 2625​ if (leftChecked) {
2579 view->setGridSize(QSize()); 2626​ view->setGridSize(QSize());
2580 KFileItemDelegate *delegate = qobject_cast<KFileItemDelegate *>(view->itemDelegate()); 2627​ KFileItemDelegate *delegate = qobject_cast<KFileItemDelegate *>(view->itemDelegate());
2581 if (delegate) { 2628​ if (delegate) {
2582 delegate->setMaximumSize(QSize()); 2629​ delegate->setMaximumSize(QSize());
2583 } 2630​ }
2584 } else { 2631​ } else {
2585 const QFontMetrics metrics(itemView->viewport()->font()); ​
rkflxUnsubmitted
Done

Unrelated change.

rkflx: Unrelated change.
2586 int size = itemView->iconSize().height() + metrics.height() * 2; 2632​ int size = itemView->iconSize().height() + metrics.height() * 2;
rkflxUnsubmitted
Done

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

rkflx: You still got an unrelated whitespace change here (check on Phabricator with {nav Revision…
2587 // some heuristics for good looking. let's guess width = height * (3 / 2) is nice 2633​ // some heuristics for good looking. let's guess width = height * (3 / 2) is nice
2588 view->setGridSize(QSize(size * (3.0 / 2.0), size + metrics.height())); 2634​ view->setGridSize(QSize(size * (3.0 / 2.0), size + metrics.height()));
2589 KFileItemDelegate *delegate = qobject_cast<KFileItemDelegate *>(view->itemDelegate()); 2635​ KFileItemDelegate *delegate = qobject_cast<KFileItemDelegate *>(view->itemDelegate());
2590 if (delegate) { 2636​ if (delegate) {
2591 delegate->setMaximumSize(QSize(size * (3.0 / 2.0), size + metrics.height())); 2637​ delegate->setMaximumSize(QSize(size * (3.0 / 2.0), size + metrics.height()));
2592 } 2638​ }
2593 } 2639​ }
2594} 2640​}
▲ Show 20 Lines • Show All 74 Lines • Show Last 20 Lines