8000 docs: views API: support setting content view by zcbenz · Pull Request #254 · electron/governance · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

docs: views API: support setting content view #254

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
Merged
merged 3 commits into from
Apr 6, 2020
Merged

docs: views API: support setting content view #254

merged 3 commits into from
Apr 6, 2020

Conversation

zcbenz
Copy link
Contributor
@zcbenz zcbenz commented Mar 19, 2020

This spec proposes to make parts of views related APIs public, which should be enough to implement a simple splash screen for apps.

const {app, nativeImage, webContents, ImageView, TopLevelWindow, WebContentsView} = require('electron')

app.once('ready', () => {
  const win = new TopLevelWindow({})
  const imageView = new ImageView()
  imageView.setImage(__dirname + '/splash.png')
  win.setContentView(imageView)

  const contents = webContents.create({})
  const web = new WebContentsView(contents)
  contents.once('did-finish-load', () => win.setContentView(web))
  contents.loadURL('https://github.com/')
})

@zcbenz zcbenz requested a review from a team March 19, 2020 10:04
jkleinsc
jkleinsc approved these changes Mar 19, 2020
View reviewed changes
Copy link
Member
@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

@itsananderson
Copy link
Member

This seems like a super useful set of APIs to unlock. Very exciting!

If I want to use TopLevelWindow rather than BrowserWindow, how much glue code am I going to need to implement myself? One example that occurs to me is that a BrowserWindow automatically updates its window title when the title of the WebContents changes. Would we also expect that to happen automatically if you embed a WebContentsView in a TopLevelWindow, or would you need to listen for title changes in the WebContents and then propagate to the TopLevelWindow?

I don't have a strong opinion about how much of that should happen "for free", so long as the delta between BrowserWindow and TopLevelWindow is understood. We'll want to documented the differences and provide TopLevelWindow APIs that allow people to switch without regressing the behavior they had with BrowserWindow

@zcbenz
Copy link
Contributor Author
zcbenz commented Mar 20, 2020

@itsananderson The BrowserWindow inherits from TopLevelWindow so you don't have to use the latter if you want all the glue:

const win = new BrowserWindow({show: false})
const web = win.getContentView()
win.setContentView(new ImageView())
win.show()

win.webContents.once('did-finish-load', win.setContentView(web))
win.loadURL('https://github.com')

Adding a WebContentsView to TopLevelWindow won't automatically setup the glue code, because users would be able to add multiple child views in future so it is not certain which view would be the major client area.

And yeah it is very reasonable to document all the behaviors of BrowserWindow, and we should ensure users can replicate the behaviors using our public APIs. Currently the implementation of BrowserWindow has already been separated from TopLevelWindow, so you can read its source code to have an idea of what it is doing:
https://github.com/electron/electron/blob/master/lib/browser/api/browser-window.js
https://github.com/electron/electron/blob/master/shell/browser/api/electron_api_browser_window.cc

@zcbenz zcbenz mentioned this pull request Mar 20, 2020
Merged
6 tasks
@zcbenz
Copy link
Contributor Author
zcbenz commented Mar 24, 2020

@electron/wg-api I have updated the spec:

  • Remove webContents.create()
  • Make the constructor of WebContentsView take webPreferences instead of WebContents
  • Add TopLevelWindow.getContentView() (Required for implementing a splash screen for BrowserWindow)
  • Add WebContentsView.webContents property (Required for refactoring WebContentsView to take webPreferences)

Also on the behavior when a View with a parent is added to another parent, both Cocoa and views library would remove the View from the old parent and then add it to the new parent, I think we can just do the same for the setContentView API.

jkleinsc
jkleinsc requested changes Mar 24, 2020
View reviewed changes
Copy link
Member
@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

@zcbenz the example needs to be updated to reflect the changes and I've suggested an update here. The only thing I couldn't figure out is why or where TopLevelWindow.getContentView() is needed?

@itsananderson
Copy link
Member
  • Remove webContents.create()

When you say "remove" do you mean remove from this spec, or remove from Electron entirely? webContents.create() already exists, though I think it's not documented.

@zcbenz
Update the code example
babf601 
Co-Authored-By: John Kleinschmidt <jkleinsc@github.com>
@zcbenz
Copy link
Contributor Author
zcbenz commented Mar 25, 2020 •
edited
Loading

The only thing I couldn't figure out is why or where TopLevelWindow.getContentView() is needed?

It would be required if user wants to keep using BrowserWindow and temporarily swap its content view with an ImageView to implement splash screen. You can check the example in the comment above: #254 (comment).

It would also be required to write tests on how setContentView affects BrowserWindow, and on the behavior when a view with a parent is re-added to another view.

  • Remove webContents.create()

When you say "remove" do you mean remove from this spec, or remove from Electron entirely? webContents.create() already exists, though I think it's not documented.

I mean we won't make the API public as part of this spec. This API is already being used heavily in our tests.

@zcbenz zcbenz requested a review from jkleinsc March 25, 2020 01:16
@zcbenz zcbenz merged commit 9d93c9a into master Apr 6, 2020
@zcbenz zcbenz deleted the views-api-1 branch April 6, 2020 23:14
This was referenced Apr 7, 2020
Merged
Closed
This was referenced Apr 10, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@zcbenz @itsananderson @jkleinsc
0