-
-
Notifications
You must be signed in to change notification settings - Fork 744
Additional APIs for WebContents #958
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
Conversation
FlorianRappl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - great; you sir are a true hero!
|
Just some struggles with a few flaky tests but getting there. It wasn't that much work to be honest, dealing with random failing tests is way more frustrating. |
|
Had to exclude some tests depending on GitHub runner OS as they can become flaky. |
softworkz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agracio - Great work! I'm sorry for being late.
Would you be able to make the properties async? The .Result property must not be used to synchronously wait for an async operation - it is only there to be used when a task has completed.
The appropriate pattern for such cases (and only when it's really unavoidable) is .GetAwaiter().GetResult().
But we have all properties async anyway, so there's no reason to diverge from that pattern.
Thanks
| public bool IsDevToolsOpened() | ||
| { | ||
| return Task.Run(() => InvokeAsync<bool>()).Result; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns boolean - Whether the devtools view is focused. | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public bool IsDevToolsFocused() | ||
| { | ||
| return Task.Run(() => InvokeAsync<bool>()).Result; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These properties should be async. It could deadlock like this.
| public int ZoomLevel | ||
| { | ||
| get | ||
| { | ||
| return Task.Run(() => this.InvokeAsync<int>()).Result; | ||
| } | ||
| set | ||
| { | ||
| BridgeConnector.Socket.Emit("webContents-zoomLevel-set", Id, value); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// A number property that determines the zoom factor for this web contents. | ||
| ///The zoom factor is the zoom percent divided by 100, so 300% = 3.0. | ||
| /// </summary> | ||
| public double ZoomFactor | ||
| { | ||
| get | ||
| { | ||
| return Task.Run(() => this.InvokeAsync<double>()).Result; | ||
| } | ||
| set | ||
| { | ||
| BridgeConnector.Socket.Emit("webContents-zoomFactor-set", Id, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=> async!
| public bool AudioMuted | ||
| { | ||
| get | ||
| { | ||
| return Task.Run(() => this.InvokeAsync<bool>()).Result; | ||
| } | ||
| set | ||
| { | ||
| BridgeConnector.Socket.Emit("webContents-audioMuted-set", Id, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=> async
| public string UserAgent | ||
| { | ||
| get | ||
| { | ||
| return Task.Run(() => this.InvokeAsync<string>()).Result; | ||
| } | ||
| set | ||
| { | ||
| BridgeConnector.Socket.Emit("webContents-userAgent-set", Id, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=> async
| public async Task GetPrintersAsync_check() | ||
| { | ||
| Skip.If(Environment.GetEnvironmentVariable("GITHUB_TOKEN") != null, "Skipping printer test in CI environment."); | ||
| Skip.If(Environment.GetEnvironmentVariable("GITHUB_RUN_ID") != null, "Skipping printer test in CI environment."); | ||
| var info = await fx.MainWindow.WebContents.GetPrintersAsync(); | ||
| info.Should().NotBeNull(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows server 2025 is the only platform where this takes longer, no need to completely disable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: this method was failing randomly and since we already had a skip on it, I just made it a correct one. I can undo but the problem is that I cannot rerun tests if they fail so need to do a fresh commit every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh - where is my commit which increases the timeout for GetPrintersAsync()? Did I forget to submit it...oh..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was on the wrong branch 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can try to increase the timeout to 10s for GetPrintersAsync(). Most likely there's a Windiows service which is not started by default and takes some time.
| [SkippableFact(Timeout = 20000)] | ||
| public async Task AudioMutedProperty_check() | ||
| { | ||
| Skip.If(Environment.GetEnvironmentVariable("GITHUB_RUN_ID") != null && RuntimeInformation.IsOSPlatform(OSPlatform.Windows), "Skipping test on Windows CI."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And attribute for controlling this would be nice (similar to SkipOnWslFactAttribute)
|
I checked other properties that we have in different classes and all are executed sync, do you have an example property where it is executed async? |
Sure - I'm counting 96... |
|
Most of the examples you have are methods and not properties, and the ones in AutoUpdater and BrowserView use Just to clarify - all properties (not methods) that we have in AutoUpdater, BrowserView and now WebContents use the same pattern |
9 are methods, 87 are properties. |
|
Process has async properties : |
That would be very nice! I would changed the (long-existing) sync ones to async as well, but people don't like breaking changes.. 😉 |
I think I am missing something, most are methods although they might return an underlying Electron property but are methods in c# code. |
|
.NET doesn't have support for actual async properties. When I'm talking about properties then it's properties in the Electron API for which we have GetXXXAsync() methods as accessors. |
|
As long as they return |
I was just about to write that. Yes, it's posible to return a Task from a property. It's only that the setters and getters can't be async. But's that's a somewhat uncommon pattern and kind-of contradictive to the concept of a Property. |
|
Exactly. Then you would have to provide a |
|
So what would be a solution, remove those properties completely? Another example public bool AudioMuted
{
get
{
return Task.Run(() => this.InvokeAsync<bool>()).Result;
}
set
{
BridgeConnector.Socket.Emit("webContents-audioMuted-set", Id, value);
}
}
public Task<bool> IsAudioMutedAsync() => InvokeAsync<bool>();
public Task<bool> IsCurrentlyAudibleAsync() => InvokeAsync<bool>();
public void SetAudioMuted(bool muted)
{
BridgeConnector.Socket.Emit("webContents-setAudioMuted", Id, muted);
} |
async getter and sync setter - like everywhere. |
Take a look at examples this would serve no purpose. public string UserAgent
{
get
{
return Task.Run(() => this.InvokeAsync<string>()).Result;
}
set
{
BridgeConnector.Socket.Emit("webContents-userAgent-set", Id, value);
}
}
public Task<string> GetUserAgentAsync() => InvokeAsync<string>();
public void SetUserAgent(string userAgent)
{
BridgeConnector.Socket.Emit("webContents-setUserAgent", Id, userAgent);
}
public bool AudioMuted
{
get
{
return Task.Run(() => this.InvokeAsync<bool>()).Result;
}
set
{
BridgeConnector.Socket.Emit("webContents-audioMuted-set", Id, value);
}
}
public Task<bool> IsAudioMutedAsync() => InvokeAsync<bool>();
public Task<bool> IsCurrentlyAudibleAsync() => InvokeAsync<bool>();
public void SetAudioMuted(bool muted)
{
BridgeConnector.Socket.Emit("webContents-setAudioMuted", Id, muted);
} |
Why now purpose? This IS our API pattern: public Task<bool> IsAudioMutedAsync() => InvokeAsync<bool>();
public void SetAudioMuted(bool muted)
{
BridgeConnector.Socket.Emit("webContents-setAudioMuted", Id, muted);
}Just to be clear: this is not talking about which might be the best kind of API pattern. It's merely about consistency (and avoiding async-to-sync). When we want to change that, then it must be everywhere. |
It would start by awaitng void calls as well and properly conveying exceptions from electron back to the .net API - and many more things. It's not that I don't have ideas for making this reallly great - but it's a lot of work... |
|
So if we look at the example below, the difference between those properties and methods is that property calls Electron property and methods calls Electron method, underlying Electron API call is not the same.
public string UserAgent
{
get
{
return Task.Run(() => this.InvokeAsync<string>()).Result;
}
set
{
BridgeConnector.Socket.Emit("webContents-userAgent-set", Id, value);
}
}
public Task<string> GetUserAgentAsync() => InvokeAsync<string>();
public void SetUserAgent(string userAgent)
{
BridgeConnector.Socket.Emit("webContents-setUserAgent", Id, userAgent);
}My question is what should we do with it, remove property? |
Yes. Because we don't have that information available synchronously. It' requires an async operation of which we are pretending it would be sync. If a user of the lib would encounter a deadlock due to this, and realizes what we are doing, he would be upset - and it would be justified. |
|
I've thought a lot about these things because my main project is also using a similar kind of communication (js <> C#), and the only way for making a remote propety available synchronously on the .net side, would be that the middleware (like ours here) would read the remote value on instantiation and register for a change event. Then it can mirror the remote value, so that .net callers can retrieve it really synchronously. But I concluded that this effort is probably only justified in very specific cases... |
|
Unrelated question, ElectronNET.Host,csproj points to non existing version of |
Oh yes of course! That's funny, because I had just used the "New Project" procedure in VS... Today, when I add such project, it has 1.0.2752196. So I suspect that there was mistake in updating where they accidentally had forgotten to replace their internal test version number in the template - or something.. |
|
Interesting is a valid package number although very outdated, I added this change to #960 |
|
It's 8 months old, but it has the most downloads of all versions in 2025. That's because it's a VS template which doesn't get updated so often. |
|
Hah! I know how it happened! When I create a new such project with VS 2026, it still uses that number... => Bug Interestingly it has still been working. |
|
I think VS has it's own ways to manage the project while other IDEs rely on a nuget package. It never failed on CI either. |
Removing all properties merged in #958
|
How does it behave in VS with the latest update, I used latest version instead of any VS versions. Hopefully this does not affect VS. |
|
Is there a reason why most calls to the bridge connector socket are not awaitable? I see that the public void OpenDevTools()
{
BridgeConnector.Socket.Emit("webContents-openDevTools", (object) this.Id); //<-- shouldn't this returned Task be returned to the caller?
}So the method call is synchronous but the socket call itself is not awaited (and an exception would fall through?). Just curious for the API/design decision in general. |
This should be fine, no worries, but thanks for the afterthought. |
That's a good question. The reason for that is that the asynchronicity of Ultimately, we should have asynchronous methods which complete only after we ahve received an indication from the remote side and error the task in case there was an error. It's not that easy though, because the remote implementation currently doesn't return anything, so it requires a rewrite on both sides. |
|
A partial solution to that would be to expose corresponding events where possible, this is unlikely to work for every single method but it would resolve the issue for some of them. For dev tools: |
That's something quite different (albeit useful), as the events are not telling you anything about the success of a method invocation - e.g. it could also be that the method failed but the user has opened dev-tools manually. |
|
Yes, that is why a called it partial solution. |
|
A different approach would be to emit |
Thanks for the clarification; however, wouldn't an exception still bypass the call site since it will be unwrapped inside the task? Or am I missing something here. I understand that it isn't waiting for the method call on the electron side, but the execution on the C# side is still executed asynchronously as a Task. |
Yes, that's correct, but there aren't any exceptions to expect on those paths. |
Example public Task OpenDevToolsAsync()
{
var tcs = new TaskCompletionSource();
BridgeConnector.Socket.Once("webContents-openDevTools-completed" + Id, () => tcs.SetResult());
BridgeConnector.Socket.Emit("webContents-openDevTools", Id);
return tcs.Task;
}socket.on("webContents-openDevTools", (id, options) => {
if (options) {
getWindowById(id).webContents.openDevTools(options);
} else {
getWindowById(id).webContents.openDevTools();
}
electronSocket.emit("webContents-openDevTools-completed" + id);
}); |

Added the following APIs and tests
Zoom
API for window zoom factor and limits.
User Agent
Audio
Global audio/mute controls for the entire window.
Dev Tools
Additional methods to control dev tools.
Miscellaneous