-
Notifications
You must be signed in to change notification settings - Fork 38
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 macOS embed UI resize from host #129
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: falkTX <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, although, bad timing, I'm gutting some of the problematic size/pos stuff as we speak. I left some review comments as notes, but don't waste any time trying to address them in the code.
@@ -1806,7 +1811,7 @@ - (void)windowDidExitFullScreen:(NSNotification*)notification | |||
[impl->drawView setFrameOrigin:drawPt.origin]; | |||
|
|||
// Dispatch new configuration | |||
return dispatchCurrentChildViewConfiguration(view); | |||
return dispatchCurrentChildViewConfiguration(view, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not so much false because "we [...] set size just before", but because this function must not change the size at all.
@@ -197,6 +197,11 @@ | |||
return PUGL_SUCCESS; | |||
} | |||
|
|||
if (drawViewResize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is an appropriate place to be messing with the views, before this function did just what it said on the tin, now it's dispatchCurrentChildViewConfigurationExceptSometimesMaybeResizeTheDrawView
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right yes, it could be done on the caller function. but I took advantage that size was being calculated nearby to just plug the fix in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Come to think of it, at a higher level, doing this in dispatchExpose
seems like a kludge. This should have happened earlier, although I'm not sure where off the top of my head.
Seems dropping support for noUserResize is forcing bugs to be discovered and fixed now. In retrospect everyone should have done that ages ago, if the experience with size and position stuff has taught us anything, it's that workarounds only make things worse. |
Initially discovered in trummerschlunk/PodcastPlugins#22
Ardour does not currently support fixed-size LV2 UIs, it always allows resizing.
This works fine on Linux/X11 but was causing some odd issues on macos, related to the ticket above.
When resizing through the plugin ui (and Ardour then adjust to it), everything seems well.
After some tests I noticed that the opengl draw (ns)view did not have the correct size applied when resized from the host, even though the main wrapper view did.
Maybe we assumed that
[impl->wrapperView setAutoresizesSubviews:YES];
would be enough, but it is clearly not, leading to this issue.Fix is possible by forcing the draw view to be resized, only done for the cases where we dont set size just before