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

Migrate from PhosphorJS to Lumino #14320

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

sdirix
Copy link
Member

@sdirix sdirix commented Oct 15, 2024

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.

This PR is a draft PR as not all known issues are yet fixed.

Open issues are:

  • Tab bars currently have double the width than they should have (@tsmaeder)
  • The "..." bar actions do not work (@tsmaeder)
  • When shrinking the left view tab bar, the content is not adjusted (normally, we get a "..." when not all view tabs can be shown (@tsmaeder)
  • When opening a diff editor (from the git changes view), the height of the editor tab bar is too big. When switching between editors before and after the diff editor tab, the height becomes correct sometimes (when switching from the one after the diff editor)
  • The preferences tree does not react on selections (@tsmaeder)
  • The preferences content does not scroll (@tsmaeder)
  • Existing import and style changes update to latest code base (@sdirix)
  • Fix CI checks (@sdirix)

If anyone is interested in solving one or more of these issues, contact me or open a PR against the lumino-migration branch in the EclipseSource fork.

Note that Lumino introduced a code change in which empty menus can't be opened. However Theia uses an approach in which menus are filled only shortly before they are shown. Therefore this new behavior is for now patched in this draft PR. However this means the patching is now ALWAYS required and not only when wanting to support secondary windows.

We might want to think about contributing a change to Lumino first before requiring this for all downstream projects.

In any case this is of course a huge breaking change.

How to test

Build and test the application

Review checklist

Reminder for reviewers

- 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
'@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
sdirix and others added 9 commits November 29, 2024 11:01
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Waiting on reviewers
Development

Successfully merging this pull request may close these issues.

2 participants