Fix bugs in popup #10573
Merged
Fix bugs in popup #10573
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pascalkuthe
added
C-bug
the-mikedavis
previously approved these changes
Apr 25, 2024
|
(I will rebase later to absorb that commit sp we don't have to Squash merge) |
Co-authored-by: ath3 <[email protected]>
Trunctation should always be handled by the parent. Returning None is only supposed to indicate a missing implementation Co-authored-by: Ben Fekih, Hichem" <[email protected]>
ath3
suggested changes
Apr 26, 2024
ath3
suggested changes
Apr 26, 2024
Co-authored-by: Ben Fekih, Hichem" <[email protected]>
ath3
approved these changes
Apr 26, 2024
dead10ck
approved these changes
Apr 27, 2024
dead10ck
reviewed
Apr 27, 2024
Comment on lines
-158
to
-160
| if PADDING >= viewport.1 || PADDING >= viewport.0 { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Was this the bug? Returning None instead of a default size?
There was a problem hiding this comment.
Yeah this was an existing bug that caused crashes. required_size is only meant to always return None (for components that can't be embedded in a popup) or always meant to return Some. The API is a bit confusing and was very inconsistently used so this seems to have slipped trough at some point.
the-mikedavis
approved these changes
Apr 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #10569
this fixes regression introduced by #10257 both regarding general crashes and bugs when popup borders are used (#10566/#10553). While working on this I noticed there were actually existing bugs where the border size wasn't quite taken into account while scrolling which I also fixed. Since borders now need to be accounted for in the popup I made the border config and entirely internal config so its moore self contained.
Also incorporated the fixes from #10507 with some slight adjustments.
Marked for the next release since this fixes newly introduced crashes that are fairly easy to trigger