Maintain active layer track in view

[HeCorr]

Fixes #1858.

This change maintains the currently selected layer visible when the active layer changes, so that using the Up and Down arrow keys works as expected:

2024-07-24.02-20-20.mp4

It also works when the layer is far away from view, not just right above/below:

2024-07-24.02-38-54.mp4

Caveat

When clicking on a layer that is barely in view (which isn't considered as in view by the code), it is moved due to the sudden scroll as the cursor is still being held:

2024-07-24.01-59-36.mp4

I thought of making it so that the layer is only selected upon mouse release but I'm willing to bet the code responsible for reordering layers decides which one to move based on which one is active, but I could be wrong.

Another potential solution would be to snap the timeline height so that it never shows half a layer:

[MrStevns]

Thanks for the contribution, this is an excellent improvement to the timeline.

Changes looks good to me, and it works as expected for me.
Good job 👍

Feel free to merge if you're happy with the changes.

[MrStevns]

My bad, I missed the quite important caveat, which I can confirm happens. We need to take care of that.
Unfortunately the timeline cell logic is such a mess, it might not be trivial to fix without introducing some other bug.

You don't need to click on a half hidden layer to make it happen either, if you add 5 layers and click on layer 2, then it also happens.

[HeCorr]

Maybe instead of listening to currentLayerChanged which is emitted with both arrows and cursor clicks, I could emit a new signal from gotoPreviouslayer and gotoNextlayer, so that this only runs when using the arrows.

Or a simpler way, which I couldn't figure out is to directly listen to arrow key presses from timeline.cpp.

Or maybe even change the currentLayerChanged signal itself, adding an enum which specifies the source of the change (mouse or keyboard).

[HeCorr]

@MrStevns is the signature change of currentLayerChanged suggested above acceptable? If so I'm gonna give that a go soon.

[MrStevns]

Maybe instead of listening to currentLayerChanged which is emitted with both arrows and cursor clicks, I could emit a new signal from gotoPreviouslayer and gotoNextlayer, so that this only runs when using the arrows.

Or a simpler way, which I couldn't figure out is to directly listen to arrow key presses from timeline.cpp.

Or maybe even change the currentLayerChanged signal itself, adding an enum which specifies the source of the change (mouse or keyboard).

I'm not convinced that either of these will fix the problem. Why should changing the signature of currentLayerChanged change the behavior, when what's causing the problem is most likely the logic in timelinecells or the TimeLine::updateLayerView which controls the maximum amount of steps of the scrollbar?

edit: upon further inspection, it is indeed caused by the code in TimelineCells. For this to work properly, I think you'll have to add some logic that prevents it from doing the layer swapping on mouse release when you're not explicitly dragging.

[HeCorr]

Why should changing the signature of currentLayerChanged change the behavior?

Consider the following:

void TimeLine::currentLayerChanged(int layerIndex, LayerChangeSource source)

I can check whether source == LayerChangeSource::ArrowKey and only scroll if it's true, which would ignore mouse clicks (...unless clicking a half-visible layer already scrolls by default/outside this branch, which I honestly haven't tested).

For this to work properly, I think you'll have to add some logic that prevents it from doing the layer swapping on mouse release when you're not explicitly dragging.

I'll have to see what I can do about that.

[HeCorr]

Just checked on Nightly and indeed it does not scroll by default when clicking, which means my theoretical fix could work.

2024-07-26.17-07-02.mp4

[HeCorr]

I did a quick test and it did work, but I ended up changing too many places that use the currentLayerChanged signal, so I'm going to try to create a new signal instead.

2024-07-26.17-21-59.mp4

[MrStevns]

Consider the following:

void TimeLine::currentLayerChanged(int layerIndex, LayerChangeSource source)

I can check whether source == LayerChangeSource::ArrowKey and only scroll if it's true, which would ignore mouse clicks (...unless clicking a half-visible layer already scrolls by default/outside this branch, which I honestly haven't tested).

We do not want to complicate the layer manager with what source; be it keyboard or mouse, is required to trigger an behavior that is meant for a GUI element.

It's a good thing that you're experimenting and that you've found a few solutions that works. However, I still think we should deal with this through the interface code, rather than adding an additional emitter logic to the LayerManager, when the source of the problem comes from the timeline logic.

I can understand if you're reluctant to add more code to the timeline cell logic, as it's a mess but that's imo. the correct way to fix this issue. Any other way is beating around the bush.

[HeCorr]

I can understand if you're reluctant to add more code to the timeline cell logic, as it's a mess

I wouldn't even know where to begin.

I understand that this simple fix is not the ideal solution but it works and unfortunately this is all I can provide at the moment.

[MrStevns]

I can understand if you're reluctant to add more code to the timeline cell logic, as it's a mess

I wouldn't even know where to begin.

I understand that this simple fix is not the ideal solution but it works and unfortunately this is all I can provide at the moment.

No worries, I took a stab at it which required minimal amount of changes to TimeLineCells.
You can check out the commits yourself but the gist of it is that we've introduced a mScrollingVertically property so we can avoid moving the layers while actively scrolling. Once that's done, we still have one problem to solve though, which is how do we reset the value?

I figured that a timer that triggers after x milliseconds would do the trick. It will only trigger once we've scrolled one way or another, and only once.

After inspecting the TimeLine class myself I also noticed that we had a ´layerChanged` slot already, so I've refactored your logic into that method instead of having an additional signal.

Let me know what you think.

[HeCorr]

I personally don't understand why you would opt for a more complex implementation that uses a timer over simply emitting two signals and picking one to listen to depending on what makes the most sense.

Sure, it wasn't an elegant solution, but it was simple and worked pretty well. That being said, if the new solution works better for you then there's nothing I can do about it.

[MrStevns]

I get your point but what might seem like a simpler solution at first, doesn't make sense in the long run.

Your solution adds seemingly abysmal complexity to the core layer manager, an object that is tied to everything, to fix a specific scenario in a specific GUI component, while I add logic to the timeline, which only affects that specific component and lives in the APP part of the code base.

Consider this example. Another person writes their own app client for Pencil2D; possibly for mobile, and write a better timeline implementation. Why should they have to deal with this new signal, when it's a problem in our GUI code?

Small things like this add up over time and makes the code base less maintainable. Even though we're open source, doesn't mean we don't want the code to be as good as it can be.

It's not a rant nor critique, truthfully, I'm just trying to voice why I think it's important to fix bugs where they appear. :)