-
Notifications
You must be signed in to change notification settings - Fork 27.9k
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
allow commands to run from terminal accessible view #194246
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty big change to go in after testing, it doesn't seem too critical as it's about canceling an action so would it be better to merge in October?
I wondered about that, but this is a regression bc the following worked before:
Without this change:
With this change:
|
Actually, the above case can be fixed by setting |
show(provider: IAccessibleContentProvider): void; | ||
showLastProvider(id: AccessibleViewProviderId): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of firing an event with an id back to another provider (circular dep), can we indicate in the show
call that we want to go back upon exit/close. Then manage it all inside the a11y view service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because at the time of the show
call, we don't know that we want to go back.
Show
is called before run recent command is invoked for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the service knows what came before, so the second provider could signal that it was canceled and it should open the one before. For example, onClose: Event<{ reopenLast: boolean }>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is we only want to reopen last onClose
for certain cases. And the provider isn't aware of what those cases are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by "second provider" 🤔 . The parties of interest here are the original provider and the quick-pick / action that interrupts / hides it temporarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I get it. It just feels a little flimsy right now and easily breakable. Could we do something to make it more reliable? Tests that verify it interacts nicely with the second quick pick would be good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think smoke tests would be needed for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be good, but I can just imagine the flakes now... 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smoke tests would be best yeah. I would think they'd be fairly reliable if all they do is verify the view is up, not validate the contents.
allow commands to run from terminal accessible view
Allows a SR user to go back to the accessible view after invoking
Run Recent Command
,Go to Recent Directory
, or when escaping fromOpen detected link
fixes #193988