Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

AntonBogun
Copy link
Contributor

No description provided.

@tobiolo
Copy link
Collaborator

tobiolo commented Jan 25, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

tobiolo commented Jan 26, 2024

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 #604

Best regards,
Tobias

@tobiolo tobiolo closed this Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants