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

Stop Darktable from crashing when closing secondary darkroom window while window is in fullscreen #18361

Merged
merged 3 commits into from
Feb 7, 2025

Conversation

stnKrisna
Copy link
Contributor

#18314

On macOS, when the secondary darkroom window is closed while it is currently full screened, there are events that are still pending that needs to be consumed. When the window is destroyed by gtk_widget_destroy before consuming all of the unfullscreen event, gtk_main() under dt_gui_gtk_run will throw exc_bad_access exception.

On macOS, when the secondary darkroom window is closed while it is currently full screened, there are events that are still pending that needs to be consumed. When the window is destroyed by gtk_widget_destroy before consuming all of the unfullscreen event, gtk_main() under dt_gui_gtk_run will throw exc_bad_access exception.
@TurboGit TurboGit modified the milestones: 5.2, 5.0.1 Feb 5, 2025
@TurboGit TurboGit requested review from TurboGit and zisoft February 5, 2025 17:24
@TurboGit TurboGit added scope: macos support macos related issues and PR bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes scope: UI user interface and interactions release notes: pending labels Feb 5, 2025
Copy link
Collaborator

@zisoft zisoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crash is 100% reproducible on macOS und successfully fixed by this PR.

@dterrahe
Copy link
Member

dterrahe commented Feb 5, 2025

Is the conditional compilation required or would this work as well (without being needed) on non OSX? It just is easier to have as little conditional compilation as possible. If you accidentally break this code by changing something around it, but you are not compiling on Mac yourself, you'll only find out in CI...

If the new code works everywhere without causing problems, I'd prefer a simple comment that it is required for Mac instead of the #ifdef

@stnKrisna
Copy link
Contributor Author

@dterrahe Im not sure. Technically it should work without the conditional compilation but I have no way to validate. I only have macOS to compile on

@TurboGit
Copy link
Member

TurboGit commented Feb 6, 2025

For me on GNU/Linux when I quit darktable and that the second window is displayed it crashes. Do you also have this on macOS?

@stnKrisna
Copy link
Contributor Author

For me on GNU/Linux when I quit darktable and that the second window is displayed it crashes. Do you also have this on macOS?

I think I used to get that behaviour. I just tried it again just now and it exits as normal

@stnKrisna
Copy link
Contributor Author

Funny enough, when I close the secondary window by clicking the close button, darktable did not crash. Only when I click the "Display a second Darktable" button it crashes

Simplify cleanup so that its all located in _second_window_delete_callback
Let MacOS handle with the fullscreen state change
@stnKrisna stnKrisna changed the title Consume unfullscreen event when closing secondary window Stop Darktable from crashing when closing secondary darkroom window while window is in fullscreen Feb 6, 2025
@TurboGit
Copy link
Member

TurboGit commented Feb 6, 2025

@stnKrisna : Before merging one question, what is your macOS version? I'd like to have some other macOS dev testing this on different versions.

@zisoft @MStraeten

@TurboGit
Copy link
Member

TurboGit commented Feb 6, 2025

@wpferguson : BTW, would be nice if you could also test this on Windows before merging to 5.0.1. TIA.

@zisoft
Copy link
Collaborator

zisoft commented Feb 6, 2025

No crash on macOS 15.3, darktable with homebrew build.

The scenario from the issue desctiption is fixed.
Closing darktable with secondary window open does not crash either.

I will test on windows as well tomorrow.

@stnKrisna
Copy link
Contributor Author

@TurboGit Im on macOS 14.6.1 (23G93)

@MStraeten
Copy link
Collaborator

MStraeten commented Feb 7, 2025

fix also works with my darktable-5.0.x bugfix build for macos 15.3

@zisoft
Copy link
Collaborator

zisoft commented Feb 7, 2025

No crash on Windows 11 Enterprise with 2 screens.

Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So all good, thanks all!

@TurboGit TurboGit merged commit 2b208ad into darktable-org:master Feb 7, 2025
6 checks passed
TurboGit pushed a commit that referenced this pull request Feb 7, 2025
…hile window is in fullscreen (#18361)

* Consume unfullscreen event when closing secondary window

On macOS, when the secondary darkroom window is closed while it is currently full screened, there are events that are still pending that needs to be consumed. When the window is destroyed by gtk_widget_destroy before consuming all of the unfullscreen event, gtk_main() under dt_gui_gtk_run will throw exc_bad_access exception.

* Refactor

Simplify cleanup so that its all located in _second_window_delete_callback

* Let GTK destroy the window

Let MacOS handle with the fullscreen state change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes scope: macos support macos related issues and PR scope: UI user interface and interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants