Fix visualizations sometimes not opening with space bar. #5624

MichaelMauderer · 2023-02-10T15:33:07Z

Pull Request Description

Visualizations closing right after opening was caused by the GUI being unresponsive during loading of some visualizations. This caused the timer for measuring the time between space bar press and space bar release to be inflated. The delayed events triggered the "visualization preview mode”, thus closing the visualization has it seemed that the space bar was held down, even though the events just arrived with some delay.

The problem is mitigated by considering the number of frames that have passed between the space and down and the space bar up event, instead of just the wall clock time. If the number of frames is too low, this indicates that frames were dropped to the time is inflated.

Fixes #5223

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run ide build and ./run ide watch.

…tion opening do not erroneously close the visualisation as if it was opened as preview.

kazcw · 2023-02-10T15:56:46Z

lib/rust/web/src/lib.rs

+        let frames: Counter = default();
+        let frames_handle = Rc::clone(&frames);
+        let closure_handle = Rc::new(RefCell::new(None));
+        let closure_handle_internal = Rc::clone(&closure_handle);


Is there a memory leak here? The closure strongly-references the closure handle, which owns the closure.

kazcw · 2023-02-10T15:56:55Z

lib/rust/web/src/lib.rs

+type Counter = Rc<std::cell::Cell<i32>>;
+
+#[derive(Debug)]
+/// A counter that counts frames thr frames that have passed since its initialization.


farmaazon

Please address the leak issue by @kazcw

farmaazon · 2023-02-13T09:56:09Z

app/gui/view/graph-editor/src/lib.rs

+/// Number of frames we expect to pass during the
+/// `VIZ_PREVIEW_MODE_TOGGLE_TIME_MS` interval. Assumes 60fps.  
+const VIZ_PREVIEW_MODE_TOGGLE_FRAMES: i32 = (VIZ_PREVIEW_MODE_TOGGLE_TIME_MS / 16.0) as i32;


If we assume 60 FPS I would rather put here 1000.0/60.0. Or make assumed FPS another constant

…with_space_does_not_always_work_#5223

wdanilo

I need a better explanation of why this PR works and what was the cause that visualizations were disappearing. The code is probably fine, but it should explain itself for future readers

wdanilo · 2023-02-13T14:57:23Z

app/gui/view/graph-editor/src/lib.rs

@@ -94,6 +94,10 @@ pub mod prelude {

 const SNAP_DISTANCE_THRESHOLD: f32 = 10.0;
 const VIZ_PREVIEW_MODE_TOGGLE_TIME_MS: f32 = 300.0;
+/// Number of frames we expect to pass during the
+/// `VIZ_PREVIEW_MODE_TOGGLE_TIME_MS` interval. Assumes 60fps.  


This lacks documentation of what these constants do. Please add docs to previous constant as well. Also, the explanation Number of frames we expect to pass during the... doesn't really explain what it means and what it is used for.

wdanilo · 2023-02-13T14:58:50Z

app/gui/view/graph-editor/src/lib.rs

+        // We also check the number of passed frames to ensure we did not spend too much time in
+        // dropped frames during the visualisation initialisation.


What does it mean? What is "too much time"? There is no explanation why this code exists and what it really does.

MichaelMauderer · 2023-02-13T16:19:00Z

I need a better explanation of why this PR works and what was the cause that visualizations were disappearing. The code is probably fine, but it should explain itself for future readers

The problem here is, that we are trying to measure the time between the key down and the key up event. If the key up event happens before the VIZ_PREVIEW_MODE_TOGGLE_TIME_MS has passed, we consider it a normal key press that opens the visualization. If the key up event happens after VIZ_PREVIEW_MODE_TOGGLE_TIME_MS has passed, we assume that a "press and hold” action was performed, which would trigger our visualization preview and thus the visualization should be closed when the key is release. But if the opening of the visualization takes a lot of time, specifically, longer than VIZ_PREVIEW_MODE_TOGGLE_TIME_MS, then the key up event will be processed too late, even if the key was already released right after the key down. We can guard against this by not just measuring the time, but checking that enough frames have passed. Since if we take too much time processing things, we get dropped frames.

I'll now update the docs to make this clearer.

…with_space_does_not_always_work_#5223

wdanilo

Perfect, now it is fully understandable, thanks!

MichaelMauderer · 2023-02-15T11:44:55Z

@kazcw Could you also re-review tour requested changes?

vitvakatu

QA passed

…with_space_does_not_always_work_#5223

Add a safeguard that ensures that dropped frames during the visualisa…

ecaaffb

…tion opening do not erroneously close the visualisation as if it was opened as preview.

MichaelMauderer self-assigned this Feb 10, 2023

MichaelMauderer marked this pull request as ready for review February 10, 2023 15:33

MichaelMauderer requested review from wdanilo, farmaazon, mwu-tow and kazcw as code owners February 10, 2023 15:33

MichaelMauderer changed the title ~~Fix visualisation sometimes no opening with space bar.~~ Fix visualization sometimes not opening with space bar. Feb 10, 2023

MichaelMauderer changed the title ~~Fix visualization sometimes not opening with space bar.~~ Fix visualizations sometimes not opening with space bar. Feb 10, 2023

kazcw requested changes Feb 10, 2023

View reviewed changes

farmaazon approved these changes Feb 13, 2023

View reviewed changes

MichaelMauderer added 6 commits February 13, 2023 11:04

Fix memory leak.

35d3a9e

Fix docs.

c9bbec0

Bugfixes.

b70eff4

Remove unused code.

b5f2e39

Remove unused code.

3e891b7

Make clippy happy.

16c5cfa

MichaelMauderer requested a review from kazcw February 13, 2023 13:39

MichaelMauderer 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 13, 2023

Merge branch 'develop' into wp/MichaelMauderer/Opening_visualization_…

3dfdf9b

…with_space_does_not_always_work_#5223

wdanilo requested changes Feb 13, 2023

View reviewed changes

mergify bot and others added 6 commits February 13, 2023 18:39

Merge branch 'develop' into wp/MichaelMauderer/Opening_visualization_…

6d9fc2d

…with_space_does_not_always_work_#5223

Merge branch 'develop' into wp/MichaelMauderer/Opening_visualization_…

88e557c

…with_space_does_not_always_work_#5223

Merge branch 'develop' into wp/MichaelMauderer/Opening_visualization_…

308d2a6

…with_space_does_not_always_work_#5223

Merge branch 'develop' into wp/MichaelMauderer/Opening_visualization_…

4069a16

…with_space_does_not_always_work_#5223

Clarify docs.

5148832

Clippy/fmt

f95058c

MichaelMauderer requested a review from wdanilo February 14, 2023 12:03

MichaelMauderer removed the CI: Ready to merge This PR is eligible for automatic merge label Feb 14, 2023

wdanilo approved these changes Feb 15, 2023

View reviewed changes

kazcw approved these changes Feb 16, 2023

View reviewed changes

vitvakatu approved these changes Feb 16, 2023

View reviewed changes

MichaelMauderer added the CI: Ready to merge This PR is eligible for automatic merge label Feb 17, 2023

mergify bot added 2 commits February 17, 2023 09:16

Merge branch 'develop' into wp/MichaelMauderer/Opening_visualization_…

7adf721

…with_space_does_not_always_work_#5223

Merge branch 'develop' into wp/MichaelMauderer/Opening_visualization_…

9f779fd

…with_space_does_not_always_work_#5223

mergify bot merged commit e81d1e3 into develop Feb 17, 2023

mergify bot deleted the wp/MichaelMauderer/Opening_visualization_with_space_does_not_always_work_#5223 branch February 17, 2023 11:33

farmaazon mentioned this pull request Mar 6, 2023

Visualization Sometimes Blank on First View #5620

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix visualizations sometimes not opening with space bar. #5624

Fix visualizations sometimes not opening with space bar. #5624

MichaelMauderer commented Feb 10, 2023 •

edited

Loading

kazcw Feb 10, 2023

kazcw Feb 10, 2023

farmaazon left a comment

farmaazon Feb 13, 2023

wdanilo left a comment

wdanilo Feb 13, 2023

wdanilo Feb 13, 2023

MichaelMauderer commented Feb 13, 2023

wdanilo left a comment

MichaelMauderer commented Feb 15, 2023

vitvakatu left a comment

		// We also check the number of passed frames to ensure we did not spend too much time in
		// dropped frames during the visualisation initialisation.

Fix visualizations sometimes not opening with space bar. #5624

Fix visualizations sometimes not opening with space bar. #5624

Conversation

MichaelMauderer commented Feb 10, 2023 • edited Loading

Pull Request Description

Checklist

kazcw Feb 10, 2023

Choose a reason for hiding this comment

kazcw Feb 10, 2023

Choose a reason for hiding this comment

farmaazon left a comment

Choose a reason for hiding this comment

farmaazon Feb 13, 2023

Choose a reason for hiding this comment

wdanilo left a comment

Choose a reason for hiding this comment

wdanilo Feb 13, 2023

Choose a reason for hiding this comment

wdanilo Feb 13, 2023

Choose a reason for hiding this comment

MichaelMauderer commented Feb 13, 2023

wdanilo left a comment

Choose a reason for hiding this comment

MichaelMauderer commented Feb 15, 2023

vitvakatu left a comment

Choose a reason for hiding this comment

MichaelMauderer commented Feb 10, 2023 •

edited

Loading