Skip to content
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

Introduce ChangeFlags and use it instead of bools to track changes in views #6

Merged
merged 3 commits into from
Dec 1, 2022

Conversation

Zoxc
Copy link
Collaborator

@Zoxc Zoxc commented Nov 23, 2022

This approach has a number of advantages.

  • It makes it clear that set_label and set_text do not themselves set the flags and that these functions are the source of these flags.
  • The type is marked as #[must_use] so flags are not accidentally missed, which I have done previously when writing views.
  • It seems to make the update phase unnecessary.

ChangeFlags or Changes could be some alternative names. The current name could easily be confused with the update phase.

@Zoxc Zoxc changed the title Introduce UpdateFlags and use it instead of bools to track changes views Introduce UpdateFlags and use it instead of bools to track changes to views Nov 23, 2022
@Zoxc Zoxc changed the title Introduce UpdateFlags and use it instead of bools to track changes to views Introduce UpdateFlags and use it instead of bools to track changes in views Nov 23, 2022
@raphlinus
Copy link
Contributor

Thanks for this! I'm thinking it through, but while I'm doing that let me explain a bit why it is the way it is, and a potential downside.

My initial concept of the View trait was to be very generic and generally agnostic about the details of the Element tree implementation that sits under it. So this change feels like a (minor) layering violation, that a detail of the widget tree is now present in the View trait. However, the View trait has now evolved to be less generic and has an explicit Widget bound on Element, so that's not actually a layering violation.

That said, this change puts a (small) additional burden on other potential consumers of the widget tree, they have to set these flags correctly. Not a showstopper, but I think something to be considered.

The real question for me is whether we can make the update method go away entirely. I don't much like the current state where all it does is set a flag, it feels like boilerplate. But I haven't thought through the implications of that.

I like ChangeFlags as the name.

@Zoxc
Copy link
Collaborator Author

Zoxc commented Nov 23, 2022

I view the current state as having a 1-bit flag, with this PR extending it to n-bit flags. So you could consider the current View trait to assume that the Widget trait uses a 1-bit flag, which gets changed to n-bit flags, both being "layering violations". Code in widget consumers using a 1-bit flag should be identical to code using n-bit flags, so I'm not sure where the additional burden comes from. We're changing the width of the type, not the structure of the code itself.

The real question for me is whether we can make the update method go away entirely. I don't much like the current state where all it does is set a flag, it feels like boilerplate. But I haven't thought through the implications of that.

I would follow up this PR with the removal of the update method, unless we find a reason for it to exist.

@raphlinus raphlinus mentioned this pull request Nov 23, 2022
Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's go ahead and do this. We can back it out later if it turns out to be a problem. Thanks!

Regarding update, that decision is tracked in #7, as the Widget trait is going to get massively reworked anyway. It might make sense to apply it now so we can experiment with the method missing, but what lands now in Widget won't be staying that way for long.

@Zoxc Zoxc changed the title Introduce UpdateFlags and use it instead of bools to track changes in views Introduce ChangeFlags and use it instead of bools to track changes in views Nov 24, 2022
@raphlinus
Copy link
Contributor

Ah, there are merge conflicts now. This is approved, so feel free to resolve those and merge it.

@Zoxc Zoxc merged commit 3d99fdf into linebender:main Dec 1, 2022
github-merge-queue bot pushed a commit that referenced this pull request Nov 11, 2024
New tracy image:


![image](https://github.com/user-attachments/assets/94e54c89-8159-4dd4-a521-4a7122f64375)

New log tracing file:
<details><summary>An overview of the new logs</summary>
<p>

```
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}: masonry::passes::update: RootWidget received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}:SizedBox{id=#7}: masonry::passes::update: SizedBox received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}:SizedBox{id=#7}:Flex{id=#6}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}:SizedBox{id=#7}:Flex{id=#6}:Flex{id=#3}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}:SizedBox{id=#7}:Flex{id=#6}:Flex{id=#3}:Prose{id=#1}: masonry::passes::update: Prose received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}:SizedBox{id=#7}:Flex{id=#6}:Flex{id=#3}:Label{id=#2}: masonry::passes::update: Label received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}:SizedBox{id=#7}:Flex{id=#6}:Flex{id=#5}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}:SizedBox{id=#7}:Flex{id=#6}:Flex{id=#5}:VariableLabel{id=#4}: masonry::passes::update: VariableLabel received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#10}: masonry::passes::update: Button received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#10}:Label{id=#9}: masonry::passes::update: Label received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#12}: masonry::passes::update: Button received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#12}:Label{id=#11}: masonry::passes::update: Label received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#14}: masonry::passes::update: Button received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#14}:Label{id=#13}: masonry::passes::update: Label received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#16}: masonry::passes::update: Button received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#16}:Label{id=#15}: masonry::passes::update: Label received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}: masonry::passes::update: Portal received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#24}: masonry::passes::update: SizedBox received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#24}:Flex{id=#23}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#24}:Flex{id=#23}:Flex{id=#20}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#24}:Flex{id=#23}:Flex{id=#20}:Prose{id=#18}: masonry::passes::update: Prose received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#24}:Flex{id=#23}:Flex{id=#20}:Label{id=#19}: masonry::passes::update: Label received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#24}:Flex{id=#23}:Flex{id=#22}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#24}:Flex{id=#23}:Flex{id=#22}:VariableLabel{id=#21}: masonry::passes::update: VariableLabel received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#31}: masonry::passes::update: SizedBox received Update::WidgetAdded
```

</p>
</details> 

This was originally an experiment into caching spans, but I determined
that was non-viable due to the pass names.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants