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. |
| } | ||
|
|
||
| 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.
Why is that breaking anything?
There was a problem hiding this comment.
The condition/model will most likely reference some local data.
There was a problem hiding this comment.
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.
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.
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.