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.

Sign up for GitHub

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

Jump to bottom

Implement loading spinner for visualisations. #6512

Merged
merged 6 commits into from
May 12, 2023
Merged

Implement loading spinner for visualisations. #6512

merged 6 commits into from
May 12, 2023

Conversation

MichaelMauderer
Copy link
Contributor

@MichaelMauderer MichaelMauderer commented May 2, 2023 •
edited
Loading

Pull Request Description

Fixes #5088. Adds a ensoGL spinner for visualizations waiting on data.

Peek.2023-05-08.09-58.mp4

Important Notes

This spinner will not show up for the duration where visualizations are processing data on the frontend. If this is a concern, visualization need to implement heir own loading spinner, or we need to provide a unified API for them to keep the spinner visible.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@MichaelMauderer MichaelMauderer self-assigned this May 2, 2023
@MichaelMauderer MichaelMauderer marked this pull request as ready for review May 2, 2023 15:16
@wdanilo
Copy link
Member

wdanilo commented May 3, 2023 •
edited
Loading

  1. Can we do the spinner as a WebGL shape instead? Animation of it can be super simple - no FRP needed, as we have time variable in shaders (see text cursor for reference). We need these spinners in many places and putting everywhere HTML is a bad idea.
  2. Can we also make the spinner less visual intrusive? E.g. 3 dots side by side with changing alpha over time would be perfect - something like "typing" on discord.

@MichaelMauderer
Copy link
Contributor Author

MichaelMauderer commented May 4, 2023 •
edited
Loading

  1. Can we do the spinner as a WebGL shape instead? Animation of it can be super simple - no FRP needed, as we have time variable in shaders (see text cursor for reference). We need these spinners in many places and putting everywhere HTML is a bad idea.

There is a separate task for it: #5214
@vitvakatu @farmaazon, can we prioritize it?

Can we also make the spinner less visual intrusive? E.g. 3 dots side by side with changing alpha over time would be perfect - something like "typing" on discord.

This is the same spinner as for the documentation panel. Should that also be updated? @wdanilo

@vitvakatu
Copy link
Contributor

can we prioritize it?

@sylwiabr @xvcgreg

@MichaelMauderer do you have an estimate of how long would it take?

@MichaelMauderer
Copy link
Contributor Author

MichaelMauderer commented May 4, 2023 •
edited
Loading

@MichaelMauderer do you have an estimate of how long would it take?

I'd say one or two points.

@wdanilo
Copy link
Member

wdanilo commented May 4, 2023

Question – why do we estimate that to 1-2 points? I already created such a "spinner" for text in EnsoGL. When you are truncating text, the three dots after it are animating (we can re-use it). I implemented that in approx 30 minutes back then – I remember that I was on a call with Sylwia, she asked me about it and I implemented that before the call ended, couple months ago. I'm talking about the shape and its animation. If you are already placing the spinner in a correct position, then placement should not be a problem. Assuming that creation of shape is let's say 30-90 mins (let's assume 3x what it took me back then), why we have 1-2 days estimate here?

Anyway, the text spinner might be refactored and re-used. It's here: ensogl/component/text/src/component/line.rs, section "Ellipsis".

@MichaelMauderer
Copy link
Contributor Author

Question – why do we estimate that to 1-2 points? I already created such a "spinner" for text in EnsoGL. When you are truncating text, the three dots after it are animating (we can re-use it). I implemented that in approx 30 minutes back then – I remember that I was on a call with Sylwia, she asked me about it and I implemented that before the call ended, couple months ago. I'm talking about the shape and its animation. If you are already placing the spinner in a correct position, then placement should not be a problem. Assuming that creation of shape is let's say 30-90 mins (let's assume 3x what it took me back then), why we have 1-2 days estimate here?

Anyway, the text spinner might be refactored and re-used. It's here: ensogl/component/text/src/component/line.rs, section "Ellipsis".

It's always easier to estimate after the fact :)

I've added a new EnsoGL based spinner, but it is not fixing #5214. That will require some more work, as there is an issue with the layer ordering. It will need some additional work as right now, the docs panel is in HTML and overlays all EnsoGL objects, so the spinner is never visible.

vitvakatu
vitvakatu reviewed May 8, 2023
View reviewed changes
app/gui/view/graph-editor/src/component/visualization/container.rs Outdated
@@ -174,18 +176,39 @@ impl View {
let display_object = display::object::Instance::new();
let background = background::View::new();
let overlay = overlay::View::new();
display_object.add_child(&background);
// display_object.add_child(&background);
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, btw I also touched this line one time. I was like "why the heck do we need it?". And I still don't understand why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is some cruft in here. We once had an EnsoGL background, but that did not work well with HTML/.JS visualizations, as the layering was problematic. So, we added an HTML background, too. But that was problematic with EnsoGL visualizations (of which we have none currently, I think). We should sort this out at some point.

@farmaazon
Copy link
Contributor

QA: 🟢

@MichaelMauderer MichaelMauderer added the CI: Ready to merge This PR is eligible for automatic merge label May 11, 2023
@mergify mergify bot merged commit 9e71fea into develop May 12, 2023
24 of 25 checks passed
@mergify mergify bot deleted the wip/MichaelMauderer/Vis_Spinner branch May 12, 2023 04:41
Procrat added a commit that referenced this pull request May 12, 2023
@Procrat
Merge branch 'develop' into wip/procrat/home-button-6399
5a86e4a 
* develop:
  Implement loading spinner for visualisations. (#6512)
  Fix blank input port (#6614)
  Add `Date_Range` (#6621)
  All Vector operations shall be applicable on java.util.ArrayList (#6642)
  Fix redirect paths and enable authentication and new dashboard by default (#6605)
  Fix #6287: wrong nested breadcrumb ordering (#6617)
  Whitelist AWS Cognito domains (#6643)
  Revert "Add COOP+COEP+CORP headers (#6597)" (#6647)
  Fix shortcuts table formatting (#6644)
  Automatic type based dropdown does not include singleton in a union type (#6629)
  Make Meta.get_annotation work for constructor (#6633)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitvakatu vitvakatu vitvakatu left review comments

@wdanilo wdanilo wdanilo approved these changes

@farmaazon farmaazon Awaiting requested review from farmaazon farmaazon is a code owner

@kazcw kazcw Awaiting requested review from kazcw kazcw is a code owner

Assignees

@MichaelMauderer MichaelMauderer

Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

User should see a spinner in visualization window while waiting for visualization content
4 participants
@MichaelMauderer @wdanilo @vitvakatu @farmaazon