-
Notifications
You must be signed in to change notification settings - Fork 34
Conversation
|
||
|
||
// === VCS Handling === | ||
model.vcs_indicator.frp.set_status <+ frp.set_vcs_status.unwrap(); |
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 unwrap? I mean, this prevents us from disabling the vcs status there, 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.
Oh I get it, we show and hide the status in next line. I think we should redesign this api towards handling the visibility of the status by the status indicator. So, I'd refactor all these 3 FRP lines to:
model.vcs_indicator.frp.set_status <- frp.set_vcs_status.unwrap();
and handle the visibility inside of vcs_indicator instead
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 idea here is that there is a difference between "has no status" and "has a status: it was unchanged". Right now we can easily add a new status for unchanged
without much effort. If we change this as proposed, and we decide we want to show some notification for unchanged
we need to do much more refactoring.
Also, the point of the logic here is to completely remove the status indicator from the component tree if it is not needed. So instead of having some invisible shape, we remove all the shapes completely, which logically needs to be done by the node to get rid of the whole VCS component if it is not needed.
/// The node has not been modified. | ||
Unchanged, |
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 don't like this design as it repeats the ideas of Option instead of reusing it. Instead I' create:
pub enum Change { Added, Edited };
pub type Status = Option<Change>;
Also, could you please ake a screenshot with both vcs status and node selection visible? |
@wdanilo Still waiting for manual testing. |
# Conflicts: # CHANGELOG.md # src/rust/ide/view/graph-editor/src/component/node.rs # src/rust/ide/view/src/debug_scenes/interface.rs
Updated to develop. Waiting for testing. |
Pull Request Description
Add API and simple visuals to set node VCS status.
Peek.2021-02-02.13-41.mp4
Yellow: node edited
Green: node added.
There is also a demo in the
interface
debug scene.Checklist
Please include the following checklist in your PR: