Caching wxWidgetCocoaImpl::GetLayoutInset() breaks some control changes (Issue #26622)
Václav Slavík
vslavik created an issue (wxWidgets/wxWidgets#26622)In PR #26353 a cache for layout insets was introduced — with the goal of speeding up window creation as per #26318.
The change is based on the assumption that alignmentRectInsets return value is "effectively static":
Can that be cached? AppKit documentation describes alignmentRectInsets as the inset from "ornamentation", I'm not sure if it can be known if the size of that ornamentation would change (my naive assumption/though would be that it doesn't actually change for native controls, thinking outer shadows and stuff), and only fetch it in that case.
Apparently that is not the case. The alignment rectangle is, at a minimum, function of the control's style and if that changes, the insets may too -- but wx keeps using the stale cached values. This was already partially discovered in a6ce659, but I got hit by another case: changing NSButton style:
auto native = static_cast<NSButton*>(toggleButton->GetHandle()); native.bezelStyle = NSBezelStyleRecessed; native.showsBorderOnlyWhileMouseInside = YES;
Doing this changes the insets, breaks the layout and makes the buttons overlap in my case.
This is of course somewhat hacky, but such "progressive enhancements" are why GetHandle is exposed after all, and the same situation would occur with wxNativeWindow-embedded native control. Plus wxButton has paths that change the bezel type too and may be directly affected.
I'm not sure what a good fix would be. It would be nice to at least make it possible to fix the situation from user code, i.e. have [Mac]InvalidateLayoutInset() as part of the public API.
/cc @ryancog
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
ryancog left a comment (wxWidgets/wxWidgets#26622)I'm not familiar with wxNativeWindow and how that API is typically used, so I'm not sure there.
In any case, I agree it'd be good to have a public MacInvalidateLayoutInset() available, assuming there are cases with wxNativeWindow that would require it. That requires a programmer to know it needs to be called, but it makes it possible, as you said.
For specific known cases like bezelStyle, would it make sense to create a wrapper around setBezelStyle in wxNSButton that would call InvalidateLayoutInset(), so that it handles things regardless of whether the changes happens in wxWidgets or via direct access to GetHandle()? That seems the most fool-proof.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Václav Slavík
vslavik left a comment (wxWidgets/wxWidgets#26622)I'm not familiar with wxNativeWindow and how that API is typically used, so I'm not sure there.
The point of it is that it wraps a native control that wx doesn’t have full control over, so anything goes inside.
For specific known cases like bezelStyle, would it make sense to create a wrapper around setBezelStyle in wxNSButton that would call InvalidateLayoutInset(), so that it handles things regardless of whether the changes happens in wxWidgets or via direct access to GetHandle()? That seems the most fool-proof.
I’m not so sure — it’s just playing whack-a-mole with known cases. New macOS versions may change the behavior, some controls may be affected that nobody noticed yet, the API is evidently expected to be more dynamic (see alignmentRectForFrame: and frameForAlignmentRect:) etc...
I’d say that people who use wxNativeWindow or GetHandle() can be reasonably expected to call MacInvalidateLayoutInset() themselves. But it’s still a kludge — a workaround for what is ultimately an incorrect assumption that necessitates additional book-keeping inside wx.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
ryancog left a comment (wxWidgets/wxWidgets#26622)the API is evidently expected to be more dynamic
But it’s still a kludge — a workaround for what is ultimately an incorrect assumption that necessitates additional book-keeping inside wx.
Are you suggesting the caching should be removed entirely?
The fundamental problem, as I see it, is that wxWidgets is seemingly querying the insets too much. It’s measurably an expensive operation, and while I haven’t created an equal UI with native Cocoa in Objective-C to test, I can’t imagine Cocoa’s AutoLayout/creation is so slow.
When/how it should be queried (i.e. how AppKit uses it) is the part of interest that doesn’t seem to be well-documented.
Whack-a-mole isn’t ideal, but it’s the precedent in a lot of places in wx… the background handling in Tahoe comes immediately to mind.
I guess then, ultimately, is if/what to do instead of “playing whack-a-mole”. It doesn’t seem acceptable that window creation (and other things) are so slowed down by the inset query. Is there a better/reliable way to know where insets may change and be notified of that? Or is there a certain time where insets should be queried and reused over (I’m thinking, abstractly, a layout cycle or something like this)?
If not, it’s a question of which is worse, and IMO the potential layout breaking which would probably be obvious to catch and could be accounted for would be better than an overall substantial performance hit.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26622)It looks like we at least need a way to call InvalidateLayoutInset() from public API and preferably before 3.3.3. I think a PR adding wxWindow::OSXInvalidateLayoutCache() should be pretty simple to make, so if someone could please make one, it would be great.
It still seems crazy to me that querying insets is so slow, but I don't see what else (other than caching) can we do about it if it is.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Ryan Ogurek
ryancog left a comment (wxWidgets/wxWidgets#26622)@vadz put in a PR for that.
wx does change the bezel style itself, though. If that changes the insets then wx should handle the invalidation in that case.
wxOSXSetBezelStyleFromBorderFlags is the obvious place. The other places it happens (wxToolbar::DoInsertTool and -[wxMacFontPanelAccView initWithFrame:], and in osx/cocoa/renderer.mm where one of them is for text and is ignored by Cocoa anyways...) it seems to be immediately after creation, so that shouldn't cause issues, but it still seems safer to be put in a -[wxNSButton setBezelStyle:] as a catch all rather than in each place where it might be a problem. That'd have the benefit of "hiding" the kludge from application code in cases wx knows about it/has to handle itself anyways.
I can and add another PR unless there's a different way one of y'all think to do it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26622)put in a PR for that.
Thanks! @vslavik Could you please confirm that this is good enough for your needs?
wx does change the bezel style itself, though. If that changes the insets then wx should handle the invalidation in that case.
Yes, definitely. Please add the fixes for this to the same PR if this is more convenient.
Thanks again!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.