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: Swap selection start and end index if selection is created from bottom to top #967

Conversation

Doodeletion
Copy link

Fixes #966.

Copying from file diff view failed if the selection is created from bottom to top. This causes Selection.StartPosition to be below Selection.EndPosition, so the for loop would not run. Swapping start and end position in these cases fixes the issue.

On a different note, I noticed that any exception thrown in this event handler crashes the application. I think in some cases, such as this, it might be better to catch and log the exception and prompt the user about it.

@Doodeletion Doodeletion changed the title Swap selection start and end index if selection is created from bottom to top Fix: Swap selection start and end index if selection is created from bottom to top Feb 10, 2025
@Doodeletion Doodeletion force-pushed the 966-copying-from-file-diff-regression branch from c27dc03 to 33c5da5 Compare February 10, 2025 16:11
@Doodeletion Doodeletion force-pushed the 966-copying-from-file-diff-regression branch from 33c5da5 to b21bd52 Compare February 10, 2025 16:14
@Doodeletion
Copy link
Author

Sorry for force-push chaos I committed with a wrong git config. Twice. :/

@gadfly3173
Copy link
Contributor

On a different note, I noticed that any exception thrown in this event handler crashes the application. I think in some cases, such as this, it might be better to catch and log the exception and prompt the user about it.

Actually, any exception in the UI thread or any uncaught exception in other threads can cause Avalonia to crash... The non-UI thread part has been handled, but for the UI thread, there is a considerable part that Avalonia itself has not handled (such as the rendering of AvaloniaEdit). For this specific case of copying a selection, it should not throw an exception.

@love-linger love-linger self-assigned this Feb 11, 2025
@love-linger love-linger added the bug Something isn't working label Feb 11, 2025
@love-linger love-linger merged commit 821063e into sourcegit-scm:develop Feb 11, 2025
13 checks passed
@Doodeletion
Copy link
Author

Actually, any exception in the UI thread or any uncaught exception in other threads can cause Avalonia to crash... The non-UI thread part has been handled, but for the UI thread, there is a considerable part that Avalonia itself has not handled (such as the rendering of AvaloniaEdit). For this specific case of copying a selection, it should not throw an exception.

In my opinion, it is a good idea to wrap any code that is executed on the ui thread and not critical to the application (i.e. the application can "survive" the failure) in a try/catch, because bugs can always happen. When writing this code it initially had an indexing bug. Having the application crash when trying to copy text felt a bit extreme, when the copy buffer could simply have stayed empty.

That being said, I don't think SourceGit ever crashed while I was using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants