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

compiler: Fix bug in snapshotting #5518

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hunger
Copy link
Member

@hunger hunger commented Jun 30, 2024

Do not take an ElementRc to move it into the Component's root_element.

@ogoffart spotted this! Thanks!

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

I think there is still a problem here that snapshot_element calls snapshot_component again for the enclosing component

I think you'll have to either

  1. use Rc::new_cyclic, and pass the enclosing component in snapshot_element, or
  2. keep the enclosing element empty, and set it later once the component is complete.

I think i'd go with option 1

@hunger
Copy link
Member Author

hunger commented Jul 2, 2024

I tried the new_cyclic approach. I have the problem that some of the elements are first discovered elsewhere than via the component they belong to (I had actually not expected that!). So I see elements created as ElementRcs... which is a problem if one of those is later discovered to be root element.

I have been playing with separating having a create method for components/elements (just creates a basically empty object (and asserts if the object exists already), trying to create elements/components referenced in the original only), as well as a use method (asserts if some component/element is used that has not been created yet) and a final snapshot step that copies the rest of the data (properties and such that use elements and such).

I get lots of asserts triggered with that approach :-/

@hunger
Copy link
Member Author

hunger commented Jul 3, 2024

I think it works now. After way too many stupid bugs and some surprising behavior in how and when elements are created...

@hunger
Copy link
Member Author

hunger commented Jul 4, 2024

/me sighs. Left in a variable I had added to debug things.

Do not `take` an ElementRc to move it into the `Component`'s `root_element`.

@ogoffart spotted this! Thanks!
@hunger
Copy link
Member Author

hunger commented Jul 5, 2024

I used the stable order of struct fields (#5561) to debug-print the components and elements I snapshot and to compare that output to the same from the original components and elements. Nothing asserts anymore, they all produce the same debug strings.

That's a step into the right direction... BUT I see the base_type of some elements as <error> -- in both the source and the target! I m poking at that problem now.

if component.id == "TextInputInterface" ||
component.id == "NativePalette" ||
component.id == "NativeStyleMetrics" {
component.clone()
Copy link
Member Author

Choose a reason for hiding this comment

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

I Hate this part, hard-coding some "special" components!

@hunger
Copy link
Member Author

hunger commented Jul 5, 2024

Funny... I find elements with a base_type of Error in the master branch as well when taking a snapshot there. They do show up in the property editor with the expected types though! I do the Error in the Property editors type display as well when using this PR :-(

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.

None yet

2 participants