-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Migrate from PhosphorJS to Lumino #14320
Conversation
- switch to lumino packages - remove phosphorjs sharing Contributed on behalf of STMicroelectronics
Adapt all CSS and code to refer to lumino classes. Contributed on behalf of STMicroelectronics
- use 'iconClass' instead of 'class' in commands - only access shell handlers once they are initialized - use Lumino's host option for secondary window support - adapt tab bar handling - adapt to Iterable changes - adapt to Drag interface changes - adapt to Widget id changes Contributed on behalf of STMicroelectronics
Adds the patch for Lumino which achieves the following: - Make sure listeners are registered on the correct document. This is necessary for secondary window support. - Make sure empty menus can be opened. This is required as Theia fills menus right before they are shown, so they seem to be empty. Contributed on behalf of STMicroelectronics
Contributed on behalf of STMicroelectronics
991a63f
to
b3dddca
Compare
Contributed on behalf of STMicroelectronics
'@lumino/dragdrop' consumes DragEvent already at loading time. Therefore the DragEvent mock must already be available before executing the tests. Adds a new private '@theia/test-setup' dev package which is consumed in the mocha config to mock 'DragEvent' before loading and compiling tests. Contributed on behalf of STMicroelectronics
1897faf
to
b074e61
Compare
Contributed on behalf of STMicroelectronics
After being attached, the search-in-workspace widget tries to focus its input field. However the input field is rendered via React and might not be created yet. Adjusts the focus mechanism to wait until the input is rendered before trying to focus it. Contributed on behalf of STMicroelectronics
Contributed on behalf of STMicroelectronics
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
To workaround Lumino's empty menu checks we now always return menus with at least one item. Contributed on behalf of STMicroelectronics
Adapts the current focus behavior for non-native menus to work properly again. However different to before they now keep their focus when closed. Therefore this is still wip. Adds a developer variable to enable/disable the new adaptations. Contributed on behalf of STMicroelectronics
Makes sure that the terminal widget id is set to the injected value. Fixes Terminal Playwright tests. Contributed on behalf of STMicroelectronics
Adjusts Lumino for second window usage Contributed on behalf of STMicroelectronics
- removes the debug variable - do not restore focus to menu elements Contributed on behalf of STMicroelectronics
@tsmaeder Thanks for the fixes! I tested it a bit and it's significantly better than before. There is one behavioral change though: The title bar menus can no longer be closed by clicking them again which seems a bit weird. Edit: |
Contributed on behalf of STMicroelectronics
This PR is now ready for review. @msujew please have a look.
|
@msujew ping ;) |
I merged in the latest |
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 was able to find two regressions:
- The menu CSS seems to have changed apparently. This affects all main menu and context menu rendering. Menu entries now have a focus outline. The outline is also visible in case the element is not
:hover
:
Current design for reference (with rounded corners):
- Clicking on a top level menu element (such as
File
) brings it up correctly, but clicking the menu button again does nothing. I would expect it to close the menu again. This is the case in VS Code and current Theia.
Aside from that, this looks very good. I think after fixing those two issues, we should be good to go.
private refsAreSet: Promise<void> = new Promise(resolve => this.resolveRefsAreSet = resolve); | ||
private resolveRefsAreSet: () => 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.
Question/Suggestion: Why not use a Deferred
here?
private setInitialized: () => void; | ||
initialized: Promise<void> = new Promise(resolve => { this.setInitialized = resolve; }); |
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.
Same as above, why not a Deferred
?
@msujew thanks for the review.
I'm not sure this is an actual bug: we used to have a bug that would close the menu on focus change which was fixed. It might be that this is actually the correct behaviour for Lumino menus. We should check. Anyway: I don't think we should block the merge for that one: you can simple hit "escape" or click anywhere outside the menu to make it disappear. |
Personally I would also prefer the menus to close again, however this is not a regression of this PR. The current Theia |
Now I'm confused: in Theia IDE, the menu closes on second click in the native menus and stays open in the custom menus |
Yes, that's because Electron native menus behave that way, and our changes on master regarding (PhosporJS) menus behave the other way. We should align them again in a follow up at some point. |
Contributed on behalf of STMicroelectronics
Contributed on behalf of STMicroelectronics
I pushed two commits addressing your review issues. As discussed above, the title menu's (non-native) behavior already changed on |
Contributed on behalf of STMicroelectronics
I overlooked that we already had some CSS customization in menu.css. I moved the focus customization there as that is a better fit than index.css. No functional or CSS changes. |
I can confirm. I was testing using the |
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.
Alright, thank you for the swift reply. I could find no more regressions from master
, and the code looks good to me 👍
Contributed on behalf of STMicroelectronics
@tsmaeder Can you do or trigger the prerelease? |
What it does
Migrate from PhosphorJS to Lumino
PhosphorJS is no longer actively maintained for a few years now. Lumino is an active fork, maintained by the JupyterLab community.
While PhosphorJS proved reliable for a long time, the cracks are starting to show. For example we need to patch PhosphorJS to support secondary windows.
The JupyterLab community is welcoming and is open to accept contributions.
Contributed on behalf of STMicroelectronics
Current state
The PR is split into separate logical commits to make the reviewing and maintenance work easier in case the PR takes longer to complete.
All known issues are now fixed
master
: linux-only: context menu of terminal and chat view in secondary windows are opened in main window. editor context menus however do work.master
: Selecting an element in a different pane (e.g. file explorer, or market place) and then interacting with the title menu is completely broken. Cause seems to be menu-dirty-changes, triggered by the focus change in parts.This is a huge breaking change.
Follow ups
For now we continue to use patching on Lumino to enable secondary window support. We also still use some workarounds and hacks for enriching Lumino usage. As the Lumino community is open for suggestions and contributions, we can start the process of getting rid of these workarounds and the patching in the long run.
How to test
Build and test the application(s)
Review checklist
Reminder for reviewers