Align selection to center

[tobiolo]

This aligns the selection to center when selection was out of view before.

Trade-off:
🇨🇭 better placement of cells when zooming in and out
➖ bigger "jumps" when scrolling to the next cell that is outside of the view(port) as the selection will be placed to center

[aardappel]

I think adjustments for out of view should always move as little as possible. You want users to be able to keep context.

[tobiolo]

Before:

Peek.2023-08-27.17-38.mp4

After:

Peek.2023-08-27.17-39.mp4

You can see that before, when zooming out, the selection is always at the edge (r.x + r.width - canvasw).

[tobiolo]

One can also argue that with this PR, one can see more context below (until canvashis reached in this case but then the behavior is like before).

Peek.2023-08-27.17-45.mp4

[tobiolo]

On the other side, yeah it could be annoying if it jumps too much....

[AntonBogun]

The solution I came up with on my own fork is to blink (invert) the edges of the selected cell twice on a "discontinuous" transition, such as for example going to a search result or a selection change on an undo.
I could try to make that into a PR if there's a want.

[aardappel]

@t2b3
I don't think it matters too much for the zooming out case (first example), since you had no context anyway.

For the second example (extending selection) it seems useful, but it is also a bit arbitrary that it jumps by random amounts based on the current size.. if you wanted to show more context, it be better to do the old behavior - one_grid_cell_height (for this specific case).

Where it be much worse if you jump between e.g. search results.. current result is on middle of page, next result the existing code scrolls the minimum amount possible, giving a) the least amount of scrolling, and b) the most amount of change the old new location are both on screen. Your new code will simply put the new result in the middle of the screen, which would be quite bad.

So, if you wanted to make these improvements, I'd identify callers of this function, and maybe pass a flag for different possible optimal behaviors based on the operations that triggers them. Again, in case of doubt, least amount of scrolling is best.

@AntonBogun a blink to notify the user of a new selection generally sounds as nice feedback, if this can be done without complicating the code a lot (in terms of having to do this anim over time). Does it delay when the user can interact until the anim is done?

[AntonBogun]

@aardappel It works similarly to how the cursor blink works, and the rendering code lives in the selection drawing function. The idea is to activate a timer that will run 4 times which will re-render the selection with the outline on/off.

This is a video of it in action, in the beginning I am using my Selection History feature to navigate which I've made to trigger the outline.

ScreenRecorderProject30.mp4

I've been using this for a couple weeks now and it works really well thus far.

[tobiolo]

Thanks for your reply.

@t2b3

I don't think it matters too much for the zooming out case (first example), since you had no context anyway.

It might matter more when you zoom out of a cell in the center of a grid, for example?

For the second example (extending selection) it seems useful, but it is also a bit arbitrary that it jumps by random amounts based on the current size.. if you wanted to show more context, it be better to do the old behavior - one_grid_cell_height (for this specific case).

Where it be much worse if you jump between e.g. search results.. current result is on middle of page, next result the existing code scrolls the minimum amount possible, giving a) the least amount of scrolling, and b) the most amount of change the old new location are both on screen. Your new code will simply put the new result in the middle of the screen, which would be quite bad.

Do I get it right that you implicitly assume that the context is on the left and on the top of the selection? This might not be always the case when you have grids with cells around the selections when they are filled with content?

So, if you wanted to make these improvements, I'd identify callers of this function, and maybe pass a flag for different possible optimal behaviors based on the operations that triggers them. Again, in case of doubt, least amount of scrolling is best.

@AntonBogun a blink to notify the user of a new selection generally sounds as nice feedback, if this can be done without complicating the code a lot (in terms of having to do this anim over time). Does it delay when the user can interact until the anim is done?

[AntonBogun]

I think current logic just puts the whole cell text into view, regardless of what else is in the cell. This makes complete sense since a cell may be larger than even the screen.

I think any other logic will have its edge cases when it doesn't do what would be ideal, but I suspect the current logic has as little of them as possible. The only problem is that the user doesn't really know where the selection went to if it transfered to a completely different part of the treesheet, but doing a blink remedies that.

[tobiolo]

I agree that it might be a good idea to differentiate between different situations and to control it with a flag. For zooming, user expect the screen to change, but otherwise only slightly. And to keep a padding zone that will cause a jump if entered so that there will always be some distance to the edge (in the case of selection extension)

[AntonBogun]

This sounds too complicated to me, thus far the existing logic but with blink worked just fine.

[aardappel]

@AntonBogun while the idea of blinking is good, the really fat border blink you have there would be too invasive for me.. I was expecting more like blinking the actual selection box only.

@t2b3

Do I get it right that you implicitly assume that the context is on the left and on the top of the selection? This might not be always the case when you have grids with cells around the selections when they are filled with content?

No, a similar situation could occur with different configurations. I simply want the minimum amount of movement from the current view in all cases. That still means the current code can be improved, but it really depends on the case.

[tobiolo]

@AntonBogun while the idea of blinking is good, the really fat border blink you have there would be too invasive for me.. I was expecting more like blinking the actual selection box only.

@t2b3

Do I get it right that you implicitly assume that the context is on the left and on the top of the selection? This might not be always the case when you have grids with cells around the selections when they are filled with content?

No, a similar situation could occur with different configurations. I simply want the minimum amount of movement from the current view in all cases. That still means the current code can be improved, but it really depends on the case.

Ok, thanks for your reply. I am not so sure any more whether the added complexity is it worth... The PR in its current state also would violate consistent behavior as in some cases it centers a edge cell and shows regions outside of the canvas that is otherwise not accessible when the user navigates on the canvas.

[tobiolo]

@aardappel @AntonBogun
After giving it some thought, I decided to close this PR considering all the points you mentioned.
Thanks for having taken your time and for sharing the ideas! Given all the alternatives, the current implementation may be the best under these constraints.