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

Theia 1.53.0 #384

Merged
merged 5 commits into from
Sep 2, 2024
Merged

Theia 1.53.0 #384

merged 5 commits into from
Sep 2, 2024

Conversation

jfaltermeier
Copy link
Contributor

@jfaltermeier jfaltermeier commented Aug 29, 2024

What it does

closes #378
closes #383

Contributed on behalf of STMicroelectronics

How to test

  • test that IDE is still working as expected
  • test collaboration feature and that info text is available on welcome page
  • test theia:// protocol handling (see Implement previously stubbed window#registerURIhandler (#13169) theia#13306 for instructions)
    • on Linux test that opting out of desktop file creation works as well
      • the file that stores user decisions is located at ~/.theia-ide/globalStorage/theia-ide-launcher/desktopfile.json
      • current behavior:
        • we only maintain one set of Theia IDE desktop files. If there are two TheiaIDE AppImages in different locations only one can be set. (Users may create them themselves if required)
        • we store if an AppImage path should not have a desktop file -> the user will not be asked again
          • this is stored in the declined array in above file
        • if an AppImage path is not declined we check whether it is the current default
          • if yes -> no op since already set
          • if no -> ask user if desktop files should be created/updated

Review checklist

Reminder for reviewers

* update to latest Theia #378
* update electron
* add @theia/collaboration #383
* update Jenkinsfile global environment

Contributed on behalf of STMicroelectronics
@jfaltermeier jfaltermeier force-pushed the jf/1.53.0 branch 2 times, most recently from d77fe6c to b341482 Compare August 29, 2024 13:19
@jfaltermeier
Copy link
Contributor Author

jfaltermeier commented Aug 29, 2024

For testing the theia scheme on ubuntu I slightly adjusted the generated desktop file:

[Desktop Entry]
Name=TheiaIDE
Exec=/home/johannes/.local/share/applications/theia-ide-launcher.sh %U
Terminal=false
Type=Application
Icon=/home/johannes/.local/share/applications/theia-ide-electron-app.png
StartupWMClass=TheiaIDE
MimeType=x-scheme-handler/theia;
Comment=Eclipse Theia IDE product
Categories=Development;

The referenced launcher script looks like this (basically adding logging of the passed arguments):

#!/bin/bash

# Log the arguments to a file
echo "App launched with: $@" >> /tmp/xxx-theia_launch.log

# Launch the AppImage with the provided arguments
/home/johannes/Git/theia-blueprint/applications/electron/dist/TheiaIDE.AppImage "$@"

Now I can see that the AppImage gets the theia scheme URL passed:

App launched with: 
App launched with: theia://tylerleonhardt.open-external/open-external

Nothing happens though (besides that a new frontend is started), but I guess this is then an issue with plain Theia and not with the IDE and the metadata in the desktopfile

* create desktop file for linux desktop integration
* add protocol for mac

Contributed on behalf of STMicroelectronics
@jfaltermeier
Copy link
Contributor Author

On Windows and Mac the theia:// protocol gets handled as expected.

@jfaltermeier
Copy link
Contributor Author

jfaltermeier commented Aug 30, 2024

For testing the theia scheme on ubuntu I slightly adjusted the generated desktop file:

[Desktop Entry]
Name=TheiaIDE
Exec=/home/johannes/.local/share/applications/theia-ide-launcher.sh %U
Terminal=false
Type=Application
Icon=/home/johannes/.local/share/applications/theia-ide-electron-app.png
StartupWMClass=TheiaIDE
MimeType=x-scheme-handler/theia;
Comment=Eclipse Theia IDE product
Categories=Development;

The referenced launcher script looks like this (basically adding logging of the passed arguments):

#!/bin/bash

# Log the arguments to a file
echo "App launched with: $@" >> /tmp/xxx-theia_launch.log

# Launch the AppImage with the provided arguments
/home/johannes/Git/theia-blueprint/applications/electron/dist/TheiaIDE.AppImage "$@"

Now I can see that the AppImage gets the theia scheme URL passed:

App launched with: 
App launched with: theia://tylerleonhardt.open-external/open-external

Nothing happens though (besides that a new frontend is started), but I guess this is then an issue with plain Theia and not with the IDE and the metadata in the desktopfile

I can see that VSCode uses two desktop files, one passing --open-url that is also hidden in the launcher. I'll try this

Edit: That seems to do the trick, I'll adapt accordingly

* add dedicated URL handler desktop file as well
* remove MimeType from builder config again, since we need additional
open-url args
@jfaltermeier jfaltermeier changed the title WIP Theia 1.53.0 Theia 1.53.0 Aug 30, 2024
@jfaltermeier jfaltermeier marked this pull request as ready for review August 30, 2024 06:46
@jfaltermeier jfaltermeier requested a review from tsmaeder August 30, 2024 07:07
@@ -19,7 +19,7 @@ jobs:
strategy:
fail-fast: false
matrix:
os: [windows-2019, ubuntu-latest, macos-14]
os: [windows-2019, ubuntu-latest, macos-12]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the downgrade to 12?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the removal of macOS-11, we haven't had a working verification build for macOS, except on the Eclipse CI. See https://github.com/eclipse-theia/theia-blueprint/actions/workflows/build.yml . I started addressing this issue by aligning our approach with that of the CDT Cloud Blueprint, as seen here: eclipse-cdt-cloud/cdt-cloud-blueprint@fddcf9a

Generally, I believe it's better to use older machines for verification jobs because if something works on macOS 12, it's likely to work on macOS 14 as well. However, if it only runs on macOS 14, we cannot be sure it will work on macOS 12.

@@ -1,5 +1,5 @@
/********************************************************************************
* Copyright (C) 2022 EclipseSource and others.
* Copyright (C) 2022-2024 EclipseSource and others.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we update the headers for files we change, usually. Not sure if there is a policy and what it is.

}

@injectable()
export class TheiaDesktopFileServiceEndpoint implements BackendApplicationContribution {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this have to be a separate endpoint? Can't we just make it a back-end service?
Also: can we scope this to linux?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to be consistent with the existing code in that package that may create a launcher script that is available on the path. If we refactor this, then we should probably update both.

@@ -0,0 +1,151 @@
/********************************************************************************
Copy link
Contributor

Choose a reason for hiding this comment

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

From my reading on the internet, I was under the impression that electron-builder generates a .desktop file from the info in electron-builder.yml automatically. Am I mistaken?

Copy link
Contributor Author

@jfaltermeier jfaltermeier Sep 2, 2024

Choose a reason for hiding this comment

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

It can. The AppImage then does include a desktop file, but it's packaged inside the AppImage. Since the AppImage is an executable, users who directly execute it won't see the desktop file unpacked though. Typically, an AppImage launcher is needed to unpack the desktop file, but not all users install such a launcher as it's often not necessary.

Additionally, the generated desktop file behaves as described in my earlier comment: Issue Comment Link. It simply opens a new Theia window without invoking the URL.
For having a regular launcher and the url handler, I think we need to have two desktop files, as VS code does as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

"...and this my friend, is why 2024 will not be the year of Linux on the desktop" ;-)

@tsmaeder tsmaeder self-requested a review September 2, 2024 09:06
@jfaltermeier jfaltermeier merged commit bef51f3 into master Sep 2, 2024
6 checks passed
@jfaltermeier jfaltermeier deleted the jf/1.53.0 branch September 2, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new collaboration Feature to theia IDE 1.53 Add App Metadata To Open theia:// links.
3 participants