Small visualization fixes #9130

vitvakatu · 2024-02-21T17:59:46Z

Pull Request Description

Closes #9009

Fixed big white space above full screen viz.
Escape closes the full screen visualization.
Viz shortcuts (Shift-Space for toggling fullscreen vis, Ctrl-Space for switching vis type) are implemented.
The width of visualizations is preserved across project reopens (do we need height as well?)

New video:

visualization.shortctus.mp4

Older videos:

visualization.fixes.mp4

preserving.vis.width.mp4

Important Notes

Metadata format changed in backward-compatible way

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.

app/gui2/src/components/GraphEditor.vue

farmaazon · 2024-02-22T14:37:14Z

app/gui2/src/components/GraphEditor/GraphVisualization.vue

+let width = ref<Opt<number>>(props.width)
 let height = ref(150)
+// We want to debounce width changes, because they are saved to the metadata.
+const debouncedWidth = debouncedGetter(() => width.value, 300)
+watch(debouncedWidth, (value) => value != null && emit('update:width', value))


I think it's not quite working properly. We have prop.width, width and debouncedWith - the prop.width will be updated only on debounce, but even then width is not updated (ref is only initialized).

I think the getter below should return props.width, and we could have a debounce version for events.

I think it works fine. props.width is the “persistent” width of the visualization, the one we save and load from the config. width is a current width, it might not be equal to “props.width” because of debouncing, but they will sync eventually. When width is updated (user resizes the visualization), we sync props.width after timeout and everything is normal again.

Ah, I could not find where do we set width, but it's in unchanged code.

farmaazon · 2024-02-22T14:41:12Z

app/gui2/src/components/VisualizationContainer.vue

@@ -49,7 +56,7 @@ function blur(event: Event) {
 const rootNode = ref<HTMLElement>()
 const contentNode = ref<HTMLElement>()

-onMounted(() => (config.width = Math.max(config.nodeSize.x, MIN_WIDTH_PX)))
+onMounted(() => (config.width = Math.max(config.width ?? config.nodeSize.x, MIN_WIDTH_PX)))


This is also a bit of strange mechanic: we update width in config (effectively updating it also in code's metadata if I read code properly) on mount, i.e. when showing visualization, possibly with the width the node happen to have ATM (it may depend even on the project loading stage). I think it's better when the metadata keeps the width set by user by resizing.

The purpose of this code is a bit different:
If we have with from metadata (config.width), then

Set current width to at least MIN_WIDTH_PX
Else

Set current width and metadata width to current node size, or MIN_WIDTH_PX, whichever is larger.

Without this change, we won’t load the width initially, we would always use config.nodeSize.x.

app/gui2/src/components/VisualizationContainer.vue

vitvakatu · 2024-02-27T14:58:52Z

Huge update, refactoring of a lot of code places. Shortcuts and focusing is now working almost ideally. Added new screencast to the description. Now visualizations are focusable, and you can trigger Shift+Space, Cmd+Space shortcuts in two modes:

Visualization is focused (you clicked on it)
Node is selected and highlighted, and it is the only selected node on the scene

Escape also works for closing visualizations. Code editor and other graph editor shortcuts also work.

The one remaining issue is the behavior of Graph Editor which effectively ignores any shortcuts if any HTMLElement is focused at the moment. As visualizations can now be focused by mouse-clicking, it can give the user some confusion.

See this code in graph editor:

useEvent(window, 'keydown', (event) => {
  ;(!keyboardBusy() && (interactionBindingsHandler(event) || graphBindingsHandler(event))) ||
    (!keyboardBusyExceptIn(codeEditorArea.value) && codeEditorHandler(event))
})

keyboardBusy() here checks if any HTMLElement is focused, and effectively disables shortcuts in such a case.

This possible change touches a lot of moving parts of the application, and I’m not comfortable enough doing so, especially because this code was recently touched to avoid issues with code editor.

farmaazon

I added some suggestions for improvements. But let's make them in a follow-up PR, as this one is waiting too long.

farmaazon · 2024-02-29T12:52:50Z

app/gui2/src/bindings.ts

@@ -25,6 +25,7 @@ export const graphBindings = defineKeybinds('graph-editor', {
  openComponentBrowser: ['Enter'],
  newNode: ['N'],


We should remove this binding.

farmaazon · 2024-02-29T13:02:29Z

app/gui2/src/bindings.ts

+  toggleFullscreen: ['Shift+Space'],
+  exitFullscreen: ['Escape'],
+})
+


Instead of duplicating bindings, I would make GraphEditor implement all handlers for visualizationBindings in similar way as toggleFullscreen is implemented (check for single selected node).

farmaazon · 2024-02-29T13:04:45Z

app/gui2/src/components/GraphEditor/GraphVisualization.vue

+})
+
+
+useEvent(window, 'keydown', (event) => keydownHandler(event))


If GraphEditor handles nextType event, then there's no need for every visualization listen at window level. Instead we could attach handler to it's div (but then let's make sure visualization is focused after going fullscreen).

Addressing review suggestions from #9130 - Removing `N` binding - Removing duplicated `Toggle fullscreen vis` binding A few important differences from the suggested implementation: 1. There is no easy way to implement `nextType` on GraphEditor – we simply don’t have the required API 2. The keydown handler in `GraphVisualization` must be defined on window level still, otherwise it won’t get keydown events unless visualization is focused, and thus `nextType` won’t work because of (1) No visual changes to the IDE.

vitvakatu added 2 commits February 21, 2024 21:53

visualization fixes

65d4c29

Merge branch 'develop' into wip/vitvakatu/visualization-fixes

a39faaa

vitvakatu added the -gui label Feb 21, 2024

vitvakatu self-assigned this Feb 21, 2024

vitvakatu added 4 commits February 21, 2024 22:15

refactoring

8b364f1

Cleanups

e39a0a1

cleanups

f3b9434

Alternative event handling

6c25d33

vitvakatu marked this pull request as ready for review February 22, 2024 13:26

vitvakatu requested review from Frizi, farmaazon, MichaelMauderer and kazcw as code owners February 22, 2024 13:26

farmaazon requested changes Feb 22, 2024

View reviewed changes

vitvakatu added 4 commits February 23, 2024 15:54

Merge branch 'develop' into wip/vitvakatu/visualization-fixes

cdd87af

intermediate working state

4ca75ea

Some cleanups

3dcbcec

Merge branch 'develop' into wip/vitvakatu/visualization-fixes

906994e

vitvakatu marked this pull request as draft February 27, 2024 14:26

More cleanups

7cafe2e

vitvakatu marked this pull request as ready for review February 27, 2024 14:58

vitvakatu requested a review from farmaazon February 27, 2024 14:59

farmaazon approved these changes Feb 29, 2024

View reviewed changes

vitvakatu added 3 commits February 29, 2024 17:50

Merge branch 'develop' into wip/vitvakatu/visualization-fixes

7a89092

lints

f3e408f

Fix table vis test

af4233f

vitvakatu added CI: Ready to merge This PR is eligible for automatic merge CI: No changelog needed Do not require a changelog entry for this PR. labels Feb 29, 2024

mergify bot merged commit c44b7f2 into develop Feb 29, 2024
26 of 30 checks passed

mergify bot deleted the wip/vitvakatu/visualization-fixes branch February 29, 2024 16:37

vitvakatu mentioned this pull request Mar 3, 2024

Visualizations fixes follow-up #9252

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small visualization fixes #9130

Small visualization fixes #9130

vitvakatu commented Feb 21, 2024 •

edited

Loading

farmaazon Feb 22, 2024 •

edited

Loading

vitvakatu Feb 27, 2024

farmaazon Feb 29, 2024

farmaazon Feb 22, 2024 •

edited

Loading

vitvakatu Feb 27, 2024

vitvakatu commented Feb 27, 2024 •

edited

Loading

farmaazon left a comment

farmaazon Feb 29, 2024

farmaazon Feb 29, 2024

farmaazon Feb 29, 2024

		@@ -25,6 +25,7 @@ export const graphBindings = defineKeybinds('graph-editor', {
		openComponentBrowser: ['Enter'],
		newNode: ['N'],

		})


		useEvent(window, 'keydown', (event) => keydownHandler(event))

Small visualization fixes #9130

Small visualization fixes #9130

Conversation

vitvakatu commented Feb 21, 2024 • edited Loading

Pull Request Description

Important Notes

Checklist

farmaazon Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

vitvakatu Feb 27, 2024

Choose a reason for hiding this comment

farmaazon Feb 29, 2024

Choose a reason for hiding this comment

farmaazon Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

vitvakatu Feb 27, 2024

Choose a reason for hiding this comment

vitvakatu commented Feb 27, 2024 • edited Loading

farmaazon left a comment

Choose a reason for hiding this comment

farmaazon Feb 29, 2024

Choose a reason for hiding this comment

farmaazon Feb 29, 2024

Choose a reason for hiding this comment

farmaazon Feb 29, 2024

Choose a reason for hiding this comment

vitvakatu commented Feb 21, 2024 •

edited

Loading

farmaazon Feb 22, 2024 •

edited

Loading

farmaazon Feb 22, 2024 •

edited

Loading

vitvakatu commented Feb 27, 2024 •

edited

Loading