Correct scaling of MIN_WIDTH and MIN_HEIGHT on Android and WinForms #2186
Conversation
There was a problem hiding this comment.
I've got a bunch of questions about this, mostly related to consistency and enforcing minimum sizes (usually width). Comments are inline; however, I'm going to merge on the basis that it definitely corrects some of the issues I've been seeing with widget sizing.
The issues I've raised can all be safely fixed in a follow up PR, if necessary.
| intrinsic.width = intrinsic.height = None | ||
| # Default values; may be overwritten by rehint(). | ||
| self.interface.intrinsic.width = at_least(self.interface._MIN_WIDTH) | ||
| self.interface.intrinsic.height = at_least(self.interface._MIN_HEIGHT) |
There was a problem hiding this comment.
Should these default values be scaled out?
There was a problem hiding this comment.
No, because they're already in CSS pixels. The intention of this PR is that when sizes are assigned to intrinsic.width or intrinsic.height, they are already in CSS pixels as the interface layer expects, so they don't need to be scaled again.
| @@ -42,4 +43,6 @@ def create(self): | |||
| def rehint(self): | |||
| self.interface.intrinsic.width = at_least(300) | |||
There was a problem hiding this comment.
Should this constant value be scaled (and/or expressed in terms that are suitable for scaling?)
There was a problem hiding this comment.
No, for the same reason as above. And since date and time pickers look quite different on each platform, I think it makes sense for the backend to have its own value in this case, rather than defining a _MIN_WIDTH that they all share.
| self.interface.intrinsic.width = at_least(self.native.getMeasuredWidth()) | ||
| self.interface.intrinsic.height = self.native.getMeasuredHeight() | ||
| self.interface.intrinsic.width = self.scale_out( | ||
| at_least(self.native.getMeasuredWidth()), ROUND_UP |
There was a problem hiding this comment.
Should this have a MIN_WIDTH component to ensure it doesn't just collapse to the smallest possible rendered size?
There was a problem hiding this comment.
Yes, that would probably be better, and more consistent with some of the other backends. I didn't do that in this PR because I wanted to limit it to just one problem at a time.
|
Continued in #2200. |
The minimum sizes were being converted to CSS pixels even though that's what they already were. This caused them to be too small.
Also took the opportunity to reduce duplicated code by assigning default
intrinsic.widthandintrinsic.heightvalues in the base class.