SerenityOS / serenity

The Serenity Operating System 🐞
https://serenityos.org
BSD 2-Clause "Simplified" License
30.59k stars 3.19k forks source link

Spreadsheet: Change Cell Selection System #4313

Open supex0fan opened 3 years ago

supex0fan commented 3 years ago

At the moment selecting cells doesn't really work as noted in #4277, but that is just because a drag selection gets canceled if the cursor goes within 5 pixels of any corner of any cell.

But when cell selection does work, I think that the current solution of using the TableView's Model and ModelIndexes to select cells has some issues in its natural state as Model selection doesn't conform to the cell selection rules. For example, if you hold control and draw a circle in Spreadsheet, everything within the circle will be selected, rather than just a rectangle from where the selection started and where the mouse currently is.

There will also be issues in the future when we can't tell where one selection ends and another selection begins. For example, when implementing the ability to extend selections we have to set the cursor to a crosshair when the cursor is close to the bottom right of a selection block, which will be difficult if selections are just a HashTable of Positions and there could be multiple selections in that HashTable. The same goes for dragging a selection to move its contents elsewhere.

I have created a Range class which is just a start and end position (start is always top-left of the selection, and end is always the bottom-right). Each Sheet simply has HashTable of Ranges, it is very easy to check that a Position or ModelIndex is selected. I am having some issues with the TableCellPainter's paint() method not getting called by the TableView's mousedown and mouseup events. I'm also not sure what internal functions may be broken by not using the builtin ModelSelection system, if there are any problems both systems could be run simultaneously.

@alimpfard Let me know what you think of the idea.

alimpfard commented 3 years ago

Your idea sounds reasonable, though keeping LibGUI's selection and now our own selection in sync (as quite a few things depend on LibGUI's selection being up-to-date) might be a bit tedious? If you feel like it, definitely give this a go!

re paint(), I believe that only fires on an actual paint event, what are you trying to do that requires that paint() to be run on mouse down/up?

internal functions that may be broken by not using the builtin ModelSelection system

Off the top of my head, I remember these depending on it:

awesomekling commented 3 years ago

Since this is useful outside of just the Spreadsheet app, let's focus on solving this in LibGUI :)

The rubber banding selection system currently used by GUI::TableView should be optional. The cell-based selection mechanism you describe would be another option.

And using ranges inside a model selection sounds like a better solution in general :)

danboid commented 3 years ago

Erm... wot Kling sed!

supex0fan commented 3 years ago

I actually ended up creating the Range system at the Spreadsheet level yesterday and I am happy with how it turned out, but as alimpfard said, it is annoying to maintain two selection systems. So I will take a look at LibGUI and try to move the solution up a level.

@awesomekling Just to clarify on your last sentence, do you want the ModelSelection to work with ranges instead of indexes, or do you want a system where both exist and either can be used?