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.

Sign up for GitHub

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

Jump to bottom

Update to electron 4 #1590

Merged
merged 12 commits into from
Jul 30, 2019
Merged

Update to electron 4 #1590

merged 12 commits into from
Jul 30, 2019

Conversation

Borewit
Copy link
Member

@Borewit Borewit commented May 15, 2019 •
edited
Loading

Related to PR: #1549.

I have not tested these changes thoroughly.

@dsernst dsernst mentioned this pull request Jun 4, 2019
Closed
@dsernst
Copy link
Contributor

dsernst commented Jun 4, 2019 •
edited
Loading

Seeing bugginess w/ a duplicated title bar on macOS:

This branch:

image

Master (as of 9a0a211):

image

Update: Turned into a PR to this branch: #1596

Update 2: Now merged in and fixed. 🎉

@dsernst dsernst mentioned this pull request Jun 4, 2019
Merged
@dsernst
Copy link
Contributor

dsernst commented Jun 4, 2019 •
edited
Loading

Reviewing items from the Electron Breaking Changes doc: https://github.com/electron/electron/blob/master/docs/api/breaking-changes.md#breaking-api-changes-20

Breaking API Changes (2.0)

  • BrowserWindow - The single use is updated by PR Fix hiddenInset (Mac) titlebar style for electron 4 #1596.
  • menu - Only one use of menu.popup, but doesn't use changed positioning params.
  • nativeImage - No use of .toPng or .toJpeg
  • process - Two calls to process.versions.electron, but both are reads, not writes. No calls to process.version.chrome.
  • webContents - No calls to webContents.setZoomLevelLimits
  • webFrame - No calls to webFrame.setZoomLevelLimits
  • <webview> - No calls to webview.setZoomLevelLimits
  • Duplicate ARM Assets - Not quite sure what to make of this one. Linux specific. Not seeing any references to linux-arm in the codebase.
    • Happy to mark completed on confirmation from someone with better knowledge of the Linux build.

@dsernst
Copy link
Contributor

dsernst commented Jun 4, 2019 •
edited
Loading

Reviewing items from the Electron Breaking Changes doc: https://github.com/electron/electron/blob/master/docs/api/breaking-changes.md#breaking-api-changes-30

Breaking API Changes (3.0)

  • app
    • No calls to .getAppMemoryInfo()
    • No calls to .getAppMetrics()
  • BrowserWindow
    • No references to blinkFeatures
    • No references to media-play_pause
  • clipboard
    • No calls to .readRtf()
    • No calls to .writeRtf()
    • No calls to .readHtml()
    • No calls to .writeHtml()
  • crashReporter
  • nativeImage — No calls to .createFromBuffer
  • process — No calls to .getProcessMemoryInfo()
  • screen — No calls to .getMenuBarHeight()
  • session — No calls to .setCertificateVerifyProc
  • Tray — No calls to .setHighlightMode
  • webContents
  • webFrame
    • No calls to .registerURLSchemeAsSecure
    • No calls to .registerURLSchemeAsPrivileged
  • <webview>
    • No references to disableguestresize
    • No references to guestinstance
    • No references to webview, so don't think these keyboard handlers are being used.
  • Node Headers URL — Think this one is good. Not seeing any references in the codebase to atom-shell, or an .npmrc file. Everything seems to be building fine.

@dsernst dsernst mentioned this pull request Jun 4, 2019
Merged
@Borewit
Merge pull request #1596 from dsernst/electron-4-titlebar
d45e3f3 
Fix hiddenInset (Mac) titlebar style for electron 4
@Borewit
Copy link
Member Author

Borewit commented Jun 4, 2019

Thanks a lot for your review @dsernst, very useful.

Note that I may have broken something by dropping the depreciated onAppOpen event.

I guess the idea was, if the application is already running and called a second time, the argument, presumably a torrent file, would be passed and added to the instance running.

@dsernst
Copy link
Contributor

dsernst commented Jun 4, 2019 •
edited
Loading

Reviewing items from the Electron Breaking Changes doc: https://github.com/electron/electron/blob/master/docs/api/breaking-changes.md#planned-breaking-api-changes-40

Breaking API Changes (4.0)

  • app.makeSingleInstance — Only call to .makeSingleInstance replaced already.
  • app.releaseSingleInstance — No calls to .releaseSingleInstance
  • app.getGPUInfo — No calls to .getGPUInfo
  • win_delay_load_hook — No references to win_delay_load_hook

@dsernst
Copy link
Contributor

dsernst commented Jun 4, 2019

Note that I may have broken something by dropping the depreciated onAppOpen event.

I guess the idea was, if the application is already running and called a second time, the argument, presumably a torrent file, would be passed and added to the instance running.

Ah. Yes, can confirm that I'm having trouble adding a torrent via magnet link when the app is already open.

@dsernst
Copy link
Contributor

dsernst commented Jun 4, 2019 •
edited
Loading

I'm trying to test my own fix for this... but even on master (electron v1) I'm unable to add new torrents if the npm start version of the app is already open.

So the issue reported above might not have been introduced by your code changes. Unsure how to proceed.

@mathiasvr
Copy link
Contributor

You may want to take a look at my Electron 3 PR which was not merged #1481.
This should handle the breaking changes for v2 and v3.

@Borewit
Copy link
Member Author

Borewit commented Jun 17, 2019

With the remaining fix @mathiasvr put in, it looks good to me.
Can you confirm @dsernst?
Would be nice to upgrade Electron as well as part of the new release (#1604) 😉

@Borewit Borewit requested a review from feross July 10, 2019 05:31
feross
feross requested changes Jul 10, 2019
View reviewed changes
package.json
@@ -55,7 +55,7 @@
"buble": "^0.19.6",
"cross-zip": "^2.0.1",
"depcheck": "^0.7.2",
"electron": "^1.8.8",
"electron": "^4.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

This is great!

src/main/index.js Outdated
app.quit()
} else {
app.on('second-instance', (event, commandLine) => onAppOpen(commandLine))
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this logic repeated below? It seems like this will cause the 'second-instance' event to be registered twice. Is this intentional?

@feross
Copy link
Member

feross commented Jul 10, 2019

Also, can you resolve package.json conflicts?

@feross feross mentioned this pull request Jul 20, 2019
Closed
@Borewit
Copy link
Member Author

Borewit commented Jul 28, 2019

@feross, can you please update the review?

@feross feross mentioned this pull request Jul 28, 2019
Closed
@Borewit Borewit requested a review from feross July 28, 2019 07:40
feross
feross approved these changes Jul 28, 2019
View reviewed changes
Copy link
Member

@feross feross left a comment

Choose a reason for hiding this comment

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

LGTM!

mathiasvr
mathiasvr approved these changes Jul 30, 2019
View reviewed changes
Copy link
Contributor

@mathiasvr mathiasvr left a comment

Choose a reason for hiding this comment

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

Looks fine, I didn't test it though.

@Borewit Borewit merged commit 4c2381d into master Jul 30, 2019
@Borewit Borewit deleted the update-electron branch July 30, 2019 19:44
@feross
Copy link
Member

feross commented Jul 31, 2019

Thanks for landing this, @Borewit!

@DiegoRBaquero
Copy link
Member

Beautiful :´)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathiasvr mathiasvr mathiasvr approved these changes

@feross feross Awaiting requested review from feross

@DiegoRBaquero DiegoRBaquero Awaiting requested review from DiegoRBaquero

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Borewit @dsernst @mathiasvr @feross @DiegoRBaquero