Make selection scrolling go at 60 fps
AcceptedPublic

Authored by kezik on Apr 10 2019, 2:57 PM.

Details

Reviewers
aacid
sander
ngraham
Group Reviewers
Okular
Summary

When you select a square with the right mouse button, and you go past the margin of the window (but where there is still space to go), okular scrolls the document so you can select more.
With this patch this automatic scrolling goes at 60 fps instead of 10, I made it so that the speed of the scroll is the same

Diff Detail

Repository
R223 Okular
Lint
Lint Skipped
Unit
Unit Tests Skipped
kezik created this revision.Apr 10 2019, 2:57 PM
Restricted Application added a subscriber: okular-devel. · View Herald TranscriptApr 10 2019, 2:57 PM
kezik requested review of this revision.Apr 10 2019, 2:57 PM

Selection scrolling look super cool.

ngraham added subscribers: aacid, ngraham.

Looks awesome. @aacid?

aacid added a comment.Apr 10 2019, 9:29 PM

Still has a fake name attached to it?

If so i'll refrain from looking at the code and getting tainted by it.

Still has a fake name attached to it?

If so i'll refrain from looking at the code and getting tainted by it.

Can you clarify how reviewing this patch would taint you?

@kezik maybe in the interest of getting something done, you can pretend your real name is "Kezi Olio" and Albert can pretend he doesn't know that it's a fake name. Actually, we have no real way of verifying that the name someone uses is their real name unless they publicly announce "hey this is a fake name because I want to be anonymous", so the whole thing seems rather silly to me. For example @aacid, how do I know that "Albert Astals Cid" is your real name? I've never seen any documentation to back it up... :)

It's the first time after years and dozen of contributions to dozen of projects that I see such a fuss about my internet name

@aacid if I put my relicensing preferences on the kde dev script repo, will you consider the patch?
Why there is no relicensing agreement the moment I sign up / make a contribution? Why is this even a problem?

I can put a random name but that wouldn't really solve any problems.
In my country it's not illegal to use random names as long as I don't steal someone's identity or use the nickname to commit a felony, as i'm doing neither of those, and the contracts I sign are still 100% valid (this account is still identifiable to me) I don't see where the problem is.

sander added a subscriber: sander.Apr 11 2019, 2:28 PM

How did you determine the value of the damping variable? You write

With this patch this automatic scrolling goes at 60 fps instead of 10, I made it so that the speed of the scroll is the same.

If somebody in the future changes the scroll speed, will you have to update your patch? Wouldn't it be better to have only one constant that determines the speed of both regular scrolling and automatic scrolling?

kezik added a comment.Apr 11 2019, 2:45 PM

Hi,

How did you determine the value of the damping variable? You write

With this patch this automatic scrolling goes at 60 fps instead of 10, I made it so that the speed of the scroll is the same.

If somebody in the future changes the scroll speed, will you have to update your patch? Wouldn't it be better to have only one constant that determines the speed of both regular scrolling and automatic scrolling?

The value is chosen so the page speed is exactly what it was before (with approximation), I made a simple calculation, increasing how often the page is moved and decreasing by how much

For what I can see, the normal scrolling speed is not really defined somewhere .. ? And even if it was, why really is the need to match the normal scrolling with this type of scrolling, since this scrolling comes from a totally different context and "usage pattern"? You don't really want the page to fly away when selecting, and maybe you like a very fast scroll wheel.

I think that maybe the damping value should be tweaked to what seems more natural in this specific context, I chose to keep it with a value that matches the past behavior so people don't scream at me

Hi Kezi, I don't have a good answer to all that, I am just a bit distrustful regarding hard-coded constants like '6'. But I acknowledge that there are situations where they cannot be avoided.

kezik added a comment.Apr 11 2019, 2:55 PM

Well, before my patch the hardcoded constant was "100", (the time between each page update)
Now I put an universal constant there (16, as in milliseconds to create a 60fps animation), and moved the hardcoded constant elsewhere

I'll update with a comment

kezik updated this revision to Diff 55993.Apr 11 2019, 3:05 PM

As requested I explain what the "arbitrary constant" does

kezik added a comment.EditedApr 11 2019, 3:07 PM

Oh, and by the way, the _actual_ speed of the page is set by how far you move the mouse outside the viewport, that constant is a multiplicative factor, it really doesn't matter, it's only set to a nice value

Thanks! I am happy with these comments (except maybe that people may read the "/ 60 fps" in line 3770 as a division).

sander accepted this revision.Apr 11 2019, 3:12 PM
This revision is now accepted and ready to land.Apr 11 2019, 3:12 PM
ngraham added inline comments.Apr 11 2019, 4:05 PM
ui/pageview.cpp
3770

Yeah I'd change the / to == or the word "equals" for clarity's sake

kezik updated this revision to Diff 56015.Apr 11 2019, 9:21 PM

Even more clear

1000/60 yields a floating-point value. Is it acceptable that std::chrono::milliseconds can be a non-integer value?

kezik added a comment.Apr 11 2019, 9:59 PM

1000/60 yields a floating-point value. Is it acceptable that std::chrono::milliseconds can be a non-integer value?

Being 1000 and 60 integers, an integer division is performed, it really is the same as before, the result of the expression is (int)16. Otherwise the compiler would give a warning.

BTW thanks for the "moral support" on the mailing list!

ngraham accepted this revision.Apr 12 2019, 8:09 PM

Out of deference to @aacid, I'll wait to commit this until the mailing list thread has come to a conclusion we can all agree with, or at least graciously accept without complaining about it later. :)

aacid added a comment.Apr 14 2019, 4:39 PM

Still has a fake name attached to it?

If so i'll refrain from looking at the code and getting tainted by it.

Can you clarify how reviewing this patch would taint you?

If you look at code that you believe you can't commit because X reasons, then you're in deep trouble if you try to fix the same issue and end up coming with the same solution since you have to "prove" you did come up with it independently and not just simply copied it.

aacid added a comment.Apr 14 2019, 4:42 PM

It's the first time after years and dozen of contributions to dozen of projects that I see such a fuss about my internet name

@aacid if I put my relicensing preferences on the kde dev script repo, will you consider the patch?

It would ease my mind a bit.

Why there is no relicensing agreement the moment I sign up / make a contribution?

Because that is a bad way to welcome newcomers. But if you want to i'd be even more happy if you sign it https://ev.kde.org/rules/fla.php

Why is this even a problem?

Because licenses will change and personally i want to make sure we're not in a terrible place if at some point your email address disappears and we need to change the license of the code.

kezik added a comment.Tue, May 7, 3:33 PM

@aacid I put myself in the kde dev script relicense thing, so please look at the patch without getting tainted

https://phabricator.kde.org/D21070

I'm not sure if I fscked up something in creating that revision, so tell me what to fix, in case.

Thanks

Thanks, I've added your preferences there.

@aacid, can we proceed now?