Selection scrolling look super cool.

Looks awesome. @aacid?

Still has a fake name attached to it?

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

In D20437#447774, @aacid wrote:

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.

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?

Hi,

In D20437#448163, @sander wrote:

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.

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

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

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

In D20437#448397, @ngraham wrote:

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!

In D20437#447843, @ngraham wrote:

In D20437#447774, @aacid wrote:

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.

In D20437#448028, @kezik wrote:

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.

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

@aacid ping?

I have not reviewed this, i am not thrilled about the licensing situation, but at least relicensing wouldn't be harder than for other people so if other people have reviewed this and made sure it breaks nothing and is a good improvement i won't get mad if this lands.

Thanks @aacid. I've tested this and verified that it works. The code changes are a little bit over my head but @sander has reviewed them. I think it makes sense to land this.

Thanks for your patience, @kezik! Please feel free to continue submitting patches.

Thanks everybody

Sorry for testing it so late, now that I’m working on PageView.

I don’t like this much, see inline comment. Do you mind if I change it to error accumulation at some time?

By the way, this also affects the magnifier tool (Ctrl+6), which calls scrollPosIntoView() when dragged against or beyond the viewport edges.

Go ahead and submit a patch!