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

Only initialise visualisation chooser if it is used. #6758

Merged

Conversation

MichaelMauderer
Copy link
Contributor

@MichaelMauderer MichaelMauderer commented May 18, 2023

Pull Request Description

Re-introduce a feature that was removed with #6638: only initialize visualization choosers when they are visible. This avoids initializing lots of invisible UI elements at the same time when opening a project.

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 18, 2023
@MichaelMauderer MichaelMauderer marked this pull request as ready for review May 18, 2023 10:49
@MichaelMauderer MichaelMauderer added the CI: No changelog needed Do not require a changelog entry for this PR. label May 18, 2023
…r/action_bar.rs

Co-authored-by: Adam Obuchowicz <adam.obuchowicz@enso.org>
@Procrat
Copy link
Contributor

Procrat commented May 24, 2023

QA: I think there might be a small regression here. When I hover over the visualisation of an existing node in a newly opened project, it reverts back to the loading animation. See 0:13 in this video:

2023-05-24.11-09-57.online-video-cutter.com.mp4

@MichaelMauderer
Copy link
Contributor Author

QA: I think there might be a small regression here. When I hover over the visualisation of an existing node in a newly opened project, it reverts back to the loading animation.

This is now fixed. @Procrat

Comment on lines 415 to 418

// Note: we only want to update the chooser if it is visible, or when it becomes
// visible. Thus we avoid many simultaneous updates during initialisation.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this comment. What do you mean by "many simultaneous updates during initialization"? Why updating the chosen while it is invisible would cause many simultaneous updates?

Copy link
Contributor

Choose a reason for hiding this comment

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

We update all choosers upon first receiving types from the engine (the initialization), which is mostly pointless given they are not visible (and they content may easily be changed again before it will be actually shown).

Copy link
Member

Choose a reason for hiding this comment

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

Ok, @MichaelMauderer, would you be able to add this (^^^) information to the comment there as well, please? It would make (at least for me) it way more understandable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@wdanilo
Copy link
Member

wdanilo commented May 26, 2023

By "This avoids initializing lots of invisible UI elements at the same time when opening a project." - do you mean that currently multiple visualizations are initialized even if they should not?

@Procrat
Copy link
Contributor

Procrat commented May 26, 2023

QA: I think there might be a small regression here. When I hover over the visualisation of an existing node in a newly opened project, it reverts back to the loading animation.

This is now fixed. @Procrat

QA: Sweet! Looks good to me!

@MichaelMauderer MichaelMauderer added the CI: Ready to merge This PR is eligible for automatic merge label May 26, 2023
@mergify mergify bot merged commit 0dcab3d into develop May 26, 2023
23 of 24 checks passed
@mergify mergify bot deleted the wip/MichaelMauderer/Delay_Initialisation_Of_Hidden_Action_Bars branch May 26, 2023 15:17
Procrat added a commit that referenced this pull request May 30, 2023
…le-6756-6804

* develop: (22 commits)
  Coalesce graph editor view invalidations (#6786)
  Append warnings extracted before tail call execution (#6849)
  Detect and override hooks of the same kind (#6842)
  Dynamic app resampling and better performance measurements. (#6595)
  Show spinner when opening/creating a project, take #2 (#6827)
  Infrastructure for testing inter project imports and exports (#6840)
  Only initialise visualisation chooser if it is used. (#6758)
  Allow casting a Mixed column into a concrete type (#6777)
  Stop graph editing when in full-screen visualization mode (#6844)
  Handle `show-dashboard` event (#6837)
  Fix some dashboard issues (#6668)
  Fix JWT leak (#6815)
  Fix "set username" screen (#6824)
  Fallback to opened date when ordering projects (#6814)
  Various test improvements to increase coverage and speed things up (#6820)
  do not activate nested dropdowns together (#6830)
  Clearly select single specialization with enum dispatch pattern (#6819)
  Prevent incorrect application of list widget on incompatible expressions (#6771)
  Update GraalVM to 22.3.1 JDK17 (#6750)
  Import/export syntax error have more specific messages (#6808)
  ...
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. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants