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

Move Zyn GUI to main thread #4065

Merged
merged 1 commit into from Dec 22, 2017
Merged

Move Zyn GUI to main thread #4065

merged 1 commit into from Dec 22, 2017

Conversation

@tresf
Copy link
Member

@tresf tresf commented Dec 21, 2017

Closes #703

We'll need some solid testing on Windows and Linux to make sure no regressions are caused as a result of this change.

@tresf tresf added this to the 1.2.0 milestone Dec 21, 2017
@PhysSong
Copy link
Member

@PhysSong PhysSong commented Dec 22, 2017

As the author of this patch, I'd like to say that it should be fine on every platforms. The reasons are:

  1. It is safe to execute the message loop in any thread since it doesn't call UI functions or thread-dependent functions(as far as I know).
  2. It is normal to run UI loop in main thread. It's safe, too.
  3. I've changed the synchronization logic a little bit. Now the message processing thread always finishes before the GUI loop. So there should'n be any threading issues.

Anyway, testing is always good. 👍

@tresf
Copy link
Member Author

@tresf tresf commented Dec 22, 2017

@tresf
Copy link
Member Author

@tresf tresf commented Dec 22, 2017

We've extensively tested this on Mac, Linux and Windows.

Here's some evidence that the Zyn UI can remain open and stable with this PR despite several preset changes. Worth noting... I was able to get it to crash twice, but I cannot reproduce now despite how hard I try. When it crashed, the stack trace did not point to the UX, so @PhysSong suspects I had found an existing edge case that already exists.

untitled

@tresf tresf merged commit a3d72de into LMMS:stable-1.2 Dec 22, 2017
1 check passed
@tresf tresf deleted the zynmac branch Dec 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants