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

Pressing double space to open full screen vis is closing it first #6260

Closed
2 tasks
sylwiabr opened this issue Apr 13, 2023 · 7 comments · Fixed by #6663
Closed
2 tasks

Pressing double space to open full screen vis is closing it first #6260

sylwiabr opened this issue Apr 13, 2023 · 7 comments · Fixed by #6663
Assignees
Labels
--bug Type: bug -viz d-easy Difficulty: little prior knowledge required p-low Low priority

Comments

@sylwiabr
Copy link
Member

Discord username

No response

What type of issue is this?

Transient – Occuring only once

Is this issue blocking you from using Enso?

  • Yes, I can't use Enso because of this issue.

Is this a regression?

  • Yes, previous version of Enso did not have this issue.

What issue are you facing?

Having the visualization under the node open and pressing double space to have it full screen is closing it and then reopening. This causes the settings of the visualization (like custom columns width for table) to reset.

Expected behaviour

Opening the full screen visualization should not close it.

How we can reproduce it?

No response

Screenshots or screencasts

No response

Enso Version

2023.1.1-nightly.2023.4.13

Browser or standalone distribution

Standalone distribution

Browser Version or standalone distribution

standalone

Operating System

MacOS

Operating System Version

No response

Hardware you are using

No response

@sylwiabr
Copy link
Member Author

This is rather old regression so there is no point in bisecting.

@vitvakatu vitvakatu added d-unknown Difficulty: unable to estimate difficulty p-low Low priority and removed triage labels Apr 13, 2023
@farmaazon
Copy link
Contributor

farmaazon commented Apr 13, 2023

Is it actually a regression? @sylwiabr Do you remember it working at any point? Because I think it always worked this way.

We may check even the last alpha package.

@sylwiabr
Copy link
Member Author

@wdanilo mentioned we were handling it specially to preserve such behavior.

@farmaazon farmaazon added the s-help-wanted Status: help wanted with the task label Apr 17, 2023
@farmaazon
Copy link
Contributor

The double pressing is handled in the general EnsoGL keyboard handling system. I haven't found any code suppressing single press when double press is expected.

Also, I don't know how to handle it properly. We usually do not want to delay GUI reactions for keystrokes - and this would be necessary if we want to suppress single press event when double pressed event is fired. So probably we need to handle this in visualization container. But here too, I think, delaying hiding visualization on single space press would give a bad UX. Perhaps we could restrain from deleting visualization instance for some time after hiding visualization (so the state like changed column width will be preserved).

However, maybe we should somehow preserve visualization state not only in "switching to full screen" scenario?

@wdanilo
Copy link
Member

wdanilo commented Apr 18, 2023

100% agree with Adam, this would be a bad UX. Hmm, @farmaazon @sylwiabr do you have proposals of how it might work? Let's consider ANY change, including changing shortcuts.

@farmaazon farmaazon moved this from ❓New to ⚙️ Design in Issues Board Apr 24, 2023
@sylwiabr
Copy link
Member Author

sylwiabr commented May 3, 2023

I would suggest shift+space for full screen vis - what do you. think @wdanilo? Also an icon to open/close full screen vis would be a grat improvement but there is a separate task for this.

@wdanilo
Copy link
Member

wdanilo commented May 4, 2023

Let's do it :)

@sylwiabr sylwiabr moved this from ⚙️ Design to 📤 Backlog in Issues Board May 4, 2023
@sylwiabr sylwiabr added this to the Design Partners milestone May 4, 2023
@vitvakatu vitvakatu added d-easy Difficulty: little prior knowledge required and removed d-unknown Difficulty: unable to estimate difficulty --regression Important: regression s-help-wanted Status: help wanted with the task labels May 7, 2023
@vitvakatu vitvakatu assigned Procrat and unassigned farmaazon May 8, 2023
@Procrat Procrat moved this from 📤 Backlog to 🔧 Implementation in Issues Board May 12, 2023
@Procrat Procrat moved this from 🔧 Implementation to 👁️ Code review in Issues Board May 12, 2023
@Procrat Procrat moved this from 👁️ Code review to 🌟 Q/A review in Issues Board May 15, 2023
@mergify mergify bot closed this as completed in #6663 May 17, 2023
mergify bot pushed a commit that referenced this issue May 17, 2023
Fixes #6260: The shortcut to open the full-screen visualisation is now `shift-space` so it doesn't interfere with the `space` shortcut to toggle the mini-visualisation.
@github-project-automation github-project-automation bot moved this from 🌟 Q/A review to 🟢 Accepted in Issues Board May 17, 2023
@farmaazon farmaazon moved this from 🟢 Accepted to 🗄️ Archived in Issues Board May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--bug Type: bug -viz d-easy Difficulty: little prior knowledge required p-low Low priority
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants