Improve scroll wheel speed by basing it on label height, not icon height
ClosedPublic

Authored by ngraham on Feb 20 2019, 5:11 PM.

Details

Summary

Dolphin currently scrolls by the height of three items at a time per "step" when
using a scroll wheel. Because item height is highly variable, this leads to scroll
speed being inconsistent between views, and generally far too fast when using
icon view with icons larger than 22px size.

This patch makes the size of the scroll step based on the text label rather than the
icon size just like D25683, ensuring that the scroll speed does not vary and become
super fast when using large icons in particular.

It also reverts 90beb4a5e37b887caad1e767046a42dad0af1ab3, which is no longer needed.

BUG: 386379
FIXED-IN: 19.12.1

Test Plan

Use a mouse with a scroll wheel and scroll in Dolphin item views with list view,
details view, icon view, etc, using different item sizes. Speed should be
consistent in all views now, and also feel consistent with other KDE apps.

Also try with multiple scale factors to make sure the behavior does not change.

No change with high-resolution two-finger touchpad scrolling.

Diff Detail

Repository
R318 Dolphin
Branch
improve-scrollwheel-speed (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19880
Build 19898: arc lint + arc unit
ngraham created this revision.Feb 20 2019, 5:11 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptFeb 20 2019, 5:11 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
ngraham requested review of this revision.Feb 20 2019, 5:11 PM
ngraham updated this revision to Diff 52164.Feb 20 2019, 5:29 PM

Use font metrics

ngraham retitled this revision from [RFC] Improve scroll wheel speed to Improve scroll wheel speed by basing it on label height, not icon height.Feb 20 2019, 5:31 PM
ngraham edited the summary of this revision. (Show Details)
anthonyfieroni added inline comments.
src/kitemviews/kitemlistcontainer.cpp
266–267

I always think that we have that step in library, i have in mind that in system settings has a scroll speed, now i don't see, somethings changed and i did not notice :)

ngraham added inline comments.Feb 20 2019, 5:36 PM
src/kitemviews/kitemlistcontainer.cpp
266–267
ngraham edited the test plan for this revision. (Show Details)Feb 20 2019, 5:37 PM

This patch makes the size of the scroll step fixed, using the height of 2.5 text labels + their padding, which is what KDirOperator does.

Are you sure? It seems to me that the file dialog respects the standard 3-lines/items scroll step, while this patch breaks it.

This patch makes the size of the scroll step fixed, using the height of 2.5 text labels + their padding, which is what KDirOperator does.

Are you sure? It seems to me that the file dialog respects the standard 3-lines/items scroll step, while this patch breaks it.

Ah, but what constitutes an item? The bug is that the icon's height counts for the the purpose of determining the height of one line, which makes the scroll speed variable for views with different icon layouts and height, and very uncomfortable when icons are large. It's really annoying.

Ping! This is one of the most common issues in Dolphin that I hear complained about, and I experience it myself, so I'm quite eager to see it fixed. :)

elvisangelaccio requested changes to this revision.Mar 9 2019, 6:55 PM

Sorry but this patch makes dolphin scroll differently from the file dialog in details view mode, so it cannot go in as-is.

This revision now requires changes to proceed.Mar 9 2019, 6:55 PM
ngraham planned changes to this revision.Mar 10 2019, 4:29 AM

OK. We get the exact complaint for the file dialogs, so I will prepare a patch to do the same thing there too.

https://bugs.kde.org/show_bug.cgi?id=223937

ngraham requested review of this revision.Mar 14 2019, 2:18 AM

(didn't mean to mark as changes planned; I'm still working on the companion KIO patch)

@ngraham Have you made any further progress on this? Like you and I assume most everyone else, I'm so tired of this bug! I doubt I can be of much help but I'm going to take a stab at it anyway. Let me know if there's specific task/code I should focus on.

I have not been able to figure out how to do the same thing for the file dialogs to unblock this. Feel free to work on that if you have an idea of how to do it.

@ahiemstra has graciously submitted a patch for KDirOperator: D25683

@elvisangelaccio can you review or approve this one?

ahiemstra added inline comments.Dec 3 2019, 10:29 AM
src/kitemviews/kitemlistcontainer.cpp
266–267

Note that in D25683 I have used QApplication::wheelScrollLines() which to me seems to be the right method to use for this.

ngraham updated this revision to Diff 70857.Dec 3 2019, 8:47 PM
ngraham marked an inline comment as done.

Use QApplication::wheelScrollLines() instead of a magic number

ngraham edited the summary of this revision. (Show Details)Dec 3 2019, 8:48 PM
elvisangelaccio requested changes to this revision.Dec 8 2019, 8:45 PM

Please see 90beb4a5e37b887caad1e767046a42dad0af1ab3

Since this was the only place where itemSizeHint() was used, we can basically revert that commit.

This revision now requires changes to proceed.Dec 8 2019, 8:45 PM
ngraham updated this revision to Diff 71122.Dec 9 2019, 4:32 PM

Also revert 90beb4a5e37b887caad1e767046a42dad0af1ab3 which is no longer needed

ngraham edited the summary of this revision. (Show Details)Dec 9 2019, 4:33 PM
ngraham edited the summary of this revision. (Show Details)
src/kitemviews/kitemlistview.h
156

We are dropping KItemListView::itemSizeHint(), so it should not be mentioned here, right?

ngraham updated this revision to Diff 71603.Dec 15 2019, 3:03 PM
ngraham marked an inline comment as done.

Make the comment accurate

ngraham updated this revision to Diff 71604.Dec 15 2019, 3:03 PM

...Now with more accuracy

elvisangelaccio accepted this revision.Dec 21 2019, 5:59 PM
This revision is now accepted and ready to land.Dec 21 2019, 5:59 PM