Skip to content

Conversation

Copy link
Collaborator

@FlorianRappl FlorianRappl left a 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!

@agracio
Copy link
Contributor Author

agracio commented Dec 4, 2025

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.

@agracio
Copy link
Contributor Author

agracio commented Dec 4, 2025

Had to exclude some tests depending on GitHub runner OS as they can become flaky.

@FlorianRappl FlorianRappl merged commit f1063bb into ElectronNET:develop Dec 5, 2025
17 checks passed
@agracio agracio mentioned this pull request Dec 5, 2025
Copy link
Collaborator

@softworkz softworkz left a 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

Comment on lines +158 to 170
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;
}
Copy link
Collaborator

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.

Comment on lines +323 to +349
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);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

=> async!

Comment on lines +402 to +412
public bool AudioMuted
{
get
{
return Task.Run(() => this.InvokeAsync<bool>()).Result;
}
set
{
BridgeConnector.Socket.Emit("webContents-audioMuted-set", Id, value);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

=> async

Comment on lines +438 to +448
public string UserAgent
{
get
{
return Task.Run(() => this.InvokeAsync<string>()).Result;
}
set
{
BridgeConnector.Socket.Emit("webContents-userAgent-set", Id, value);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

=> async

Comment on lines 78 to 83
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();
}
Copy link
Collaborator

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.

Copy link
Contributor Author

@agracio agracio Dec 6, 2025

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.

Copy link
Collaborator

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..

Copy link
Collaborator

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 🤣

Copy link
Collaborator

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.");
Copy link
Collaborator

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)

@agracio
Copy link
Contributor Author

agracio commented Dec 6, 2025

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?

@softworkz
Copy link
Collaborator

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...

          (374,29) return this.InvokeAsync<string>();
          (510,31) return await this.InvokeAsync<string>().ConfigureAwait(false);
          (574,31) return await this.InvokeAsync<string>().ConfigureAwait(false);
          (588,31) return await this.InvokeAsync<string>().ConfigureAwait(false);
          (871,31) return await this.InvokeAsync<JumpListSettings>().ConfigureAwait(false);
          (963,31) return await this.InvokeAsync<bool>().ConfigureAwait(false);
          (1005,31) return await this.InvokeAsync<string>().ConfigureAwait(false);
          (1072,31) return await this.InvokeAsync<ProcessMetric[]>().ConfigureAwait(false);
          (1084,31) return await this.InvokeAsync<GPUFeatureStatus>().ConfigureAwait(false);
          (1123,31) return await this.InvokeAsync<int>().ConfigureAwait(false);
          (1139,31) return await this.InvokeAsync<bool>().ConfigureAwait(false);
          (1208,31) return await this.InvokeAsync<bool>().ConfigureAwait(false);
        AutoUpdater.cs
          (26,44) return Task.Run(() => this.InvokeAsync<bool>()).Result;
          (43,44) return Task.Run(() => this.InvokeAsync<bool>()).Result;
          (61,44) return Task.Run(() => this.InvokeAsync<bool>()).Result;
          (77,44) return Task.Run(() => this.InvokeAsync<bool>()).Result;
          (94,44) return Task.Run(() => this.InvokeAsync<bool>()).Result;
          (109,44) return Task.Run(() => this.InvokeAsync<string>()).Result;
          (120,44) return Task.Run(() => this.InvokeAsync<SemVer>());
          (145,44) return Task.Run(() => this.InvokeAsync<string>());
          (168,44) return Task.Run(() => this.InvokeAsync<Dictionary<string, string>>());
        BrowserView.cs
          (34,44) return Task.Run(() => this.InvokeAsync<Rectangle>()).Result;
        BrowserWindow.cs
          (326,48) public Task<bool> IsFocusedAsync() => this.InvokeAsync<bool>();
          (332,50) public Task<bool> IsDestroyedAsync() => this.InvokeAsync<bool>();
          (353,48) public Task<bool> IsVisibleAsync() => this.InvokeAsync<bool>();
          (359,46) public Task<bool> IsModalAsync() => this.InvokeAsync<bool>();
          (375,50) public Task<bool> IsMaximizedAsync() => this.InvokeAsync<bool>();
          (391,50) public Task<bool> IsMinimizedAsync() => this.InvokeAsync<bool>();
          (403,51) public Task<bool> IsFullScreenAsync() => this.InvokeAsync<bool>();
          (482,53) public Task<Rectangle> GetBoundsAsync() => this.InvokeAsync<Rectangle>();
          (501,60) public Task<Rectangle> GetContentBoundsAsync() => this.InvokeAsync<Rectangle>();
          (522,47) public Task<int[]> GetSizeAsync() => this.InvokeAsync<int[]>();
          (543,54) public Task<int[]> GetContentSizeAsync() => this.InvokeAsync<int[]>();
          (556,54) public Task<int[]> GetMinimumSizeAsync() => this.InvokeAsync<int[]>();
          (569,54) public Task<int[]> GetMaximumSizeAsync() => this.InvokeAsync<int[]>();
          (581,50) public Task<bool> IsResizableAsync() => this.InvokeAsync<bool>();
          (599,48) public Task<bool> IsMovableAsync() => this.InvokeAsync<bool>();
          (617,52) public Task<bool> IsMinimizableAsync() => this.InvokeAsync<bool>();
          (635,52) public Task<bool> IsMaximizableAsync() => this.InvokeAsync<bool>();
          (647,55) public Task<bool> IsFullScreenableAsync() => this.InvokeAsync<bool>();
          (665,49) public Task<bool> IsClosableAsync() => this.InvokeAsync<bool>();
          (703,52) public Task<bool> IsAlwaysOnTopAsync() => this.InvokeAsync<bool>();
          (753,51) public Task<int[]> GetPositionAsync() => this.InvokeAsync<int[]>();
          (767,49) public Task<string> GetTitleAsync() => this.InvokeAsync<string>();
          (812,46) public Task<bool> IsKioskAsync() => this.InvokeAsync<bool>();
          (818,57) public Task<string> GetNativeWindowHandle() => this.InvokeAsync<string>();
          (833,63) public Task<string> GetRepresentedFilenameAsync() => this.InvokeAsync<string>();
          (848,55) public Task<bool> IsDocumentEditedAsync() => this.InvokeAsync<bool>();
          (964,48) public Task<bool> HasShadowAsync() => this.InvokeAsync<bool>();
          (1065,56) public Task<bool> IsMenuBarAutoHideAsync() => this.InvokeAsync<bool>();
          (1082,55) public Task<bool> IsMenuBarVisibleAsync() => this.InvokeAsync<bool>();
          (1102,63) public Task<bool> IsVisibleOnAllWorkspacesAsync() => this.InvokeAsync<bool>();
          (1155,42) var browserWindowId = await this.InvokeAsync<int>().ConfigureAwait(false);
          (1166,43) var browserWindowIds = await this.InvokeAsync<int[]>().ConfigureAwait(false);
        Clipboard.cs
          (50,69) public Task<string> ReadTextAsync(string type = "") => this.InvokeAsync<string>(type);
          (67,69) public Task<string> ReadHTMLAsync(string type = "") => this.InvokeAsync<string>(type);
          (84,68) public Task<string> ReadRTFAsync(string type = "") => this.InvokeAsync<string>(type);
          (104,63) public Task<ReadBookmark> ReadBookmarkAsync() => this.InvokeAsync<ReadBookmark>();
          (130,57) public Task<string> ReadFindTextAsync() => this.InvokeAsync<string>();
          (157,79) public Task<string[]> AvailableFormatsAsync(string type = "") => this.InvokeAsync<string[]>(type);
          (174,75) public Task<NativeImage> ReadImageAsync(string type = "") => this.InvokeAsync<NativeImage>(type);
        NativeTheme.cs
          (111,68) public Task<ThemeSourceMode> GetThemeSourceAsync() => this.InvokeAsync<ThemeSourceMode>();
          (118,62) public Task<bool> ShouldUseDarkColorsAsync() => this.InvokeAsync<bool>();
          (126,70) public Task<bool> ShouldUseHighContrastColorsAsync() => this.InvokeAsync<bool>();
          (134,71) public Task<bool> ShouldUseInvertedColorSchemeAsync() => this.InvokeAsync<bool>();
        Notification.cs
          (120,54) public Task<bool> IsSupportedAsync() => this.InvokeAsync<bool>();
        Process.cs
          (48,51) public Task<string> ExecPathAsync => this.InvokeAsync<string>();
          (57,49) public Task<string[]> ArgvAsync => this.InvokeAsync<string[]>();
          (63,47) public Task<string> TypeAsync => this.InvokeAsync<string>();
          (69,60) public Task<ProcessVersions> VersionsAsync => this.InvokeAsync<ProcessVersions>();
          (75,51) public Task<bool> DefaultAppAsync => this.InvokeAsync<bool>();
          (81,52) public Task<bool> IsMainFrameAsync => this.InvokeAsync<bool>();
          (86,56) public Task<string> ResourcesPathAsync => this.InvokeAsync<string>();
          (92,49) public Task<double> UpTimeAsync => this.InvokeAsync<double>();
          (97,43) public Task<int> PidAsync => this.InvokeAsync<int>();
          (102,47) public Task<string> ArchAsync => this.InvokeAsync<string>();
          (107,51) public Task<string> PlatformAsync => this.InvokeAsync<string>();
        Screen.cs
          (106,64) public Task<Point> GetCursorScreenPointAsync() => this.InvokeAsync<Point>();
          (113,66) public Task<Rectangle> GetMenuBarWorkAreaAsync() => this.InvokeAsync<Rectangle>();
          (119,63) public Task<Display> GetPrimaryDisplayAsync() => this.InvokeAsync<Display>();
          (125,62) public Task<Display[]> GetAllDisplaysAsync() => this.InvokeAsync<Display[]>();
          (131,79) public Task<Display> GetDisplayNearestPointAsync(Point point) => this.InvokeAsync<Display>(point);
          (138,83) public Task<Display> GetDisplayMatchingAsync(Rectangle rectangle) => this.InvokeAsync<Display>(rectangle);
        WebContents.cs
          (160,31) return Task.Run(() => InvokeAsync<bool>()).Result;
          (169,31) return Task.Run(() => InvokeAsync<bool>()).Result;
          (183,64) public Task<bool> PrintAsync(PrintOptions options) => this.InvokeAsync<bool>(options);
          (189,44) public Task<bool> PrintAsync() => this.InvokeAsync<bool>(string.Empty);
          (327,40) return Task.Run(() => this.InvokeAsync<int>()).Result;
          (343,40) return Task.Run(() => this.InvokeAsync<double>()).Result;
          (355,49) public Task<double> GetZoomFactorAsync() => InvokeAsync<double>();
          (372,45) public Task<int> GetZoomLevelAsync() => InvokeAsync<int>();
          (406,40) return Task.Run(() => this.InvokeAsync<bool>()).Result;
          (418,46) public Task<bool> IsAudioMutedAsync() => InvokeAsync<bool>();
          (424,52) public Task<bool> IsCurrentlyAudibleAsync() => InvokeAsync<bool>();
          (442,40) return Task.Run(() => this.InvokeAsync<string>()).Result;
          (454,48) public Task<string> GetUserAgentAsync() => InvokeAsync<string>();

@agracio
Copy link
Contributor Author

agracio commented Dec 6, 2025

Most of the examples you have are methods and not properties, and the ones in AutoUpdater and BrowserView use return Task.Run(() => this.InvokeAsync<bool>()).Result;
Am I missing something?

Just to clarify - all properties (not methods) that we have in AutoUpdater, BrowserView and now WebContents use the same pattern return await this.InvokeAsync<bool>().ConfigureAwait(false)

@softworkz
Copy link
Collaborator

Most of the examples you have are methods and not properties

9 are methods, 87 are properties.

@agracio
Copy link
Contributor Author

agracio commented Dec 6, 2025

Process has async properties : public Task<string> ExecPathAsync => this.InvokeAsync<string>(); I missed them because they dont have setter. I can redo the ones I created to be async

@softworkz
Copy link
Collaborator

softworkz commented Dec 6, 2025

I can redo the ones I created to be async

That would be very nice!

I would changed the (long-existing) sync ones to async as well, but people don't like breaking changes.. 😉

@agracio
Copy link
Contributor Author

agracio commented Dec 6, 2025

9 are methods, 87 are properties.

I think I am missing something, most are methods although they might return an underlying Electron property but are methods in c# code.

 Screen.cs
          (106,64) public Task<Point> GetCursorScreenPointAsync() => this.InvokeAsync<Point>();
          (113,66) public Task<Rectangle> GetMenuBarWorkAreaAsync() => this.InvokeAsync<Rectangle>();
          (119,63) public Task<Display> GetPrimaryDisplayAsync() => this.InvokeAsync<Display>();
          (125,62) public Task<Display[]> GetAllDisplaysAsync() => this.InvokeAsync<Display[]>();
          (131,79) public Task<Display> GetDisplayNearestPointAsync(Point point) => this.InvokeAsync<Display>(point);
          (138,83) public Task<Display> GetDisplayMatchingAsync(Rectangle rectangle) => this.InvokeAsync<Display>(rectangle);
NativeTheme.cs
          (111,68) public Task<ThemeSourceMode> GetThemeSourceAsync() => this.InvokeAsync<ThemeSourceMode>();
          (118,62) public Task<bool> ShouldUseDarkColorsAsync() => this.InvokeAsync<bool>();
          (126,70) public Task<bool> ShouldUseHighContrastColorsAsync() => this.InvokeAsync<bool>();
          (134,71) public Task<bool> ShouldUseInvertedColorSchemeAsync() => this.InvokeAsync<bool>();
        Notification.cs
          (120,54) public Task<bool> IsSupportedAsync() => this.InvokeAsync<bool>();

@softworkz
Copy link
Collaborator

.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.

@agracio
Copy link
Contributor Author

agracio commented Dec 6, 2025

As long as they return Task I would say it counts as async public Task<string> ExecPathAsync => this.InvokeAsync<string>();

@softworkz
Copy link
Collaborator

As long as they return Task I would say it counts as async public Task<string> ExecPathAsync => this.InvokeAsync<string>();

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.

@softworkz
Copy link
Collaborator

Exactly. Then you would have to provide a Task<T> for setting a value where you actually should get a Task returned from the setter - so you can notice when it throws asynchronously (something which we don't have generally - but actually should...)

@agracio
Copy link
Contributor Author

agracio commented Dec 6, 2025

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);
}

@softworkz
Copy link
Collaborator

softworkz commented Dec 6, 2025

So what would be a solution, remove those properties completely?

async getter and sync setter - like everywhere.

@agracio
Copy link
Contributor Author

agracio commented Dec 6, 2025

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);
}

@softworkz
Copy link
Collaborator

Take a look at examples this would serve no purpose.

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.

@softworkz
Copy link
Collaborator

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...

@agracio
Copy link
Contributor Author

agracio commented Dec 6, 2025

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.
This is also true for Audio and Zoom properties and methods.

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?

@softworkz
Copy link
Collaborator

softworkz commented Dec 6, 2025

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.

@softworkz
Copy link
Collaborator

softworkz commented Dec 6, 2025

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...

@agracio
Copy link
Contributor Author

agracio commented Dec 6, 2025

#960

@agracio
Copy link
Contributor Author

agracio commented Dec 6, 2025

Unrelated question, ElectronNET.Host,csproj points to non existing version of Microsoft.VisualStudio.JavaScript.Sdk this fails to load it in VSCode and Rider. Should that be fixed?

@softworkz
Copy link
Collaborator

Unrelated question, ElectronNET.Host,csproj points to non existing version of Microsoft.VisualStudio.JavaScript.Sdk this fails to load it in VSCode and Rider. Should that be fixed?

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..

@agracio
Copy link
Contributor Author

agracio commented Dec 6, 2025

Interesting is a valid package number although very outdated, I added this change to #960

@softworkz
Copy link
Collaborator

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.

@softworkz
Copy link
Collaborator

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.

@agracio
Copy link
Contributor Author

agracio commented Dec 6, 2025

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.

@softworkz
Copy link
Collaborator

Found it: That package exists: it ships with Visual Studio:

image

A diff to the closest nuget version didn't show any interesting differences. Well, they may have their reasons....

FlorianRappl added a commit that referenced this pull request Dec 6, 2025
Removing all properties merged in #958
@agracio
Copy link
Contributor Author

agracio commented Dec 6, 2025

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.

@AeonSake
Copy link

AeonSake commented Dec 7, 2025

Is there a reason why most calls to the bridge connector socket are not awaitable? I see that the Emit() method returns a Task but the wrapping methods don't expose that. Example:

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.

@softworkz
Copy link
Collaborator

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.

This should be fine, no worries, but thanks for the afterthought.

@softworkz
Copy link
Collaborator

Is there a reason why most calls to the bridge connector socket are not awaitable? I see that the Emit() method returns a Task but the wrapping methods don't expose that. Example:

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.

That's a good question. The reason for that is that the asynchronicity of Socket.Emit() is misleading. It completes the Task as soon as the message has been sent "on the wire", so this doesn't tell anything about whether the invocation has completed on the remote side and whether it was successful.
If we would return that task, users would make false assumptions and expect that it behaves like I said above.

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.

@agracio
Copy link
Contributor Author

agracio commented Dec 7, 2025

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:

@softworkz
Copy link
Collaborator

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.

@agracio
Copy link
Contributor Author

agracio commented Dec 7, 2025

Yes, that is why a called it partial solution.

@agracio
Copy link
Contributor Author

agracio commented Dec 7, 2025

A different approach would be to emit -completed event from host after opening dev tools. This would allow for method to return Task instead of void and only complete once it receives a signal from host.

@AeonSake
Copy link

AeonSake commented Dec 7, 2025

Is there a reason why most calls to the bridge connector socket are not awaitable? I see that the Emit() method returns a Task but the wrapping methods don't expose that. Example:

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.

That's a good question. The reason for that is that the asynchronicity of Socket.Emit() is misleading. It completes the Task as soon as the message has been sent "on the wire", so this doesn't tell anything about whether the invocation has completed on the remote side and whether it was successful. If we would return that task, users would make false assumptions and expect that it behaves like I said above.

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.

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.

@softworkz
Copy link
Collaborator

Thanks for the clarification; however, wouldn't an exception still bypass the call site since it will be unwrapped inside the task?

Yes, that's correct, but there aren't any exceptions to expect on those paths.

@agracio
Copy link
Contributor Author

agracio commented Dec 9, 2025

A different approach would be to emit -completed event from host after opening dev tools. This would allow for method to return Task instead of void and only complete once it receives a signal from host.

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);
});

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants