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 on open TreeSheets files on MacOS #794

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

tobiolo
Copy link
Collaborator

@tobiolo tobiolo commented Dec 25, 2024

When file was loaded directly via the open protocol on MacOS,
System::FileUsed(const wxString&, Document*) had been called before
the app event loop was entered. It thus tried to access the uninitialized
file system watcher which led to a crash.

Fix this by contructing the file system watcher right away.

@tobiolo tobiolo requested a review from aardappel December 25, 2024 17:40
@aardappel
Copy link
Owner

These all seem significant changes that possibly affect multiple platforms, some explanation as to what these changes accomplish would be good.

@tobiolo tobiolo force-pushed the fix-crash-macos branch 2 times, most recently from a592369 to d6db978 Compare December 30, 2024 09:58
@tobiolo
Copy link
Collaborator Author

tobiolo commented Dec 30, 2024

Thanks for your feedback. I refined the commit message. Would this be ok for you now?

@aardappel
Copy link
Owner

Where is the commit message? Don't see it here on the PR.

Would be better to comment on the individual changes.

@tobiolo
Copy link
Collaborator Author

tobiolo commented Dec 31, 2024

I added to the description of the PR.

@tobiolo
Copy link
Collaborator Author

tobiolo commented Jan 2, 2025

@aardappel

@aardappel
Copy link
Owner

Ok, I see the file system watcher being constructed immediately, but there are other changes in this PR, which will also affect other platforms.

If the changes are unrelated, then its best to stick them into separate PRs, so those can have their own explanation.

If they are related, I'd like to know why they are needed in addition to moving the FS watcher, and how they don't matter on non-mac platforms.

@tobiolo
Copy link
Collaborator Author

tobiolo commented Jan 8, 2025

Thanks @aardappel for your feedback. I reduced the changes.

@tobiolo tobiolo merged commit 0575002 into aardappel:master Jan 13, 2025
0 of 4 checks passed
@tobiolo tobiolo deleted the fix-crash-macos branch January 13, 2025 16:27
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