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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade Electron to v1.6.x #12696

Merged
merged 407 commits into from May 19, 2017

Conversation

Projects
None yet
@thomasjo
Member

thomasjo commented Sep 15, 2016

Reported Issues

Reported issues resolved by #13880:

  • Scrollbars misbehaving on first file that is opened/restored #12696 (comment)
  • Shift-scrolling scrolls vertically instead
  • Failing spec/text-editor-component-spec.js tests

During my initial testing on macOS 10.12, this seems to be working. Haven't noticed any regressions.
Would be great if more folks could give this a spin 馃檱

/cc @atom/maintainers


Fixes #11692
Resolves #12690
Fixes #8006
Fixes #12832
Fixes #13767
Fixes #4084 (Will be fixed by the editor rendering rewrite)
Fixes #13265 (Will be fixed by the editor rendering rewrite)

@thomasjo thomasjo changed the title from Upgrade Electron to v1.4.0 to Upgrade Electron to v1.4.1 Sep 22, 2016

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Sep 22, 2016

Member

Since Electron v1.4.1 ships with electron/electron#7209, I hope this PR will resolve #11692.

Member

thomasjo commented Sep 22, 2016

Since Electron v1.4.1 ships with electron/electron#7209, I hope this PR will resolve #11692.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 2, 2016

Member

Observations from around 10 minutes of light usage:

NOTHING IS BLURRY ANYMORE!

I have beautiful subpixel antialiasing on everything, even the actual code for the first time ever* 馃挜. Fixes #12652, maybe #7261, #3869, #2833.

Scrollbars seem to disappear randomly. Haven't narrowed this down but I did notice one of my files didn't have a scrollbar.

* Just noticed that the tabs and mini-editors don't have subpixel AA, but that's super minor compared to before.

Member

50Wliu commented Oct 2, 2016

Observations from around 10 minutes of light usage:

NOTHING IS BLURRY ANYMORE!

I have beautiful subpixel antialiasing on everything, even the actual code for the first time ever* 馃挜. Fixes #12652, maybe #7261, #3869, #2833.

Scrollbars seem to disappear randomly. Haven't narrowed this down but I did notice one of my files didn't have a scrollbar.

* Just noticed that the tabs and mini-editors don't have subpixel AA, but that's super minor compared to before.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 2, 2016

Member

Ok, something else related to subpixel AA:
If I scroll all the way to the end of the file or use the scrollbar, I lose subpixel AA in the editor until I switch tabs.

Member

50Wliu commented Oct 2, 2016

Ok, something else related to subpixel AA:
If I scroll all the way to the end of the file or use the scrollbar, I lose subpixel AA in the editor until I switch tabs.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Oct 3, 2016

Member

Scrollbars seem to disappear randomly. Haven't narrowed this down but I did notice one of my files didn't have a scrollbar.

Have this happening with the vertical scrollbar on the TextEditor active when restoring session. In the case of split panes (I tried left/right) both TextEditors are missing vertical scrollbars.

  1. Open Atom 1.12-dev-fcddf83 open some file that has content enough to have a vertical scrollbar.
  2. Close Atom.
  3. Open Atom 1.12-dev-fcddf83 and keep an eye on the vertical scrollbar. It appears quickly when opening the previous session and then disappears.

If you resize the pane so that the horizontal scrollbar appears or disappears then you can get the vertical scrollbar to appear, but in all other cases this file seems to be missing the vertical scrollbar.

Tested with both Atom Light and One Dark themes. This does not reproduce in 1.11-beta5. Don't know about other dev versions.

Member

Ben3eeE commented Oct 3, 2016

Scrollbars seem to disappear randomly. Haven't narrowed this down but I did notice one of my files didn't have a scrollbar.

Have this happening with the vertical scrollbar on the TextEditor active when restoring session. In the case of split panes (I tried left/right) both TextEditors are missing vertical scrollbars.

  1. Open Atom 1.12-dev-fcddf83 open some file that has content enough to have a vertical scrollbar.
  2. Close Atom.
  3. Open Atom 1.12-dev-fcddf83 and keep an eye on the vertical scrollbar. It appears quickly when opening the previous session and then disappears.

If you resize the pane so that the horizontal scrollbar appears or disappears then you can get the vertical scrollbar to appear, but in all other cases this file seems to be missing the vertical scrollbar.

Tested with both Atom Light and One Dark themes. This does not reproduce in 1.11-beta5. Don't know about other dev versions.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 3, 2016

Member

The disappearing scrollbar issue appears to happen on the first file that is opened/restored. From the dev tools, the scrollbar (and scrollbar corner) is being given a width of 10px instead of the usual 15px. If I change it to 15, then the scrollbar appears. Anything > 10 works. /cc @simurai
I've also been seeing a related issue where the scrollbar flickers like crazy when moving the mouse between the tab bar and the editor. I'll try to find a stable repro case for that, but it also happens on the first file.

Now, some more subpixel antialiasing updates. As I said before, it works for the most part. Here's when it fails:

  1. In a mini-editor
  2. (Almost) anywhere with rendered text (eg Settings View, Tabs, Notifications, About). The tree view and status bar are fine though?
  3. Upon reaching the end of the file
  4. Upon scrolling with a touchpad or scrollbar. Mousewheel scrolling is fine.
  5. Upon editing the last three visible lines, causing the editor to scroll and triggering case 4).
  6. Dev tools

Also, Atom occasionally crashes when I try to close it. I'm not sure if that is specific to the Electron upgrade though as it's happened before as well.

Member

50Wliu commented Oct 3, 2016

The disappearing scrollbar issue appears to happen on the first file that is opened/restored. From the dev tools, the scrollbar (and scrollbar corner) is being given a width of 10px instead of the usual 15px. If I change it to 15, then the scrollbar appears. Anything > 10 works. /cc @simurai
I've also been seeing a related issue where the scrollbar flickers like crazy when moving the mouse between the tab bar and the editor. I'll try to find a stable repro case for that, but it also happens on the first file.

Now, some more subpixel antialiasing updates. As I said before, it works for the most part. Here's when it fails:

  1. In a mini-editor
  2. (Almost) anywhere with rendered text (eg Settings View, Tabs, Notifications, About). The tree view and status bar are fine though?
  3. Upon reaching the end of the file
  4. Upon scrolling with a touchpad or scrollbar. Mousewheel scrolling is fine.
  5. Upon editing the last three visible lines, causing the editor to scroll and triggering case 4).
  6. Dev tools

Also, Atom occasionally crashes when I try to close it. I'm not sure if that is specific to the Electron upgrade though as it's happened before as well.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 4, 2016

Member

More comments.

I appear to lose subpixel antialiasing during any type of scrolling (programmatically through go-to-line or find-and-replace, for example, or manually) except for mousewheel scrolling.

The dev tools are frozen after reloading the window and must be closed through the keybinding and reopened in order for it to work.

Member

50Wliu commented Oct 4, 2016

More comments.

I appear to lose subpixel antialiasing during any type of scrolling (programmatically through go-to-line or find-and-replace, for example, or manually) except for mousewheel scrolling.

The dev tools are frozen after reloading the window and must be closed through the keybinding and reopened in order for it to work.

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Oct 4, 2016

Contributor

Chrome tends to switch from sub-pixel to regular AA when it expects to need to redraw it several times such as in animation. This is because AA can be cached and re-used whereas sub-pixel depends on the horizontal offset.

Contributor

damieng commented Oct 4, 2016

Chrome tends to switch from sub-pixel to regular AA when it expects to need to redraw it several times such as in animation. This is because AA can be cached and re-used whereas sub-pixel depends on the horizontal offset.

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Oct 4, 2016

Member

The scrollbar issue is super weird. Changing the width to anything other than the original (10px) works.. but when changing bottom it alternates. See the sub-pixel (0.1px) steps.

scrollbars

It seems any property that triggers a layout reflow makes the scrollbar visibility alternate when changing the value:

scrollbars

And just typing in search + replace alternates the horizontal scrollbar:

scrollbars

ps. on macOS, this issue only happens if the "Show scroll bars" is set to "Always". Otherwise (where the scroll bars hide automatically) it's fine.

Member

simurai commented Oct 4, 2016

The scrollbar issue is super weird. Changing the width to anything other than the original (10px) works.. but when changing bottom it alternates. See the sub-pixel (0.1px) steps.

scrollbars

It seems any property that triggers a layout reflow makes the scrollbar visibility alternate when changing the value:

scrollbars

And just typing in search + replace alternates the horizontal scrollbar:

scrollbars

ps. on macOS, this issue only happens if the "Show scroll bars" is set to "Always". Otherwise (where the scroll bars hide automatically) it's fine.

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Oct 6, 2016

Member

Another thing I noticed. The native tabs in macOS Sierra seem to work in this Electron version. Problem with the custom title bar is that the native tabs are just on top and don't push the rest down. Also, hard to read with dark themes.

screen shot 2016-10-06 at 4 06 06 pm

Native title bar works fine.

Steps to reproduce:

  1. Enable Atom Settings > Core > Use Custom Title Bar.
  2. Switch macOS Prefereces > Dock > "Prefer tabs when opening documents" to "Always":
    screen shot 2016-10-06 at 4 02 36 pm
  3. Open another window with cmd-o or View > Show Tab Bar.

Possible solution

a. Either disable native tabs when custom title bar is enabled.
b. See if it's possible to detect when native tabs get added and then push the content down.

Maybe not that urgent, since you have to enable two things to run into this. Could also be a separate issue.

Update:

Fixed with Electron v1.4.2.

Member

simurai commented Oct 6, 2016

Another thing I noticed. The native tabs in macOS Sierra seem to work in this Electron version. Problem with the custom title bar is that the native tabs are just on top and don't push the rest down. Also, hard to read with dark themes.

screen shot 2016-10-06 at 4 06 06 pm

Native title bar works fine.

Steps to reproduce:

  1. Enable Atom Settings > Core > Use Custom Title Bar.
  2. Switch macOS Prefereces > Dock > "Prefer tabs when opening documents" to "Always":
    screen shot 2016-10-06 at 4 02 36 pm
  3. Open another window with cmd-o or View > Show Tab Bar.

Possible solution

a. Either disable native tabs when custom title bar is enabled.
b. See if it's possible to detect when native tabs get added and then push the content down.

Maybe not that urgent, since you have to enable two things to run into this. Could also be a separate issue.

Update:

Fixed with Electron v1.4.2.

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Oct 6, 2016

Member

@simurai Electron v1.4.2 disabled the native tabs support, so this shouldn't be an issue anymore 馃幈

Member

thomasjo commented Oct 6, 2016

@simurai Electron v1.4.2 disabled the native tabs support, so this shouldn't be an issue anymore 馃幈

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Oct 7, 2016

Member

馃檲 Drag image 馃檲

dragimage

Member

Ben3eeE commented Oct 7, 2016

馃檲 Drag image 馃檲

dragimage

@v3ss0n

This comment has been minimized.

Show comment
Hide comment
@v3ss0n

v3ss0n Oct 12, 2016

electron 1.x is coming in atom 1.12?

v3ss0n commented Oct 12, 2016

electron 1.x is coming in atom 1.12?

@thomasjo thomasjo changed the title from Upgrade Electron to v1.4.1 to Upgrade Electron to v1.4.3 Oct 12, 2016

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 12, 2016

Member

Correct.

Member

50Wliu commented Oct 12, 2016

Correct.

@thomasjo thomasjo changed the title from Upgrade Electron to v1.4.3 to Upgrade Electron to v1.4.x Oct 21, 2016

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 21, 2016

Member

Fixes #12832.

Member

50Wliu commented Oct 21, 2016

Fixes #12832.

@Palabola

This comment has been minimized.

Show comment
Hide comment
@Palabola

Palabola Nov 14, 2016

Blurry Text still exist under Windows 7 even with the newest Electron!

Blurry Text still exist under Windows 7 even with the newest Electron!

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Nov 15, 2016

Member

Fixes #13104. Already fixed via #13247

Member

Ben3eeE commented Nov 15, 2016

Fixes #13104. Already fixed via #13247

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Nov 15, 2016

Member

Seems the drop marker is missing again:
drop marker
drop marker 2

Member

Ben3eeE commented Nov 15, 2016

Seems the drop marker is missing again:
drop marker
drop marker 2

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 15, 2016

Contributor

It would be helpful if we could assemble a list of issues that need to be resolved before we can merge this in the body of the issue.

Contributor

nathansobo commented Nov 15, 2016

It would be helpful if we could assemble a list of issues that need to be resolved before we can merge this in the body of the issue.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Nov 15, 2016

Member

Updated the body with the issues I could find. @50Wliu if you could also check that would be great.

@50Wliu I can reproduce the scrollbar flickering issue when hovering over the tab bar. There have been a few reports of various things flickering when moving the mouse over the tab bar since 1.11. Maybe we do something strange?

Member

Ben3eeE commented Nov 15, 2016

Updated the body with the issues I could find. @50Wliu if you could also check that would be great.

@50Wliu I can reproduce the scrollbar flickering issue when hovering over the tab bar. There have been a few reports of various things flickering when moving the mouse over the tab bar since 1.11. Maybe we do something strange?

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Nov 15, 2016

Contributor

@Palabola can you attach a screenshot showing what you mean by blurry text?

Contributor

damieng commented Nov 15, 2016

@Palabola can you attach a screenshot showing what you mean by blurry text?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Nov 15, 2016

Member

I've removed the dev tools freezing since I can't reproduce it anymore.

Member

50Wliu commented Nov 15, 2016

I've removed the dev tools freezing since I can't reproduce it anymore.

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Nov 16, 2016

Member

I've narrowed down the problem with the missing drop indicators to being some problem related to DataTransfer.

The values set during the dragstart event via event.dataTransfer.setData(format, value) are empty strings during in the dragover event via event.dataTransfer.getData(format).

Haven't yet figured out what has changed in Chromium and how to fix it. Quick, cursory search did not reveal any obvious Chromium bugs.

/cc @Ben3eeE @atom/maintainers


Relevant spec:
https://developers.whatwg.org/dnd.html#dragevent

Member

thomasjo commented Nov 16, 2016

I've narrowed down the problem with the missing drop indicators to being some problem related to DataTransfer.

The values set during the dragstart event via event.dataTransfer.setData(format, value) are empty strings during in the dragover event via event.dataTransfer.getData(format).

Haven't yet figured out what has changed in Chromium and how to fix it. Quick, cursory search did not reveal any obvious Chromium bugs.

/cc @Ben3eeE @atom/maintainers


Relevant spec:
https://developers.whatwg.org/dnd.html#dragevent

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Nov 17, 2016

Member

Got this today when on this branch in dev mode, Windows 7 x64. Electron 1.4.6.

image

Member

Ben3eeE commented Nov 17, 2016

Got this today when on this branch in dev mode, Windows 7 x64. Electron 1.4.6.

image

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Nov 18, 2016

Member

Another object of undefined error today when reloading the Atom window. I was fooling around with my init.coffee writing keystroke api code. Previous was when tabbing out of Atom. This time with Electron 1.4.7.

image

Member

Ben3eeE commented Nov 18, 2016

Another object of undefined error today when reloading the Atom window. I was fooling around with my init.coffee writing keystroke api code. Previous was when tabbing out of Atom. This time with Electron 1.4.7.

image

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 18, 2016

Contributor

@Ben3eeE I've added the object undefined errors to the list in the issue body.

Contributor

nathansobo commented Nov 18, 2016

@Ben3eeE I've added the object undefined errors to the list in the issue body.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Jan 10, 2017

Member

@damieng just a heads up, Electron 1.4.14 contains the normalizeAccessKeys option from electron/electron#8308

Member

kevinsawicki commented Jan 10, 2017

@damieng just a heads up, Electron 1.4.14 contains the normalizeAccessKeys option from electron/electron#8308

@damieng damieng referenced this pull request Jan 26, 2017

Closed

Changed File Close Dialogue not usable with keyboard #2928

3 of 4 tasks complete
@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Feb 20, 2017

Member

Two errors when testing latest artifact here. I don't see these in 1.15.0-beta3.

Failed to activate the status-bar package
contentWidth is not defined
Hide Stack Trace
ReferenceError: contentWidth is not defined
    at AtomClockView.adjustElementSize (atom-clock-view.js:97:5)
    at AtomClockView.initialize (atom-clock-view.js:21:10)
    at AtomClockView.start (atom-clock-view.js:14:10)
    at Object.consumeStatusBar (atom-clock.js:45:24)
    at Provider.module.exports.Provider.provide (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\node_modules\service-hub\lib\provider.js:29:52)
    at ServiceHub.module.exports.ServiceHub.provide (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\node_modules\service-hub\lib\service-hub.js:30:20)
    at Package.module.exports.Package.activateServices (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package.js:396:71)
    at Package.module.exports.Package.activateNow (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package.js:221:16)
    at C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package.js:190:32
    at Package.module.exports.Package.measure (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package.js:96:15)
    at C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package.js:183:26
    at Package.module.exports.Package.activate (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package.js:180:34)
    at PackageManager.module.exports.PackageManager.activatePackage (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package-manager.js:550:34)
    at C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package-manager.js:531:29
    at Config.module.exports.Config.transactAsync (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\config.js:337:18)
    at PackageManager.module.exports.PackageManager.activatePackages (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package-manager.js:526:19)
    at PackageManager.module.exports.PackageManager.activate (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package-manager.js:508:46)
    at C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\atom-environment.js:812:28
Uncaught TypeError: atom.views.pollDocument is not a function  C:\Users\lineri\.atom\packages\minimap\lib\minimap-element.js:345 
Member

Ben3eeE commented Feb 20, 2017

Two errors when testing latest artifact here. I don't see these in 1.15.0-beta3.

Failed to activate the status-bar package
contentWidth is not defined
Hide Stack Trace
ReferenceError: contentWidth is not defined
    at AtomClockView.adjustElementSize (atom-clock-view.js:97:5)
    at AtomClockView.initialize (atom-clock-view.js:21:10)
    at AtomClockView.start (atom-clock-view.js:14:10)
    at Object.consumeStatusBar (atom-clock.js:45:24)
    at Provider.module.exports.Provider.provide (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\node_modules\service-hub\lib\provider.js:29:52)
    at ServiceHub.module.exports.ServiceHub.provide (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\node_modules\service-hub\lib\service-hub.js:30:20)
    at Package.module.exports.Package.activateServices (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package.js:396:71)
    at Package.module.exports.Package.activateNow (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package.js:221:16)
    at C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package.js:190:32
    at Package.module.exports.Package.measure (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package.js:96:15)
    at C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package.js:183:26
    at Package.module.exports.Package.activate (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package.js:180:34)
    at PackageManager.module.exports.PackageManager.activatePackage (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package-manager.js:550:34)
    at C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package-manager.js:531:29
    at Config.module.exports.Config.transactAsync (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\config.js:337:18)
    at PackageManager.module.exports.PackageManager.activatePackages (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package-manager.js:526:19)
    at PackageManager.module.exports.PackageManager.activate (C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\package-manager.js:508:46)
    at C:\Users\lineri\AppData\Local\atom\app-1.16.0-dev-49ec99d\resources\app.asar\src\atom-environment.js:812:28
Uncaught TypeError: atom.views.pollDocument is not a function  C:\Users\lineri\.atom\packages\minimap\lib\minimap-element.js:345 
@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii May 16, 2017

Member

Just saw a little lag on this branch, can anyone else repo? (filesize was too large so had to upload to Google Drive)

I couldn't reproduce the find-and-replace issue, but it might be worth to keep an eye on it and make sure it didn't regress with these changes. I am pretty sure it's not related to the editor rendering rewrite though.

Not too sure what happened or if this is isolated to the Electron upgrade but my Find menu looks like this now:

Tentative repro steps seem to be opening the menu through the alt-i accelerator.

Interesting, unfortunately this is not happening for me on macOS. Can you reproduce consistently? @Ben3eeE: if I recall correctly you use Windows as well. Can you reproduce this on your machine?

Also very likely related, but slightly different: If you try to take a CPU profile and while recording reload the Atom window you get an "Editor has crashed" dialog (and the same blank window).

I have noticed this one too lately, it might have something to do with the new Electron. Interestingly enough, reloading when DevTools are closed works perfectly. I'll file a bug report on Electron and see if we can fix this somehow.

Seeing some odd folding behavior.

This should be fixed as of the latest commit. Nice finding!

Member

as-cii commented May 16, 2017

Just saw a little lag on this branch, can anyone else repo? (filesize was too large so had to upload to Google Drive)

I couldn't reproduce the find-and-replace issue, but it might be worth to keep an eye on it and make sure it didn't regress with these changes. I am pretty sure it's not related to the editor rendering rewrite though.

Not too sure what happened or if this is isolated to the Electron upgrade but my Find menu looks like this now:

Tentative repro steps seem to be opening the menu through the alt-i accelerator.

Interesting, unfortunately this is not happening for me on macOS. Can you reproduce consistently? @Ben3eeE: if I recall correctly you use Windows as well. Can you reproduce this on your machine?

Also very likely related, but slightly different: If you try to take a CPU profile and while recording reload the Atom window you get an "Editor has crashed" dialog (and the same blank window).

I have noticed this one too lately, it might have something to do with the new Electron. Interestingly enough, reloading when DevTools are closed works perfectly. I'll file a bug report on Electron and see if we can fix this somehow.

Seeing some odd folding behavior.

This should be fixed as of the latest commit. Nice finding!

as-cii added some commits May 16, 2017

@Arcanemagus

This comment has been minimized.

Show comment
Hide comment
@Arcanemagus

Arcanemagus May 17, 2017

Contributor

Not too sure what happened or if this is isolated to the Electron upgrade but my Find menu looks like this now:

Just a note I couldn't reproduce this either (on Windows 10).

Contributor

Arcanemagus commented May 17, 2017

Not too sure what happened or if this is isolated to the Electron upgrade but my Find menu looks like this now:

Just a note I couldn't reproduce this either (on Windows 10).

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE May 18, 2017

Member

@Ben3eeE: if I recall correctly you use Windows as well. Can you reproduce this on your machine?

I am unable to reproduce on Windows 7.

Member

Ben3eeE commented May 18, 2017

@Ben3eeE: if I recall correctly you use Windows as well. Can you reproduce this on your machine?

I am unable to reproduce on Windows 7.

@50Wliu 50Wliu referenced this pull request May 18, 2017

Closed

Atom Dark Layout Issue on MacOS Sierra GM #12634

6 of 6 tasks complete
@kaiser101

This comment has been minimized.

Show comment
Hide comment
@kaiser101

kaiser101 May 18, 2017

How long before this change will be merged into the master?

How long before this change will be merged into the master?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu May 18, 2017

Member

I can't reproduce the find menu issue in safe mode, so it looks like that one was a false alarm.

Member

50Wliu commented May 18, 2017

I can't reproduce the find menu issue in safe mode, so it looks like that one was a false alarm.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo May 19, 2017

Contributor

@kaiser101 We're facing some hard crashes with Electron when the dev tools are open. We may merge anyway and figure it out on master though.

Contributor

nathansobo commented May 19, 2017

@kaiser101 We're facing some hard crashes with Electron when the dev tools are open. We may merge anyway and figure it out on master though.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo May 19, 2017

Contributor

@steelbrain Unclear from your gif... Were you seeing lag scrolling the editor? If so, the lag could be due to longer pauses garbage collecting DOM nodes as we dropped the recycling strategy. @as-cii I wonder if we should revisit that. At some point we're gonna GC DOM and I worry those pauses could get long. It allocates quite a bit of memory. But first priority is these crashes I suppose.

Contributor

nathansobo commented May 19, 2017

@steelbrain Unclear from your gif... Were you seeing lag scrolling the editor? If so, the lag could be due to longer pauses garbage collecting DOM nodes as we dropped the recycling strategy. @as-cii I wonder if we should revisit that. At some point we're gonna GC DOM and I worry those pauses could get long. It allocates quite a bit of memory. But first priority is these crashes I suppose.

@steelbrain

This comment has been minimized.

Show comment
Hide comment
@steelbrain

steelbrain May 19, 2017

Contributor

@nathansobo The editor was scrolling perfectly, the lag was only visible when scrolling search-and-replace tab (there would be white spaces in screen that would be filled later)

Contributor

steelbrain commented May 19, 2017

@nathansobo The editor was scrolling perfectly, the lag was only visible when scrolling search-and-replace tab (there would be white spaces in screen that would be filled later)

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo May 19, 2017

Contributor

@steelbrain That's definitely a find and replace bug and is worth reporting on that package if it hasn't been already.

Contributor

nathansobo commented May 19, 2017

@steelbrain That's definitely a find and replace bug and is worth reporting on that package if it hasn't been already.

as-cii added some commits May 19, 2017

Use custom snapshot row translation to prevent Electron 1.6 reload crash
Adding a source map for the entire snapshot was expensive in terms of
memory and seemed to be triggering some sort of bug in Chromium when
reloading with the DevTools open.

The custom row translation relies on a much more compact representation
of the data and avoids the crash.

Signed-off-by: Nathan Sobo <nathan@github.com>
Fix lint errors
Signed-off-by: Nathan Sobo <nathan@github.com>
@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii May 19, 2017

Member

Seems like AppVeyor failed but I believe that's just spurious. I will go ahead and merge it into master so that it can bake for a while before putting it on beta.

Thanks everyone for the help and the hard work on this! 馃檹

Member

as-cii commented May 19, 2017

Seems like AppVeyor failed but I believe that's just spurious. I will go ahead and merge it into master so that it can bake for a while before putting it on beta.

Thanks everyone for the help and the hard work on this! 馃檹

@as-cii as-cii merged commit 910fbee into master May 19, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

facebook-github-bot added a commit to facebook/nuclide that referenced this pull request May 31, 2017

Accomodate changes in text-editor-component refactor
Summary:
Imported from joefitzgerald's #1171

In atom/atom#12696, significant changes were made to the composition of the text-editor-component. These changes break Nuclide when working against current Atom master.

This PR allows Hyperclick to correctly function with the text-editor-component changes. It also fixes an issue with logic used to suppress cursor blinking for read only text editors.

Fixes #1154

Reviewed By: hansonw

Differential Revision:
D5151519

Tags: nuclide

fbshipit-source-id: fa7030921260474fd315cb70efc7c5e8384848b3

@t9md t9md referenced this pull request Jun 9, 2017

Closed

[v1.19] adding blockDecoration makes editor rendering odd #14743

1 of 1 task complete

@thomasjo thomasjo deleted the tj-upgrade-electron branch Jun 16, 2017

@damieng damieng referenced this pull request Jun 20, 2017

Closed

Error: Please enter a valid URL to shorten #132

6 of 6 tasks complete
@hansonw

This comment has been minimized.

Show comment
Hide comment
@hansonw

hansonw Jul 26, 2017

Contributor

@nathansobo Was this intentional? It seems like any user of jasmine.useMockClock() now ends up attempting to double-mock Date.now(), causing an exception. I think just the first spy above (in beforeEach) should be enough?

Contributor

hansonw commented on spec/spec-helper.coffee in 80f0331 Jul 26, 2017

@nathansobo Was this intentional? It seems like any user of jasmine.useMockClock() now ends up attempting to double-mock Date.now(), causing an exception. I think just the first spy above (in beforeEach) should be enough?

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Jul 27, 2017

Contributor

Yeah, you're right. Sorry. I'll get a PR going to remove the default mocking since it's probably more disruptive and just require the helper call. Not sure what was going through my head here. Thanks.

Contributor

nathansobo replied Jul 27, 2017

Yeah, you're right. Sorry. I'll get a PR going to remove the default mocking since it's probably more disruptive and just require the helper call. Not sure what was going through my head here. Thanks.

hansonw pushed a commit to facebook-atom/atom-ide-ui that referenced this pull request Sep 6, 2017

Accomodate changes in text-editor-component refactor
Summary:
Imported from joefitzgerald's facebook/nuclide#1171

In atom/atom#12696, significant changes were made to the composition of the text-editor-component. These changes break Nuclide when working against current Atom master.

This PR allows Hyperclick to correctly function with the text-editor-component changes. It also fixes an issue with logic used to suppress cursor blinking for read only text editors.

Fixes #1154

Reviewed By: hansonw

Differential Revision:
D5151519

Tags: nuclide

fbshipit-source-id: fa7030921260474fd315cb70efc7c5e8384848b3
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 2, 2017

Contributor

Shouldn't this be editorHeightInLines?

Shouldn't this be editorHeightInLines?

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Oct 3, 2017

Member

Ugh, my apologies: this was an oversight on my part while porting the logic in 4eecf8d. According to what we do below, however, it seems like we want to actually express this in terms of "height in tiles" and then multiply by rowsPerTile. I am unclear, however, on why we are not using getVisibleTileCount to perform this logic. Maybe we didn't want to use the cached values?

I am also concerned about "visible" vs. "rendered": if we don't use the latter, isn't it possible that we render screen lines that may not have been populated in the spatial index yet?

I also think we should rename this method to something like populateSpatialIndexUntilLastRenderedRow. I am happy to look into this together later today.

Member

as-cii replied Oct 3, 2017

Ugh, my apologies: this was an oversight on my part while porting the logic in 4eecf8d. According to what we do below, however, it seems like we want to actually express this in terms of "height in tiles" and then multiply by rowsPerTile. I am unclear, however, on why we are not using getVisibleTileCount to perform this logic. Maybe we didn't want to use the cached values?

I am also concerned about "visible" vs. "rendered": if we don't use the latter, isn't it possible that we render screen lines that may not have been populated in the spatial index yet?

I also think we should rename this method to something like populateSpatialIndexUntilLastRenderedRow. I am happy to look into this together later today.

@jasonrudolph jasonrudolph referenced this pull request May 8, 2018

Merged

Upgrade to Electron 2.0.0 #17273

60 of 60 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment