Change track on scroll option

[RozeFound]

Closes #91

I added added a bit of buffer before changing the track, so only the last one of scroll events calls the function

[Batwam]

Thanks for sharing this. Full disclosure, I haven't tested the code, I only read it.

Based on my reading the implementation looks fine, I just think that you will also need to remove the timeout in _disable() as the EOG reviewers usually expects everything to get cleaned up on exit.

Regarding the timeout mechanism, I understand that it will execute 100ms after the last scroll event as the timeout is getting reset. This might make it feel a bit sluggish. Do you think that it would be possible to execute the action right away, and instead use the timeout purely as some sort of lock which prevents the song skip action from being repeated as long as it's not complete?

Regarding the duration of that lock/timeout, I would have thought that 1000ms would be reasonable. If you wanted to skip multiple songs by scrolling, what sort of frequency would feel natural to you? I tried to see how quickly I would tap and got to 70bpm/1110ms personally.

[RozeFound]

Do you think that it would be possible to execute the action right away, and instead use the timeout purely as some sort of lock which prevents the song skip action from being repeated as long as it's not complete?

Unfortunately I have no idea how to implement it in any other way, I simply can't find the way to determine if that was last scroll or not without the delay. Timeout here works purely as a buffer, if you try to scroll in this 100ms gap, the timer will reset, otherwise it'll execute the action. I think 100ms is barely noticeable anyway with average human reaction time around 200-250ms.

[RozeFound]

I reduced the delay to 75ms, now it feels almost instant, but still prevents from rapid song-skipping. And you can skip songs relatively fast if you want to skip multiple.

[Batwam]

Timestamps might be the easiest solution for this. we use timestamps in other parts of the scope (see players.js). Something simple like this using timestamps works for instance:

    case "track-change":
        let timeGap = Date.now() - this.__track_change_tid;
        let scrollDelay=700;
        if ( ! this.__track_change_tid || timeGap > scrollDelay){ //700ms gap to prevent repeated skips
            this.__track_change_tid = new Date().getTime();
            if (delta > 0) 
                this._activateAction("next-track");
            else if (delta < 0) 
                this._activateAction("prev-track");
        }
        break;

it will execute right away but will not allow another skip for another 1000ms. What do you think?

I don't think that you need to define this.__scroll_delay globally as it's only used in one function. I have it defined as a local variable above for clarity. I also combined with this._changeTrack as it only have a few lines and it's called only once.

[RozeFound]

Huh, that's another way to do it. I never wrote a line of JavaScript in my life before, so I implemented more generic, or I'd say GTK way. Yup, this way will also remove overhead of queuing and canceling a function call every excess scroll. But it's basically upside-down to my solution, and if you scroll long enough it will change the track twice. Also this feels a bit restrictive, you basically forbid the user to change tracks often then once a second, which is reasonable but some may consider it as unresponsive.

[RozeFound]

To sum up

My solution:

• Any length
• Fixed speed
• Delayed execution

Your solution:

• Fixed length
• Any speed
• No delay

No idea what's better tbh

[RozeFound]

I have an idea, we can combine two solutions

We change the track, create timestamps, and if the user is trying to scroll in this gap, we'll reset timestamp.

    case "track-change":
        let timeGap = Date.now() - this.__track_change_tid;
        let scrollDelay=100;
        if (!this.__track_change_tid || timeGap > scrollDelay) {
            if (delta > 0) 
                this._activateAction("next-track");
            else if (delta < 0) 
                this._activateAction("prev-track");
        }
        this.__track_change_tid = new Date().getTime();
        break;

This way we have basically my solution but without delay

[Batwam]

yeah, the updated solution with timestamps works. I actually reworked the solution and tried to implement it using your original timeouts in the meantime which appears to work. I still feel like timestamps are easier to work with / troubleshoot and you don't need extra cleanup on _disable(). See below for what it's worth:

    case "track-change":
        if (this.__track_change_tid)
            GLib.Source.remove(this.__track_change_tid);
        else {
            if (delta > 0) 
                this._activateAction("next-track");
            else if (delta < 0) 
                this._activateAction("prev-track");
        }

        let scrollDelay=100;
        this.__track_change_tid = GLib.timeout_add(GLib.PRIORITY_HIGH,
            scrollDelay, ()=> {this.__track_change_tid = null});
        break;

[RozeFound]

Glib timeouts like this gives unnecessary overhead I think especially with high priority Anyway, how does current solution looks?

[Batwam]

that seems to work fine. Upon reflection, I think that in the spirit of the rest of the rest extension where everything is customisable and very little is hard coded, we should probably allow the user to vary the scroll delay just in case. Would you mind adding a spin button to this section and set your preferred values as default?

you will need to update the xml file in schemas. to generate the gschemas.compiled, run glib-compile-schemas schemas/ within the extensions' folder. more details here if needed. (don't forget to add it to addResetButton as well)

[Batwam]

see proposed implementation in the commit above. Let me know what you think.

[RozeFound]

How about we make it grayed out when track change is not selected? Or even hide entirely

[Batwam]

looking good, I just updated from visible to sensitive for consistency with Enable double clicks on the same settings page

[Batwam]

That's all looking good to me. Is the extension working the way you wanted now?

@Moon-0xff if no further comment, I'll go ahead and merge.

[RozeFound]

Yep, works like I wanted it to

[Moon-0xff]

This is good! Only I would have squashed the merge instead of adding the whole commit history.

I'm also thinking that this code can be refactored to allow execution of all or most actions (probably some will crash the shell though).