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

Upgrade to Electron 4 #19373

Merged
merged 29 commits into from Jul 19, 2019

Conversation

@nathansobo
Copy link
Contributor

commented May 20, 2019

This PR will track our progress upgrading to Electron 4.

We just upgraded to Electron 3, but we're experiencing a crash deep in the bowels of V8 (#19372). I'm interested in whether Electron 4 might fix this crash.

Issues to resolve

@@ -12,7 +12,7 @@
"url": "https://github.com/atom/atom/issues"
},
"license": "MIT",
"electronVersion": "3.1.9",
"electronVersion": "4.2.0",

This comment has been minimized.

Copy link
@mfonville

mfonville May 21, 2019

Contributor

Isn't it more practical to use 4.2.2 to also have the latest bugfixes?

This comment has been minimized.

Copy link
@UziTech

UziTech May 21, 2019

Contributor

Why not go straight to v5.0.1?

This comment has been minimized.

Copy link
@nathansobo

nathansobo May 21, 2019

Author Contributor

It's just about being incremental. We tried to go to 3 first, but we're hitting #19372, so we wanted to see how we'd do in 4. Unfortunately we still don't really know if the crash is fixed on Electron 4 because we can't get it started yet.

This comment has been minimized.

Copy link
@nathansobo

nathansobo May 21, 2019

Author Contributor

But yeah, we should be on the latest patch release.

This comment has been minimized.

Copy link
@mfonville

mfonville Jun 1, 2019

Contributor

4.2.3 has been released yesterday

This comment has been minimized.

Copy link
@mfonville

mfonville Jun 5, 2019

Contributor

And 4.2.4 has been tagged

This comment has been minimized.

Copy link
@mfonville

mfonville Jun 19, 2019

Contributor

And 4.2.5 has been tagged now too

@50Wliu

This comment has been minimized.

Copy link
Member

commented May 21, 2019

I was able to successfully build Atom by skipping the snapshot generation and using a patched version of fuzzy-native (to bypass node-pre-gyp), but starting Atom fails with "The specified procedure could not be found" when attempting to load nsfw 😕. That usually points to a rebuild problem but apm seems to be doing the right thing...

Edit: Making progress. I can get Atom to start now. Mostly by patching our native modules to get rid of win_delay_load_hook: https://electronjs.org/docs/tutorial/using-native-node-modules#a-note-about-win_delay_load_hook

David Wilson and others added some commits Dec 13, 2018

Use correct fontWeight value in ThemeManager spec
In Chrome 66+ it seems that getComputedStyle().fontWeight returns the computed
numeric value of the style instead of the original descriptive name.  We now
look for value '700' which corresponds to the value of 'bold'.
⬆️ @atom/nsfw, git-utils to allow loading on Windows
And a bump for fuzzy-finder to remove unnecessary dependencies that were causing installation issues
Try updating @atom/nsfw to 1.0.23-1
Let's see if it fixes the config watching warnings...
@Asday

This comment has been minimized.

Copy link

commented May 23, 2019

Just to make sure, this finally fixes #15323, right?

50Wliu added some commits May 30, 2019

rafeca added some commits Jun 15, 2019

Use random port to initialize ChromeDriver
Since ChromeDriver v2.41, ChromeDriver will only connect if, either we precise a port
for remote debugging, either the embedder (ie electron) made sure to pass `USER_DATA_DIR`
to the remote debugging server.
So, for now, we'll just use a random port (we don't care about its value since we're not
connecting through it).

(inspired by electron-userland/spectron@737db13).
@rafeca rafeca referenced this pull request Jun 25, 2019
0 of 8 tasks complete
@rafeca

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

General testing

  • Review issues tagged with electron to see if any of them are fixed
  • Review any comment in the Atom code that contains the keyword TodoElectronIssue to see if any of them can be resolved (search link).
  • Verify that you can collaborate with Teletype
    • Host using Atom's current stable release can collaborate with guest using Atom with the new version of Electron
    • Guest using Atom's current stable release can collaborate with host using Atom with the new version of Electron
    • Host and guest can collaborate when both are using Atom with the new version of Electron
    Steps 1. Open a file 1. Share a portal 1. Have a guest join the portal 1. When guest edits the text in the file, verify that the host sees those changes

Check for regressions experienced in previous upgrades

Text Input/Keybindings

See how to setup keyboard layouts.

  • IME not working (#14911)

    • macOS
    • Windows 10
    • Ubuntu
    Repro steps
    1. Install an IME layout, for example Pinyin simplified
    2. Open Atom and use the layout you installed as the keyboard layout
    3. Example, if you picked Pinyin simplified type in zhongwenshuru

    Expected: in Pinyin simplified 中文输入 is expected

    gif

    Issues we've seen: Only shows latin first character and not every character typed.

    gif

  • macOS: Key binding with composition characters are ignored (#15189)

    Repro steps
    1. Open your keymap.cson file and add the following:
      'atom-text-editor':
        # use hjkl; vim-like when pressing alt (word by word, paragraph by paragraph)
        # or alt (letter by letter)
        'alt-h': 'core:move-left'
        'alt-j': 'core:move-down'
        'alt-k': 'core:move-up'
        'alt-l': 'core:move-right'
    2. Save keymap.cson and reload atom.
    3. Try the new keymappings under ABC - Extended keyboard layout

    Expected: the keybindings work

    Issues we've seen: it types out the modified keys and ignores the key mapping

  • macOS: Composition characters mess up insertion point (#15344)

    Repro steps
    1. Open keymap.cson and add the following:
      'atom-text-editor':
        'alt-n': 'core:move-down'
        'alt-o': 'core:move-down'
    2. Have the following in your buffer:
      Foo
      Bar
      
    3. Have the cursor at the beginning of file(before F character) Type alt-n, then arrow right, then insert a character. Observe that the character is inserted at the beginning of the line (where the alt-n left you).
    4. Repeat steps using alt-o instead of alt-n.

    Expected: Both alt-o and alt-n behave the same

    Issues we've seen:
    bug

  • Ubuntu: Keystrokes involving ctrl resolve to the default layout instead of the active layout (#13170)

    Repro steps

    This should be tested on Linux with gnome

    1. Install a Linux ditro with gnome, for example Ubuntu
    2. Install French AZERTY and English US QWERTY layouts
    3. Change the default layout to be French AZERTY
    4. Open Atom and switch to English US QWERTY layout
    5. Press ctrl-w

    Expected: core:close to dispatch (Or the keybinding-resolver to resolve to ctrl-w). Should be resolving to the keyboard layout that is chosen and not the OS default layout.

    Issues we've seen: core:undo dispatched because it was resolved as ctrl-z because AZERTY has Z where W is on QWERTY.

  • Other keyboard layouts on new Electron version

UI

  • tree-view drag image (atom/tree-view#1054)

    • macOS
    • Windows 10
    • Ubuntu
    Repro steps
    1. Open a project in Atom with multiple files and a few folders
    2. Drag a file into a folder

    Expected:
    image

    Issues we've seen:

    image

  • drag-and-drop indicator (atom/tree-view#1055, atom/tabs#426, atom/tabs#437)

    • macOS
    • Windows 10
    • Ubuntu
    Repro steps
    1. Open Atom with a few folder projects on tree-view (you can do this on terminal in a directory that has existing folder and having a space between each folder. Example: atom folderA folderB folderC)
    2. Drag and drop all of the folders to reorder them on Atom

    Expected:
    The placeholder indicates where the folder/project is being dropped to.

    screen shot 2017-03-29 at 16 31 16

    Issues we've seen:
    No placeholder shows up after drag and dropping

    screen shot 2017-03-29 at 16 30 51

  • Large file rendering (#16591)

    • macOS
    • Windows 10
    • Ubuntu
    Repro steps
    1. Open a file that has over one million lines
    2. Scroll down to the bottom

    Expected: To not regress the number of lines that can be rendered. Rendering to be correct for lines past a certain point. Atom 1.25 can render around 800k to 1 million lines correctly.

    Issues we've seen: Increased number of lines rendered but bad rendering past a certain point

    bad renderer

  • Loss of subpixel AA when the cursor is at the end of long lines (#16889, #16595)

    • macOS
    • Windows 10
    • Ubuntu
    Repro steps
    1. Open text-editor.js in Atom
    2. Copy the first 21 lines to a new untitled window
    3. Turn on soft wrapping and change language mode to javascript
    4. Resize the Window so it's soft wrapped
    5. On line 15 type a until the you reach the end of the window

    Expected: To not lose subpixel AA

    Issues we've seen: Loss of subpixel AA. Both when soft wrapping was enabled and disabled.

    subpixel aa loss softwrap

  • Scrolling horizontally shift + scroll wheel (#12696 (comment))

    • macOS
    • Windows 10
    • Ubuntu
    Repro steps
    1. Open a file that has long lines
    2. Turn off soft wrapping and resize the window so you have a horizontal scrollbar
    3. Press shift and scroll in both directions using the mouse wheel

    Expected: File to scroll horizontally in both directions

    Issues we've seen: File does not scroll horizontally at all in any direction

  • Scrollbars misbehaving on the first file that is opened (#12696 (comment))

    Repro steps
    1. Open Atom
    2. Open some file that has content enough to have a vertical scrollbar
    3. Close Atom
    4. Open Atom and look at the vertical scrollbar

    Expected: Scrollbar to be visible

    Issues we've seen: Scrollbar is not visible and is flickering when you are editing

  • Middle clicking on unsaved tab (#15197)

    • macOS
    • Ubuntu
    Repro steps
    1. Open Atom and open a new unsaved file.
    2. Make edits to the unsaved file and middle click on the tab.

    Expected:
    Clicking save/cancel or the options on the dialog works.

    Issues we've seen:
    The UI and the dialog is unable to receive mouse clicks. You can still choose options via Keyboard, but not mouse.

  • Linux: Atom scrolls even when not focused (#15482)

    Repro steps
    1. Open Atom with a large enough file that has a scrollbar
    2. Open a different application and place it over the Atom window
    3. Scroll with the mouse inside the other window
    4. Focus the Atom window

    Expected: Atom window should keep the original scroll position

    Issues we've seen: The Atom window scrolls after it is focused

Other

  • Deprecation warnings (#12696 (comment))

    • macOS
    • Windows 10
    • Ubuntu
    Repro steps

    This should be tested on macOS, Linux and Windows with community packages installed

    1. Open Atom
    2. Open the Developer tools console on the master branch and check for deprecation warnings
    3. Open the Developer tools console on the electron upgrade branch and check for deprecation warnings
    4. Compare the two outputs

    Expected: No new deprecation warnings

    Issues we've seen: New deprecation warnings both from core and community packages

  • Supported Versions of OS (#15297)

    • macOS
    • Windows 10
    • Ubuntu
    Repro steps
    1. Check the list of supported platforms Electron Master
    2. Check what version is listed as minimum for each platform on https://atom.io and https://atom.io/faq

    Expected: https://atom.io and https://atom.io/faq to display the same versions as the Electron documentation

    Issues we've seen: macOS version support changed and https://atom.io and https://atom.io/faq was not updated

  • macOS : Native tabs (#14711)

    Repro steps
    1. Open two atom windows on mac
    2. Go to View in menus and check Show Tab Bar
      screen shot 2017-06-03 at 5 10 32 pm
    3. Do this for both Atom windows.
    4. Drag one atom window to the other via tab bar

    Expected:
    Native Tab bar shows up and able to drag different windows to each other. Also make sure UI isn't messed up.

    Issues we've seen:
    Native Tab Bar doesn't work and messes up UI when enabled

  • Ubuntu with KDE: Menu uses 100% CPU (electron/electron#8455, #13885)

    Repro steps
    1. Install Atom on Ubuntu KDE. [instructions]
    2. Enable KDE plasma global menu: Right-click on the desktop -> Add Panel => Application Menu Bar. [demo]
    3. Start Atom.
    4. Try to use the desktop menu.

    Expected:
    Desktop menu items work

    Issues we've seen:
    Desktop menu items do nothing when you click on things.

  • macOS: Slovak QWERTZ (atom/atom-keymap#223)

    Repro steps
    1. Install Slovak keyboard layout on mac
    2. Open keyboard viewer to see the keys are are suppose to show up and open keyboard resolver on atom.
    3. hit cmd+'.

    Expected: Resolves to cmd+' like it shows on keyboard viewer on mac.

    Issues we've seen: It resolves to ctrl+§

  • macOS: IME jump (#15696)

    Repro steps
    1. Install an IME layout, for example Chinese pinyin
    2. Type one character

    Expected:
    Expected the IME to be positioned at under the character you entered
    Issues we've seen: The IME window is in the top left corner. When you enter the second character it jumps to be positioned under the text

@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

@rafeca: Since Electron 4 drops support for macOS 10.9, is there something we can do to ensure that Atom users on macOS 10.9 don't get auto-upgraded to this incompatible version of Electron?

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

@rafeca: Since Electron 4 drops support for macOS 10.9, is there something we can do to ensure that Atom users on macOS 10.9 don't get auto-upgraded to this incompatible version of Electron?

I have no idea, maybe @ckerr , @jkleinsc or @BinaryMuse have some suggestions here, since other Electron apps that upgraded to v4 have probably experienced this same problem.

What we can do now is to start showing now banners on Atom running on OSX 10.9 informing users that we're going to drop support for this operating system very soon (maybe we can add this banner to v1.39 or v1.40).

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

Regarding blocking 10.9 users from upgrading, this seems to be possible to do only by our atom.io backend system, which is the one that returns the potential list of versions to upgrade (This is based on a Slack conversation with @jkleinsc).

client side changes

In order to do so, Atom needs to pass the OS version to atom.io when it asks if there are new versions. This is a very easy change which can be added in this line.

In order to make everything work, though, we need to do this change at least one the version before we ship Electron v4, so older clients that do not need to get updated are already sending the OS version.

My suggestion is to make these changes as soon as possible and cherry-pick them to the current beta to get them to stable soon.

atom.io changes

Then, we need to change atom.io logic to take into account the OS version. This is going to be trickier to be able to also deal correctly with clients having much older versions of Atom.

In order to explain the logic a little bit clearer, I'm going to assume that the new parameter is added on Atom v1.40 and Electron v4 is added on Atom v1.42:

  1. If the client does not pass the os_version param (old client), atom.io will return the latest v1.40.x version.
  2. If the client passes os_version=10.9 (and is a mac), atom.io will return the latest v1.41.x version.
  3. If the client passes any other os_version param (or it's not a mac), atom.io will return the latest version (as it does now).

The same logic should be applied to each release channel (stable, beta and nightly).

Good thing is that if we build this logic in a configurable way, later it's going to be trivial to block e.g older Windows versions from upgrading if Electron drops support for them.

The atom.io changes can wait until we merge this branch (then we'll want to have them to prevent nightly users from upgrading).

@jasonrudolph thoughts?

@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

@rafeca: That approach sounds good to me. 👍

@nathansobo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

This plan makes sense to me @rafeca.

One thing worth noting... Unless there's an API I don't know about, I think the best we'll be able to do is for determining the OS version is require("os").release(). That reports different version numbers than the 10.x used in Apple's marketing. Based on this table of macOS releases, it looks like 10.9 corresponds to a release number of 13.0.0 and 10.10 corresponds to 14.0.0.

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

While doing the regression testing I found an issue related to native tabs: when I enable them, doing operation with tabs becomes super slow and makes the editor unresponsive for ~5-10s. Same happens when doing operations with the native tabs (like merging different windows into a single window or splitting tabs into multiple windows).

I've created an issue to track it: #19611

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

Ok, it looks like all the identified blockers are solved. In order to reduce the risk we're going to wait to merge this PR until just after the Atom Railcar Release for 1.39 is done (which is expected on July 19th).

Until then, I'll be using a version of Atom built from this PR to see if I can find any other regression.

@rafeca rafeca marked this pull request as ready for review Jul 5, 2019

@@ -12,7 +12,7 @@
"url": "https://github.com/atom/atom/issues"
},
"license": "MIT",
"electronVersion": "3.1.10",
"electronVersion": "4.2.5",

This comment has been minimized.

Copy link
@mfonville

mfonville Jul 6, 2019

Contributor

4.2.6 is also available

This comment has been minimized.

Copy link
@mfonville

mfonville Jul 16, 2019

Contributor

and 4.2.7

This comment has been minimized.

Copy link
@mfonville

mfonville Jul 18, 2019

Contributor

@rafeca I guess the 4.2.7 assets were not live yet when you tried to push the update

This comment has been minimized.

Copy link
@rafeca

rafeca Jul 19, 2019

Contributor

Yup, I tried 4.2.7 and it didn't work well (the release notes were not even published). I'm gonna upgrade to 4.2.7 now

@rafeca rafeca force-pushed the electron-4 branch from c9a290a to 79a952b Jul 16, 2019

@mfonville

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

Will Electron 4 be merged for the 1.41-dev branch?

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

Will Electron 4 be merged for the 1.41-dev branch?

Yup, I'm going to merge this PR once the CI passes. Since we've just rolled the railcars for v1.40.0, Electron v4 going to be available on v1.41 (probably from v1.41.0-nightly1).

@rafeca rafeca merged commit e0a377e into master Jul 19, 2019

2 checks passed

Atom Production Branches #1.41.0-dev+45805 succeeded
Details
Atom Pull Requests #20190719.1 succeeded
Details

@rafeca rafeca deleted the electron-4 branch Jul 19, 2019

@Arcanemagus Arcanemagus referenced this pull request Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.