search

aardappel / treesheets

TreeSheets : Free Form Data Organizer (see strlen.com/treesheets)
zlib License
2.5k stars 185 forks source link

Fix crash when moving cells after copy paste while in textedit #603

Closed AntonBogun closed 5 months ago

tobiolo commented 5 months ago

Hi @AntonBogun Could you describe with an example how you produce the crash? Thanks in advance! I tried to reproduce with the help of the title of the Pull Request, but I could not produce a crash or move cells at all while Selection is in textedit mode. Best regards, Tobias

AntonBogun commented 5 months ago

Produce the crash like this: Write some text in two cells (unsure if necessary but regardless), Select them Move over to a different cell and enter text edit Paste (you should be selecting cells at this stage except treesheets didn't realize you're not in text edit anymore) Finally, press ctrl+right (or any arrow I think)

tobiolo commented 5 months ago

Hi @AntonBogun

Thanks for your reply. I could reproduce the issue. Thank you very much for pointing out to this issue and for your attention to detail!

While I definitively see your valid idea in your PR, I have two major concerns with this PR:

  1. While its intention seems right to me, I think that it is "too late" to switch the textedit boolean at the proposed place. Rather than letting the wrong state of Selection::textedit get into the function void Selection::Dir(Document*, bool, bool, wxDC&, int, int, int, int, int, bool, bool, bool) and correct with the help of an extra condition check (GetCell()), it should be fixed at the origin of the bug. According to my investigation the bug roots in the miss of the textedit not being switched off when a grid gets merged into its parent grid (void Grid::MergeWithParent(Grid*, Selection&)), so I created another proposal in a PR.
  2. It is quiet expensive to check the condition GetCell() at every call of void Selection::Dir(Document*, bool, bool, wxDC&, int, int, int, int, int, bool, bool, bool). As said in 1, we should come "clean" to the function.

Another minor concern is that there is a proper method in the Selection class to leave textedit mode with the function void Selection::ExitEdit(Document*) that also resets the cursor.

I hope that you can agree with my points here. @aardappel Which way do you prefer? My alternative approach lies in PR https://github.com/aardappel/treesheets/pull/604

Best regards, Tobias