-
Notifications
You must be signed in to change notification settings - Fork 497
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
live-preview: Checks element to decide whether it can be moved #5204
Conversation
IMHO this is way too strict. And yet this is far from sufficient to prevent all errors. So I think this is way too restrictive. |
So it is too restrictive, yet you only list more things I can not allow? I am getting mixed signals :-) |
…n be moved Be way more conservative with when an element can be moved around. Look at eleemnt properties, element id, etc. Start out simple that errs on the side of caution.
The latest version allows any move that does not re-parent the element. |
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.
You also should prevent deleting element that can be referenced.
But I still think this cure is worse than the disease.
} | ||
|
||
let is_simple_element = selected_element.with_decorated_node(|node| match node.kind() { | ||
SyntaxKind::ConditionalElement | SyntaxKind::RepeatedElement => false, |
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.
Why is that breaking anything?
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.
The condition/model will most likely reference some local data.
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 will need to parse the condition/model/etc. properly to figure that out. I did not want to do that before the release though.
let se: syntax_nodes::SubElement = node.clone().into(); | ||
se.first_token().map(|t| t.kind()).unwrap_or(SyntaxKind::Error) | ||
== SyntaxKind::Identifier |
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 believe this can be written as
let se: syntax_nodes::SubElement = node.clone().into(); | |
se.first_token().map(|t| t.kind()).unwrap_or(SyntaxKind::Error) | |
== SyntaxKind::Identifier | |
node.child_token(SyntaxKind::Identifier).is_some() |
if drop_target.children().contains(selected_element) { | ||
return true; | ||
} |
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.
Basically, what you can check, is if they are in the same scope instead of the same parent. Either from the nodes: try to find the first ancestor which is either a Component, or a ConditionalElement; or a RepeatedElement, and they must be the same for both node.
Or easier from the ElementRc if they are in the same item tree (The enclosing component of the ElementRc)
My point is tat this is too restrictive and prevent the d&d feature to be useful, while at the same time not doing what it's supposed to do: prevent any operation to cause errors. |
Yeap, I know:-/ The only reliable way to detect that moving a element is not going to cause a compile error is to do the change and compile the changed code. Everything else is going to be flawed in some major way. The compiler is unfortunately too slow to run on every move() event... Do you think we can speed it up for "minor changes" like moving some element somehow? Or maybe have some "check mode" in the compiler that skips most of the passes and the actual code gen? That would be similar to what the LSP does already, maybe I can reuse the logic from there? It would also require changes to how we do things right now, but the preview has all the code, so we could probably drive the compiler somehow. That could be a step towards not needing an editor to do the actual edits for us, too. |
I'll try to compile-test the change now. If that is too slow, we can come back to this and discuss better heuristics. |
Be way more conservative with when an element can be moved around. Look at eleemnt properties, element id, etc.
Start out simple that errs on the side of caution.
This is following up on what Olivier suggested during the coffee break: To disallow dropping onto something, not to forbid drag and drop altogether.
Note that this is very restrictive and basically only keeps really simple objects movable for now. We can lift the limits as we figure out ways to do so.