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

Electron 6 Take Two #21079

Merged
merged 47 commits into from Aug 20, 2020
Merged

Conversation

lkashef
Copy link
Contributor

@lkashef lkashef commented Jul 16, 2020

Original Issue: #20764

TODO

  • Update the CI with the proper node and npm versions
  • Update electron version
  • Update electron dependencies (electron-chromedriver and electron-mksnapshot) to match the electron version
  • Get the app to bootstrap on the new electron version
  • Once the build job passes make sure the artifacts are runnable/installable
  • Fix any failing tests and make sure the test jobs are passing on the CI
  • Test the artifacts against the regression checklist below
  • Check performance issue after upgrade on Linux (Check Slow UI and busy GPU thread with Electron 6 on Linux (with Intel CPU and no GPU?) #20924 and the conversation below)

BLOCKERS AND ISSUES

  • fix scrollbar-style compilation problems with node 12.4.0
  • fix @atom/watcher compilation problems with node 12.4.0
  • Minimum supported macos version is 10.10 and not 10.9
    • Update atom.io download page for beta and stable -- apparently nightly has been already updated (PR)
    • Update flight manual (PR)
    • Unblock flight manual CI (PR)
  • Last release we agreed that we will not check for deprecation warnings, so I updated the template to reflect that (PR)
  • Linux: Atom scrolls even when not focused (#15482) is a regression since electron 3, will update the template to keep to check during regression testing once it's fixed
  • It seems that find-and-replace in find-in-project is not working, even though tests are passing, check if others can reproduce this Replace All function is broken find-and-replace#1129
  • Settings page for packages doesn't scroll anymore https://github.com/atom/atom/issues/21094

Regression Checklist

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).
    • The changes seems to be just a bit of code enhancement that I am not sure it's a top priority for now
  • 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 (1.19.0-beta2: Cursor stucks at the first letter when using macOS Chinese IME #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 (key-mappings with alt don't suppress Mac OS composition characters anymore (atom-1.19) #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 (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 (Keystrokes involving ctrl on Linux 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 (Display the correct drag image on Electron >= 1.14 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 (Fix missing drop indicator on Electron >= 1.14 tree-view#1055, Fix missing drop indicator on Electron >= 1.4 tabs#426, Fix mistakenly shown docks drop indicator on Electron >= 1.4 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 (Bad rendering after ~1 million lines #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 (Loss of subpixel AA on soft-wrapped line #16889, Don't break subpixel AA when cursor is at the end of longest line #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 (Upgrade Electron to v1.6.x #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 (Upgrade Electron to v1.6.x #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 (1.19, Linux: Middle-clicking an unsaved tab causes entire desktop to be unresponsive to clicks #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 (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
    Note: this is a regression that has not been fixed since Electron 3, will leave it in the template to keep it on our radar once it's fixed

Other

@aminya
Copy link
Contributor

aminya commented Jul 22, 2020

Thank you for the recent updates. Both find and replace and scrolling in the settings view issues are gone.

I tested this with the artifacts from @DeeDeeG. This hidden issue does not happen on those. Maybe something is wrong with my working branch.

However, now I cannot search for packages for downloading in the settings view. Does this happen for others or it is just me? I built this on a branch that had other changes, so I am not certain if it is caused by those two updates.

image

@darangi
Copy link
Contributor

darangi commented Jul 22, 2020

Thank you for the recent updates. Both find and replace and scrolling in the settings view issues are gone.

I tested this with the artifacts from @DeeDeeG. This hidden issue does not happen on those. Maybe something is wrong with my working branch.

Yea, I also confirmed it works fine on this build. Thanks for the effort.

aminya
aminya approved these changes Jul 23, 2020
Copy link
Contributor

@aminya aminya left a comment

Thank you so much for this work.

@aminya
Copy link
Contributor

aminya commented Jul 28, 2020

Let us know if there is anything blocking this from merging. We may be able to help.

@aminya
Copy link
Contributor

aminya commented Aug 10, 2020

Could you check my comment at atom/apm#889 (comment)

I am seeing some issues with apm. The Node version difference makes the native modules that are compiled using apm unusable. We should make the Node versions identical on apm and atom.

I reproduced the error with the binaries uploaded in the CI of this PR. You can reproduce the error by installing https://atom.io/packages/ink

@jeff-hykin
Copy link

jeff-hykin commented Aug 11, 2020

On a similar note @aminya , could the version of node being used be added to the package.json dependencies? (E.g. npm install -s node)

The atom build fails with newer versions of node because of native modules, but the version of node isn't mentioned in the documentation or repo AFAIK

@aminya
Copy link
Contributor

aminya commented Aug 12, 2020

On a similar note @aminya , could the version of node being used be added to the package.json dependencies? (E.g. npm install -s node)

The atom build fails with newer versions of node because of native modules, but the version of node isn't mentioned in the documentation or repo AFAIK

Yes, we should unify the Node versions and write it down in the package.json.

If for some reason, the old Electron was not picky about the Node versions, the new one is, and we should absolutely make this unification before merging this pull request.

@aminya
Copy link
Contributor

aminya commented Aug 19, 2020

@darangi apm 2.5.1 solves the issue for Electron 5. You should merge this as well: atom/apm#896

@darangi darangi merged commit 50a74a5 into master Aug 20, 2020
2 checks passed
@the-j0k3r
Copy link

the-j0k3r commented Aug 20, 2020

Congrats on the merge =) and thanks for the work on these electron bumps

@the-j0k3r
Copy link

the-j0k3r commented Aug 25, 2020

Please dont forget #20819 is fixed with electron 6.x merged

@sadick254 sadick254 mentioned this pull request Sep 7, 2020
sadick254 added a commit that referenced this pull request Sep 9, 2020
We have been experiencing linux build failures on our CI.
This failure seem to be cause by
#21079.

Related conversation atom/apm#900
sadick254 added a commit that referenced this pull request Sep 9, 2020
We have been experiencing linux build failures on our CI.
This failure seem to be cause by
#21079.

Related conversation atom/apm#900
sadick254 added a commit that referenced this pull request Sep 9, 2020
We have been experiencing linux build failures on our CI.
This failure seem to be cause by
#21079.

Related conversation atom/apm#900
@sadick254 sadick254 deleted the electron-6.1.12-bump-electron-packager-from-git branch Oct 25, 2021
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.

None yet

7 participants