Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lp1719474: Fix unresponsive scrolling through crates & playlists using encoder #1619

Merged
merged 1 commit into from
Apr 17, 2018
Merged

lp1719474: Fix unresponsive scrolling through crates & playlists using encoder #1619

merged 1 commit into from
Apr 17, 2018

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Apr 16, 2018

https://bugs.launchpad.net/mixxx/+bug/1719474

A delay of 100 ms provides (almost) smooth scrolling and doesn't have a noticeable impact on the responsiveness when clicking on a crate or playlist item using the mouse.

You need a controller with a properly mapped encoder to test this PR!

@uklotzde uklotzde changed the title lp1719474: Fix unresponsive scrolling through crates & playlists with encoder lp1719474: Fix unresponsive scrolling through crates & playlists using encoder Apr 16, 2018
@uklotzde uklotzde added this to the 2.1.1 milestone Apr 16, 2018
QObject* parent)
: QObject(parent),
m_pConfig(pConfig),
m_clickedChildActivationTimeoutMillis(0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is 0 here allowed?

Copy link
Contributor Author

@uklotzde uklotzde Apr 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is the default behavior, i.e. just process all system events and let the timer fire immediately. Almost the same behavior as before, just one roundtrip through the event loop. Prevents special case handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we could also define a default > 0 if that makes sense. Currently it is opt-in, only used by crates and playlists.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the timer fires mediately with 0? The Qt doc is not clear about that, or negative values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally clear about that: "As a special case, a QTimer with a timeout of 0 will time out as soon as all the events in the window system's event queue have been processed. This can be used to do heavy work while providing a snappy user interface: ..."

@daschuer
Copy link
Member

Ups,Yes now I see.
LGTM

@daschuer
Copy link
Member

And works as it should.

@daschuer daschuer merged commit 34b0a82 into mixxxdj:2.1 Apr 17, 2018
@nikmartin
Copy link
Contributor

In testing this, it's working fine, but could be user/hardware dependent on the delay time that provides adequate scrolling and loading responsiveness. In my case, I changed the delay to 250 ms and it was perfect. About the time I'm deciding I want to see the contents of the crate or playlist is when it loads now, but never loads while scrolling. 100 msec would still cause delays due to db loading as I would scroll across folders. Is this possibly a candidate for a config option in the library preferences dialog?

@Be-ing
Copy link
Contributor

Be-ing commented Apr 17, 2018

No, we should not clutter the preferences with yet another option for this. It should just work.

@uklotzde
Copy link
Contributor Author

I agree with @Be-ing, we should try to find a sensible default. It would be difficult to explain such a configuration option to the causal user.

Agreed, 100 ms is already very tight if you scroll slowly throw the list. I will re-test with 250 ms.

@nikmartin
Copy link
Contributor

nikmartin commented Apr 17, 2018

In that case, I'd vote for bumping the delay up a bit to 200-250 msec. At 100 msec, my brain hasn't had enough time to decide if I want to see a particular playlist. It requires me to still have to scroll pretty fast to avoid hangups, making me have to concentrate very hard on scrolling, taking away from the 'it should just work' mantra.

@uklotzde
Copy link
Contributor Author

Next evolution: #1620

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants