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

fix accessibility support status bar on reload and window open #172934

Merged
merged 8 commits into from Feb 1, 2023

Conversation

meganrogge
Copy link
Contributor

fix #172690
fix #172688

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

I was not suggesting to store this in UI state (storage service) but just in a property of the service. I do not think we can assume that after a restart, accessibility mode is still on, or can we?

I am not fully understanding why we cannot do this:

  • vscode starts
  • we check if a18n is on from the main process
  • we pass this as value to windows on startup
  • we keep the value in a variable in the a18n service
  • the status bar reads it
  • we also update from events as we do today

@meganrogge
Copy link
Contributor Author

I misunderstood when I read cache because typically the storage service is where we do that. I will try that approach

This reverts commit 259ab0b.
@meganrogge
Copy link
Contributor Author

meganrogge commented Feb 1, 2023

@rzhao271 suggested this should maybe go in onReady instead. thoughts @bpasero ?

global['accessibilitySupportEnabled'] = app.accessibilitySupportEnabled;

src/main.js Outdated Show resolved Hide resolved
@meganrogge meganrogge changed the title cache value of accessibilitySupport so status bar entry is correct regardless of event timing fix accessibility support status bar on reload and window open Feb 1, 2023
src/main.js Outdated Show resolved Hide resolved
src/vs/code/electron-main/app.ts Outdated Show resolved Hide resolved
src/vs/platform/windows/electron-main/windowImpl.ts Outdated Show resolved Hide resolved
This reverts commit 39954d2.
@meganrogge
Copy link
Contributor Author

meganrogge commented Feb 1, 2023

@deepak1556 from your comments, I thought this was more than a UX/status bar item issue. But in my testing, it's just that. This fix is simple & works

src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
@meganrogge meganrogge merged commit e3d73a5 into main Feb 1, 2023
@meganrogge meganrogge deleted the merogge/acc-status branch February 1, 2023 05:28
@github-actions github-actions bot locked and limited conversation to collaborators Mar 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants