eject: double click does 'un-replace', i.e. reloads the previous track

[ronso0]

follow-up for Un-eject #4668

Long-press eject (> 800 ms) to reload the previous track.

Double-press (< 500 ms) Eject to reload the last replaced track.

Like uneject this works across decks (restore a replaced track in another deck).

TODO

• adjust tests

[ronso0]

Okay, the current implementation would break all mappings that don't send 0 when Eject is released, and some tests need adjusted (press, sleep, release).

I think I'll try the double-tap solution used for deck cloning.

[ronso0]

Does anyone have recommendations how to do timed actions (sleep) in tests?

edit QTest::qSleep(millis) seems to be the preferred solution.

[ronso0]

Done. Works and tests pass 🎉

[daschuer]

I can confirm that this PR works and the code looks good.

However I am unsure if it works as you might expect, after a dim memory in a panic situation:
"double click does 'un-replace'".

Scenario:

• Replace a track accidentally
• Shit ... eject
• Remember: double click.
• The wrong track appears and disappears.

So we do not have a double click. In fact we have the requirement to undo the eject within 500 ms.

How about bringing back the SecondLastEjectedTrack in that scenario as well?

[ronso0]

I don't understand why you would do that "Shit... eject" in the first place? That kinda feels like setting the table cloth on fire and "Shit... turn away"

I mean, I could implement it in a way that we always wait for the second click before reacting. That would allow testing for a double-click but it would also delay the single-click eject. Eject is not time-sensitive, so I that is acceptable and would be m preferred solution.

Otherwise (double-eject empty deck after panic eject) it would be

• track1 loaded
• load track2
• oh shit...
• panic, eject
• remember double-click
• double-click
  • reload track2
  • reload track1

If that one useless track load is acceptable..

[daschuer]

That was exactly my idea. Do you think that will still "feel" natural?

[ronso0]

I don't know, we'd have to try.

However, I'm still not convinced we need to worry about the "Oh shit... eject" situation, I consider that part of the learning process tbh. What matters is the feature description (tooltip and manual): if that clearly states that double-click does does un-replace while the wrong new track is still loaded we're fine I'd say.

[daschuer]

When trying you PR the first time I had just the expectation that I can restore the old track with a double click unconditionally.
Than I have realized it is not longer possible whenever you have reached the 500 ms tresshold. I think the feature would be more robust, if we can "undo" such a fail in passing the threshold.

[ronso0]

That was exactly my idea. Do you think that will still "feel" natural?

The delay feels weird, it's not clear why eject takes "so long".

I changed it to always un-replace on double-click, no matter the delay or if a track is loaded. Note that like uneject this works across decks (restore a replaced track in another deck).

[daschuer]

Except of the test crasher, it works good now.

Do you mean, just toggle between un-load the last and the second last track?
That works for me as well and would make the feature even more predictable.

[ronso0]

Do you mean, just toggle between un-load the last and the second last track?

That's what it does right now, no?
Double-click does eject first so last and second last are swapped each time.

I'll fix the tests.

[daschuer]

Yes, it is working fine.

I just did not understand that comment:

The delay feels weird, it's not clear why eject takes "so long".

[ronso0]

I meant it's not clear for users why there's a delay when ejecting a track because everything else happens immediately incl. track load.

[ronso0]

I fixed EngineSyncTest.EjectTrackSyncRemains, please check if that's the preferred solution (I didn't deal with tests that much, let alone the engine mock)

[Swiftb0y]

IMO PerformanceTimer is a little overkill, but we don't have anything better in mixxx right now (except possibly implementing a custom clock based on std::chrono). Unimportant.

[Swiftb0y]

Thank you. Code LGTM, still need to do a manual test though.

[ronso0]

@ywwg Do you have time to take a look?

[ronso0]

friendly ping @ywwg and @Swiftb0y maybe

[daschuer]

Looks almost good. Just a text nit pick.

[ronso0]

Okido, done

[daschuer]

Thank you.