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

Update Theia version to 1.25.0 #947

Merged
merged 2 commits into from
May 25, 2022
Merged

Update Theia version to 1.25.0 #947

merged 2 commits into from
May 25, 2022

Conversation

msujew
Copy link
Contributor

@msujew msujew commented Apr 6, 2022

Motivation

The recent two updates have fixed a lot of issues with Theia, so it would be wise to update.

Closes #852

Change description

Updates the @theia/* dependencies to 1.24.0. Changes any code that is affected by this update. Most changes are due to the monaco uplift and changes in how electron windows are handled/created.

Other information

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@msujew msujew added the topic: theia Related to the Theia IDE framework label Apr 6, 2022
@AlbyIanna
Copy link
Contributor

@msujew CI is failing here, probably because of the number of changes in the yarn.lock
Is it possible to reduce them to a minimum? Probably it'd be enough to unlock the build.

@msujew msujew force-pushed the msujew/update-theia-1.24.0 branch from 54dff56 to d36c108 Compare April 11, 2022 14:54
@msujew
Copy link
Contributor Author

msujew commented Apr 11, 2022

@AlbyIanna It took me a bit, but it seems to be a bug in node-gyp, which doesn't run correctly on some Windows systems (including GH Actions Windows runners). See this comment. Running npx node-gyp install 14.18.1 during the build seems to fix the incorrect node-gyp setup.

The other updates in the yarn.lock were mandatory, as they were related to updated dependencies used by @theia/* packages.

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

UPDATE: resolved

The IDE starts fine for me in the environment established by running a previous version of the IDE, but if I simulate a fresh install it fails to start:

To reproduce

  1. Do a minimal simulation of a fresh install by deleting the "User data" folder:
    • Windows:
      • %APPDATA%\arduino-ide\
        
    • Linux:
      • ~/.config/arduino-ide/
        
    • macOS:
      • ~/Library/Application Support/arduino-ide/
        
  2. Start the Arduino IDE.

🐛 The IDE hangs forever at the splash screen.

Arduino IDE version

2.0.0-rc5-snapshot-414f51f
(the tester build for 8357946)

Operating system

Windows 10, Ubuntu 20.04

(I didn't try it on macOS.)

Additional context

This part of the command line output looks interesting to me:

root ERROR Failed to start the frontend application.
root ERROR Error: No matching bindings found for serviceIdentifier: Symbol(CompressionToggle)
    at file:///E:/downloads/arduino-ide_2.0.0-rc5-snapshot-414f51f_Windows_64bit/resources/app/lib/bundle.js:2:8809025
    at m (file:///E:/downloads/arduino-ide_2.0.0-rc5-snapshot-414f51f_Windows_64bit/resources/app/lib/bundle.js:2:8809322)
    at b (file:///E:/downloads/arduino-ide_2.0.0-rc5-snapshot-414f51f_Windows_64bit/resources/app/lib/bundle.js:2:8809497)
    at file:///E:/downloads/arduino-ide_2.0.0-rc5-snapshot-414f51f_Windows_64bit/resources/app/lib/bundle.js:2:8810031
    at Array.forEach (<anonymous>)
    at file:///E:/downloads/arduino-ide_2.0.0-rc5-snapshot-414f51f_Windows_64bit/resources/app/lib/bundle.js:2:8810010
    at Array.forEach (<anonymous>)
    at b (file:///E:/downloads/arduino-ide_2.0.0-rc5-snapshot-414f51f_Windows_64bit/resources/app/lib/bundle.js:2:8809557)
    at file:///E:/downloads/arduino-ide_2.0.0-rc5-snapshot-414f51f_Windows_64bit/resources/app/lib/bundle.js:2:8810031
    at Array.forEach (<anonymous>)

@fstasi fstasi force-pushed the msujew/update-theia-1.24.0 branch from ddee44e to 8357946 Compare April 12, 2022 10:04
@kittaakos

This comment was marked as off-topic.

@per1234 per1234 dismissed their stale review April 13, 2022 13:10

No matching bindings found for serviceIdentifier: Symbol(CompressionToggle) error has been resolved. Thanks!

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

UPDATE: resolved by https://github.com/arduino/arduino-ide/compare/00386d4fddc7a5ce5f4fdc4032096e5b4f1ac67b..a3d38384b93ebeab10ec7f5c402bc859355c261e

The IDE is unable to open additional windows:

To reproduce

  1. Start the Arduino IDE.
    🐛 The splash screen is not shown.
  2. Open an additional window via any available mechanism:
    • File > New
    • File > Open
    • File > Sketchbook
    • Sketchbook view

🐛 A splash screen is shown unexpectedly (normally the splash screen is only shown on IDE start up, not when opening an additional window)
🐛 The new window never opens.
🐛 The splash screen never closes.

Arduino IDE version

2.0.0-rc5-snapshot-6269207
(the tester build for 00386d4)

Operating system

Windows 10, Ubuntu 20.04

(I didn't try it on macOS.)

Additional context

The above description is for when only a single window opens on startup of the IDE. The behavior is a little different when multiple windows open on startup of the IDE (due to having been open at the end of a previous session using a working version of the IDE). In this case, one window opens fully and the others do appear but never load fully:

image

@kittaakos
Copy link
Contributor

A splash screen is shown unexpectedly (normally the splash screen is only shown on IDE start up, not when opening an additional window)

That must be my fault, let me check.

@kittaakos
Copy link
Contributor

@msujew, could you please drop my suggestion (00386d4). It is causing the issue. Let's put aside that I messed up the condition here 😕, but the logic simply wrong when opening the second, third, etc. windows. We cannot rely on the window tracking from the superclass. I am sorry for the inconvenience, and thanks for the help 🙏


Per, I promise I will do more manual testing before submitting anything.

@per1234
Copy link
Contributor

per1234 commented Apr 13, 2022

Per, I promise I will do more manual testing before submitting anything.

Hey now, leave some fun for me Akos! 😆

@msujew msujew force-pushed the msujew/update-theia-1.24.0 branch from a3d3838 to 0d7b185 Compare April 20, 2022 10:53
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

UPDATE: resolved by https://github.com/arduino/arduino-ide/compare/a70273a6c10d34c6b75c446439da0209c67bfb97..130586e0f40b46d4beef4065fbc9e713ae5d712d

"Auto save" preference is not remembered

Describe the problem

🐛 The "Auto save" setting in the File > Preferences dialog is always checked, even when you had unchecked it previously.

🐛 The IDE never saves automatically, even though the "Auto save" setting is shown as enabled in the Preferences dialog.

To reproduce

  1. Select File > Preferences... from the Arduino IDE menus.
  2. Uncheck the box next to " Auto save".
  3. Click the OK button.
  4. Select File > Preferences... from the Arduino IDE menus.
    🐛 The box next to " Auto save" is checked.
  5. Click the OK button.
  6. Make a change to the sketch.
    🐛 indicator appears on right side of sketch tab.
    🐛 The change is not saved to disk.

Expected behavior

The state of the "Auto save" setting in the File > Preferences dialog reflects my previous selection.

The IDE's auto save behavior reflects the "Auto save" setting's state.

Arduino IDE version

2.0.0-rc6-snapshot-e2a9dd5
(the tester build for 0d7b185)

OS: Windows 10, Ubuntu 20.04

Additional context

This part of the command line output looks interesting:

root WARN Request to validate preference with no type information:

I don't have that when using 2.0.0-rc6


The problem does not occur when using 2.0.0-rc6

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

UPDATE: resolved by https://github.com/arduino/arduino-ide/compare/a70273a6c10d34c6b75c446439da0209c67bfb97..130586e0f40b46d4beef4065fbc9e713ae5d712d

Cursor is not placed at definition by "Go to definition" ### Describe the problem

The "Go to definition" capability provided by the integrated language server gives the user quick access to the definition of any object referenced in the sketch code.

🐛 Even though the file containing the definition is opened in the IDE, the cursor is not moved to the definition, meaning the user would still need to search through the file to find it.
When the definition is in the currently open file, this means "Go to definition" has no value and no apparent effect.

To reproduce

  1. Create the following sketch:
    void setup() {
      digitalRead(2);
    }
    void loop() {}
  2. Select Tools > Board > Arduino AVR Boards > Arduino Uno from the Arduino IDE menus.
  3. Wait for sketch processing to finish.
  4. Right click digitalRead.
    There is no special significance of this function other than it provides a more dramatic demonstration due to its definition being located many lines down in the source file.
  5. Select "Go to Definition" from the context menu.

🐛 wiring_digital.c opens as expected, but the cursor is not placed at the location of the digitalRead definition.

Expected behavior

The cursor is placed at the definition of the object that was the target of the "Go to definition" operation.

Arduino IDE version

2.0.0-rc6-snapshot-e2a9dd5
(the tester build for 0d7b185)

OS: Windows 10, Ubuntu 20.04

Additional context

This screencast provides a minimal (but less dramatic) demonstration of the issue. Note that the cursor never moves to line 1 as expected:

goto


Sketch and language server logs from the operation shown in the screencast:

LSGotoSameFile-947.zip


The issue does not occur when using Arduino IDE 2.0.0-rc6


The issue occurs even when the object reference is in a .cpp or .c source file instead of .ino.

@per1234
Copy link
Contributor

per1234 commented Apr 29, 2022

Since I see the same bug in Theia Blueprint 1.24.0, it would likely be considered out of scope for this PR, but maybe worth mentioning that this makes the already buggy "Keyboard Shortcuts" view even worse:


Custom key bindings are never shown in the "Keyboard Shortcuts" view

To reproduce

  1. Select File > Advanced > Keyboard Shortcuts from the Arduino IDE menus.
  2. Hover the mouse pointer over "Add Cursor Above".
    there is nothing special about this particular shortcut. The issue occurs with any of them.
  3. Click the pencil icon ("Edit Keybinding").
  4. Change the value of the field to alt+ctrl+shift+up
  5. Click the OK button.
    🙂 The key binding shown in the "Keybinding" column for "Add Cursor Above" is changed to Ctrl+Alt+Shift+Up as expected.
  6. Select File > New from the Arduino IDE menus.
    the bug affects all windows other than the one the customization was made in, so it will also occur via any operation that opens a new window, including restarting the IDE.
  7. Select File > Advanced > Keyboard Shortcuts from the Arduino IDE menus.
    🐛 The key binding shown in the "Keybinding" column for "Add Cursor Above" is the default Ctrl+Alt+Up (even though the custom one is actually in effect rather than the one indicated by the view.
    This is the same buggy behavior as in 2.0.0-rc6
  8. Select the "Search keybindings" field.
  9. Press any key (e.g., Space
    🙁 The The key binding shown in the "Keybinding" column for "Add Cursor Above" is still the default Ctrl+Alt+Up.
    In 2.0.0-rc6, the listed key binding finally updates to show the custom setting after you press any key with that field selected.

UPDATE: Still occurs with 2.0.0-rc6-snapshot-fd113af (build for 130586e) and with Theia Blueprint 1.25.0 (Beta)

@per1234
Copy link
Contributor

per1234 commented May 2, 2022

UPDATE: resolved by https://github.com/arduino/arduino-ide/compare/a70273a6c10d34c6b75c446439da0209c67bfb97..130586e0f40b46d4beef4065fbc9e713ae5d712d

Another change I noticed and decided I should probably mention:

The window no longer automatically reloads after changing the "Language" preference.

I am OK with the new behavior for the following reasons:

But I thought I should mention it anyway since I think the change was unintended and perhaps it is a symptom of a problem that has other more significant implications.

To reproduce

  1. Select File > Preferences... from the Arduino IDE menus.
  2. Select a different language from the "Language" menu.
  3. Click the OK button.

😕 The window does not reload.

@fstasi
Copy link
Contributor

fstasi commented May 5, 2022

@msujew do you have updates on this PR?

@msujew
Copy link
Contributor Author

msujew commented May 5, 2022

@fstasi As noted in Slack, I'm on vacation 'til the 16th of May. Afterwards this'll be my first priority.

@kittaakos
Copy link
Contributor

I checked and fixed the followings:

"Auto save" preference is not remembered

It should work now. However, there is a drawback. There was a breaking change in Theia and editor.autoSave was renamed to files.autoSave.

Suppose a user disabled the autosave, the "editor.autoSave": "off" was written in the settings JSON. This version ignores the old autosave key/value and starts the IDE2 with autosave enabled. Users can change it.
(Yes, we can read the old vale and make the code backward compatible, but we need additional Theia customization.)


Cursor is not placed at definition by "Go to definition"

I could not reproduce it. Please double-check. Thank you!

go_to_definitions.mp4

The window no longer automatically reloads after changing the "Language" preference.

It's fixed. It reloads. It was a generic reload issue with the first window. I decided to change the splash screen logic once more and clean it up.

@msujew msujew changed the title Update Theia version to 1.24.0 Update Theia version to 1.25.0 May 18, 2022
@per1234
Copy link
Contributor

per1234 commented May 19, 2022

Thanks @kittaakos. The build workflow did not run because of the merge conflict. Would you mind resolving that so I can make sure to use a standardized build for the testing?

@kittaakos
Copy link
Contributor

When reloading the browser window (for example, the language has changed), we show index.html instead of Arduino IDE.

index_html.mp4

Also, when changing the language, we should reload all windows, not just a current one.

The spinner was a change in Theia, but it feels stupid to have the new, fancy spinner, although the IDE startup performance time is the same. Do you want me to restore the old spinner? I vote to restore the old spinner. Thoughts? Thank you!


Dev notes: I merged the main instead of rebasing. Once all the feedback threads are resolved, I will squash rebase.


I found these problems:

Tons of warnings in the logs:

[83713:0519/093905.459621:ERROR:accelerator_util.cc(24)] The accelerator string can only contain ASCII characters
[83713:0519/093905.459641:ERROR:accelerator_util.cc(24)] The accelerator string can only contain ASCII characters
[83713:0519/093905.459661:ERROR:accelerator_util.cc(24)] The accelerator string can only contain ASCII characters
[83713:0519/093905.459684:ERROR:accelerator_util.cc(24)] The accelerator string can only contain ASCII characters

Sometimes the firmware updater gets an incorrect JSON (see here):

root ERROR Request updatableBoards failed with error: Error executing "/Users/a.kitta/dev/git/arduino-ide/arduino-ide-extension/build/arduino-fwuploader" firmware list --format json: unexpected end of JSON input
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x14bd30e]

goroutine 1 [running]:
github.com/arduino/arduino-fwuploader/cli/firmware.list(0x0, 0x0)
	/home/runner/work/arduino-fwuploader/arduino-fwuploader/cli/firmware/list.go:66 +0x4e
github.com/arduino/arduino-fwuploader/cli/firmware.newListCommand.func1(0xc000492500, 0xc000158ea0, 0x0, 0x2)
	/home/runner/work/arduino-fwuploader/arduino-fwuploader/cli/firmware/list.go:42 +0x39
github.com/spf13/cobra.(*Command).execute(0xc000492500, 0xc000158e80, 0x2, 0x2, 0xc000492500, 0xc000158e80)
	/home/runner/go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:856 +0x2c2
github.com/spf13/cobra.(*Command).ExecuteC(0xc0001eb900, 0x0, 0x0, 0x1501260)
	/home/runner/go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:960 +0x375
github.com/spf13/cobra.(*Command).Execute(...)
	/home/runner/go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:897
main.main()
	/home/runner/work/arduino-fwuploader/arduino-fwuploader/main.go:35 +0x3c Error: Error executing "/Users/a.kitta/dev/git/arduino-ide/arduino-ide-extension/build/arduino-fwuploader" firmware list --format json: unexpected end of JSON input
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x14bd30e]

goroutine 1 [running]:
github.com/arduino/arduino-fwuploader/cli/firmware.list(0x0, 0x0)
	/home/runner/work/arduino-fwuploader/arduino-fwuploader/cli/firmware/list.go:66 +0x4e
github.com/arduino/arduino-fwuploader/cli/firmware.newListCommand.func1(0xc000492500, 0xc000158ea0, 0x0, 0x2)
	/home/runner/work/arduino-fwuploader/arduino-fwuploader/cli/firmware/list.go:42 +0x39
github.com/spf13/cobra.(*Command).execute(0xc000492500, 0xc000158e80, 0x2, 0x2, 0xc000492500, 0xc000158e80)
	/home/runner/go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:856 +0x2c2
github.com/spf13/cobra.(*Command).ExecuteC(0xc0001eb900, 0x0, 0x0, 0x1501260)
	/home/runner/go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:960 +0x375
github.com/spf13/cobra.(*Command).Execute(...)
	/home/runner/go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:897
main.main()
	/home/runner/work/arduino-fwuploader/arduino-fwuploader/main.go:35 +0x3c
    at ChildProcess.<anonymous> (/Users/a.kitta/dev/git/arduino-ide/arduino-ide-extension/lib/node/exec-util.js:53:31)
    at ChildProcess.emit (node:events:394:28)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:290:12)
    at Process.callbackTrampoline (node:internal/async_hooks:130:17)

Co-authored-by: Mark Sujew <mark.sujew@typefox.io>
Co-authored-by: Akos Kitta <a.kitta@arduino.cc>

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
@per1234
Copy link
Contributor

per1234 commented May 20, 2022

Looking good! All the outstanding issues from my previous reviews are resolved except for the keyboard shortcuts UI issue (which I see is also still in Theia Blueprint 1.25.0 (Beta)).


#947 (comment)

This version ignores the old autosave key/value and starts the IDE2 with autosave enabled. Users can change it.
(Yes, we can read the old vale and make the code backward compatible, but we need additional Theia customization.)

Since the project is still in a prerelease phase, I don't think it is necessary.

I would like to see it be clearly documented in the release notes of the release where this is introduced though.


#947 (comment)

When reloading the browser window (for example, the language has changed), we show index.html instead of Arduino IDE.

I think it is not a great UX. I believe this change was introduced by 112153f

I would be happy to open an issue about it if you think it is something that should be investigated eventually.


#947 (comment)

Also, when changing the language, we should reload all windows, not just a current one.

The current limited approach of only reloading the active window was explained in #625:

Known Issues

At the moment only the active window is reloaded. We need to propagate a message to other windows and prompt the user to reload the other windows. Other strategies can better suite our needs, but consider this is a corner case, as usually users change the language just once, and as soon as the install the IDE.

I think @AlbyIanna might have looked into reloading all the windows at one point, but I don't recall the exact details.

Something to consider is that full localization update will also require the preference to take effect in Arduino CLI.


#947 (comment)

Do you want me to restore the old spinner? I vote to restore the old spinner. Thoughts?

I don't have a preference either way. Maybe @91volt or @gmarchiarduino would be interested in the subject though.


#947 (comment)

Tons of warnings in the logs:

I haven't been able to reproduce this, even after checking on my Windows and Linux machines.

Do you get it even after clearing your IDE configuration files?

@kittaakos
Copy link
Contributor

Thank you for the review ❤️ See my comment below. Please let me know if there is anything else I need to do for this PR.

I would like to see it be clearly documented in the release notes of the release where this is introduced though.

I will take care of it.

I would be happy to open an issue about it if you think it is something that should be investigated eventually.

It is worth an issue. It used to work. Now, it's showing index.html.

explained in #625:

Something to consider is that full localization update will also require the preference to take effect in Arduino CLI.

👍 Thank you for linking the PR. If it does not take too much time, I think we need a ticket for this. We can reload all windows, and we need to restart the CLI.

Do you want me to restore the old spinner? I vote to restore the old spinner. Thoughts?

I don't have a preference either way. Maybe @91volt or @gmarchiarduino would be interested in the subject though.

Note: It's already reset, we have the old spinner in the IDE2. +1 for having the old spinner and switching to the new one once the startup is faster. In the long run, we can add a preload. See an example here.

Tons of warnings in the logs:

I fixed this too.

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

I did some further testing of the build and didn't find any problems. Thanks!

@kittaakos
Copy link
Contributor

Thank you for the initial changes and the review. 🙏 I am merging the PR.

@kittaakos kittaakos merged commit 522a5c6 into main May 25, 2022
@kittaakos kittaakos deleted the msujew/update-theia-1.24.0 branch May 25, 2022 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: theia Related to the Theia IDE framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pressing Ctrl+R to Verify/Compile screen goes white with only tool bar and circling dots.
5 participants