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

Greying out pending nodes in the IDE #4088

Closed
wants to merge 7 commits into from

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jan 26, 2023

Pull Request Description

Adding node Kind::Pending and associating it with some grey like color schemes.

Important Notes

Some of us might have been hoping for a fix completely on the engine side, but it is already a month and the fix doesn't exist yet. It is not clear how much work is required to create such fix. Rather than hoping for it, I propose to use the Pending information which is already available in the IDE since #3729

I don't claim to be a visual expert. I am ready to adapt the visual presentation according to suggestions of the IDE guys.

Checklist

Please include the following checklist in your PR:

  • All code conforms to the
    Rust
    style guides.
  • All code has been tested:
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 26, 2023
@JaroslavTulach JaroslavTulach self-assigned this Jan 26, 2023
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Jan 26, 2023

Here is a video showing the greying out behavior. The source code of the example is available in pivotal.

@JaroslavTulach JaroslavTulach requested a review from Frizi January 26, 2023 15:55
Copy link
Contributor

@Frizi Frizi left a comment

Choose a reason for hiding this comment

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

From code point of view, all looks good. Visually I'm not sure if using the stripes here is right, as it calls for attention a lot. Maybe using a little less strong gray color would make it look better as is.

Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

This is amazing, however, can we have video attached to this PR showing thi in action and showing corner cases, to be sure this PR works as intended? :)

app/gui/src/presenter/graph/state.rs Outdated Show resolved Hide resolved
@JaroslavTulach
Copy link
Member Author

... can we have video attached to this PR showing this in action and showing corner cases, to be sure this PR works as intended? :)

I had some problems uploading the video and embedding it, but here is a link to mp4 file - I am able to successfully play it on Chrome (but not on Firefox) or download it and play it with vlc.

@JaroslavTulach
Copy link
Member Author

From code point of view, all looks good.

Thanks a lot Pawel for helping me navigate in the IDE codebase!

Maybe using a little less strong gray color would make it look better as is.

Anyone with more visual design skills than me (e.g. everyone), feel free to suggest changes or directly commit into this PR and improve colors & etc.

Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

Based on the video I see two big issues:

  1. The graph turns red when editing the node. It should not.
  2. When nodes are marked as not computed, the optional arguments are not displayed. We still know what these nodes refer to, so the arguments should be there.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Jan 29, 2023

The graph turns red when editing the node. It should not.

That behavior not related to my changes. If you think that is a bug, it needs to be addressed separately. And yes, I find the "turning red" behavior strange as well.

As discussed with Wojciech I've reported this problem in pivotal.

@JaroslavTulach
Copy link
Member Author

When nodes are marked as not computed, the optional arguments are not displayed. We still know what these nodes refer to, so the arguments should be there.

I'd like to fix that, but that is way deeper in the IDE code than I can dig into easily. Btw. the video associated with #4006 shows the same issue - IDE doesn't render optional arguments when node is in pending/errorneous/non-normal state.

@JaroslavTulach
Copy link
Member Author

At the end #4006 got integrated quickly and there is no need for this PR anymore. Thanks everyone who provided me guidance, reviews and pointed out visual flaws in my proposed implementation.

@farmaazon farmaazon deleted the wip/eng/GreyPendingNodes_184034702 branch May 9, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants