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.

Sign up for GitHub

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

Jump to bottom

improve widget focus handling #7707

Merged
merged 2 commits into from
May 6, 2020
Merged

improve widget focus handling #7707

merged 2 commits into from
May 6, 2020

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Apr 30, 2020 •
edited
Loading

What it does

How to test

  • Close active widget and check that focus is transfered.
  • Run the debug session and check language status bar elements it should work even when there is not active widget.
  • Try in different layouts.

Review checklist

Reminder for reviewers

@akosyakov akosyakov added editor issues related to the editor shell issues related to the core shell labels Apr 30, 2020
@akosyakov akosyakov force-pushed the akosyakov/missing-keyboard-focus-7170 branch 2 times, most recently from 6bb2a12 to 3f0452e Compare May 1, 2020 14:11
@akosyakov akosyakov requested review from vince-fugnitto, spoenemann and AlexTugarev and removed request for vince-fugnitto May 1, 2020 14:24
@akosyakov
Copy link
Member Author

cc @corneliusludmann

@akosyakov akosyakov marked this pull request as ready for review May 1, 2020 14:24
@akosyakov akosyakov force-pushed the akosyakov/missing-keyboard-focus-7170 branch from 3f0452e to c78d1ec Compare May 1, 2020 14:49
akosyakov added 2 commits May 4, 2020 07:11
@akosyakov
fix eclipse-theia#7170: default to last visible editor if no last foc…
57eb460 
…used editor

Signed-off-by: Anton Kosyakov <[email protected]>
@akosyakov
fix eclipse-theia#2234: activate another widget if an active widget g…
bb7462d 
…ets closed

support next/previous tab group commands
support next/previous tab in group commands

Signed-off-by: Anton Kosyakov <[email protected]>
@akosyakov akosyakov force-pushed the akosyakov/missing-keyboard-focus-7170 branch from c78d1ec to bb7462d Compare May 4, 2020 07:12
vince-fugnitto
vince-fugnitto reviewed May 4, 2020
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I verified the changes and noticed that #7170 does not work properly?
I opened a 'preview editor' and the focus still remains in the explorer when I start typing to update the document. If I do a search however (ctrlcmd+f) I see that focus is transferred to the document successfully. Is this intended?

@akosyakov
Copy link
Member Author

akosyakov commented May 5, 2020 •
edited
Loading

I opened a 'preview editor' and the focus still remains in the explorer when I start typing to update the document. If I do a search however (ctrlcmd+f) I see that focus is transferred to the document successfully. Is this intended?

It is correct behaviour of preview, focus should still belong to the navigator. But now you should see that the status bar reflects the state of a previewed document and you can debug it from the debug view without focusing.

spoenemann
spoenemann approved these changes May 5, 2020
View reviewed changes
packages/core/src/browser/shell/application-shell.ts
newValue['onCloseRequest'] = onCloseRequest;
newValue['onCloseRequest'](msg);
};
this.toDisposeOnActiveChanged.push(Disposable.create(() => newValue['onCloseRequest'] = onCloseRequest));
Copy link
Contributor

Choose a reason for hiding this comment

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

In case we fork or reimplement PhosphorJS, we should add direct support for this so we don't have to fiddle with the onCloseRequest method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah something like onWillClose event for all widgets will be better.

@akosyakov
Copy link
Member Author

@vince-fugnitto Could you check too? I will wait to merge till tomorrow evening.

vince-fugnitto
vince-fugnitto approved these changes May 5, 2020
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes work well for me! 👍
I verified the following:

  • the new commands work successfully
  • closing an editor switches focus to an opened editor (based on group)
  • preview editors now show statusbar info (language, encoding, etc.)
  • preview editors can now be debugged (ran a mocha test as a preview)

@vince-fugnitto
Copy link
Member

@vince-fugnitto Could you check too? I will wait to merge till tomorrow evening.

Yes! I was just updating the review :)

@akosyakov akosyakov merged commit 3d17ad2 into eclipse-theia:master May 6, 2020
@akosyakov akosyakov deleted the akosyakov/missing-keyboard-focus-7170 branch May 6, 2020 07:56
@vince-fugnitto vince-fugnitto mentioned this pull request Jul 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto approved these changes

@spoenemann spoenemann spoenemann approved these changes

@AlexTugarev AlexTugarev Awaiting requested review from AlexTugarev

@kittaakos kittaakos Awaiting requested review from kittaakos

@RomanNikitenko RomanNikitenko Awaiting requested review from RomanNikitenko

Assignees
No one assigned
Labels
editor issues related to the editor shell issues related to the core shell
Projects
None yet
Milestone
No milestone
3 participants
@akosyakov @vince-fugnitto @spoenemann