-
Notifications
You must be signed in to change notification settings - Fork 841
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
Auto dimensions #527
Auto dimensions #527
Conversation
def arrange( | ||
self, parent: Widget, size: Size, scroll: Offset | ||
) -> tuple[list[WidgetPlacement], set[Widget]]: | ||
def arrange(self, parent: Widget, size: Size) -> ArrangeResult: |
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.
Dropped scroll
parameter which wasn't used. Added a type alias for the return type.
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'm afraid the layout and geometry updates themselves are still a bit too obscure for me to do any relevant feedback at this point, so as often I was only able to leave some pretty picky-and-useless ones that's you're totally free to ignore, sorry 😅
for task in self._child_tasks: | ||
task.cancel() | ||
await task | ||
self._child_tasks.clear() | ||
if self._task is not None: |
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.
Could be worth doing this only if the task is not done yet? 🙂 i.e.
if self._task is not None and not self._task.done():
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.
Probably not much point, if the task is done, awaiting it is essentially a no-op.
@@ -59,7 +59,7 @@ def get_auto_height(container: Size, parent: Size) -> int: | |||
box_model = get_box_model( | |||
styles, Size(60, 20), Size(80, 24), get_auto_width, get_auto_height | |||
) | |||
assert box_model == BoxModel(Size(60, 20), Spacing(1, 2, 3, 4)) | |||
assert box_model == BoxModel(Size(54, 16), Spacing(1, 2, 3, 4)) |
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.
Just to be sure that I understand this correctly... The returned box model now shrinks the available space of a widget according to its margin, right? 🙂
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.
yep
except Exception: | ||
pass | ||
except Exception as error: | ||
self.on_exception(error) |
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.
There was a bunch of errors related to log getting called after the App has shutdown, which were previously hidden.
for task in self._child_tasks: | ||
task.cancel() | ||
await task | ||
self._child_tasks.clear() | ||
if self._task is not None: |
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.
Probably not much point, if the task is done, awaiting it is essentially a no-op.
@@ -59,7 +59,7 @@ def get_auto_height(container: Size, parent: Size) -> int: | |||
box_model = get_box_model( | |||
styles, Size(60, 20), Size(80, 24), get_auto_width, get_auto_height | |||
) | |||
assert box_model == BoxModel(Size(60, 20), Spacing(1, 2, 3, 4)) | |||
assert box_model == BoxModel(Size(54, 16), Spacing(1, 2, 3, 4)) |
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.
yep
There's a lack of tests here for the box model / layout system here. Please let that slide just now. I have a feeling there will be more changes required, and I don't yet have a good enough understanding of the various CSS rule interactions.