Skip to content

Conversation

@Denny09310
Copy link

This PR replaces Newtonsoft.Json with the built-in System.Text.Json. The tests were taken from @softworkz’s branch and all are passing, except for the two already noted in issue #913.

@FlorianRappl
Copy link
Collaborator

Can you resolve the conflicts? I know its annoying, but right now due to the conflicts it does not show the true diff (many of the changes are already in develop as you took the other branch before it landed in develop). Many thanks!

@Denny09310
Copy link
Author

Yes, absolutely!

@FlorianRappl FlorianRappl self-requested a review November 9, 2025 13:16
@FlorianRappl FlorianRappl added this to the 0.1.0 milestone Nov 9, 2025
@softworkz
Copy link
Contributor

softworkz commented Nov 9, 2025

@Denny09310 - this doesn't look right. The PR is still including my commits. You need to rebase onto the develop branch, instead of submitting with merge commit.

@Denny09310 Denny09310 force-pushed the feature/system-text-json branch from b1c3f66 to dfe04f1 Compare November 9, 2025 13:41
@Denny09310
Copy link
Author

Hope to have done everything right. Sorry but I don't use GitHub at this level frequently so I don't know how to do everything 😅

@softworkz
Copy link
Contributor

Hope to have done everything right. Sorry but I don't use GitHub at this level frequently so I don't know how to do everything

All perfect now, thanks!

@softworkz
Copy link
Contributor

What was it, what made the one test fail?
Not that I'm really interested in that specific detail, it's just about whether it was an issue that could possibly also bite in other cases?

@Denny09310
Copy link
Author

What was it, what made the one test fail? Not that I'm really interested in that specific detail, it's just about whether it was an issue that could possibly also bite in other cases?

It was Notification_create_check not calling the OnShow due to NotificationOptions.ShowID not being serialized. Fixed by adding [JsonInclude] attribute

…directly instead of JsonSerializer.Deserialize(JsonElement), removed some unused attributes
@Denny09310
Copy link
Author

@softworkz @FlorianRappl Okay, probably something I was doing wrong long before this PR, I'm going to replace all the .On<JsonElement> into the corresponding value and test it out

@Denny09310
Copy link
Author

As the commit description says, I've refactored all On calls to deserialize directly to the correct type except some methods that expects mixed arrays or complex values (Eg. AddEvent args). Any ideas or we leave them as they are? @softworkz @FlorianRappl

@FlorianRappl
Copy link
Collaborator

Any ideas or we leave them as they are?

Of course if we know the T we could pass it on - otherwise, especially for the complex / mixed case, I'd say it's good to leave them at the JSON representation (JsonElement).

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 - thanks for the superb work @Denny09310 !

@FlorianRappl FlorianRappl merged commit 41a2b07 into ElectronNET:develop Nov 10, 2025
2 of 3 checks passed
@agracio agracio mentioned this pull request Nov 11, 2025
@agracio
Copy link

agracio commented Nov 11, 2025

@Denny09310, @FlorianRappl There is an error in this PR.

IpcMain.cs

public void Send(BrowserWindow browserWindow, string channel, params object[] data)
{
    BridgeConnector.Socket.Emit("sendToIpcRenderer", browserWindow, channel, data);
}

This causes data to always be sent as array, to reproduce fire a demo app -> "Get app and system information" -> "Get screen information".

This will fail to show correct display size, a quick debug shows that IpcMain sends [Size] instead of Size.

Demo app code:

Electron.IpcMain.On("screen-info", async (args) =>
{
    var display = await Electron.Screen.GetPrimaryDisplayAsync();
    var mainWindow = Electron.WindowManager.BrowserWindows.First();
    Electron.IpcMain.Send(mainWindow, "got-screen-info", display.Size);
});

@FlorianRappl
Copy link
Collaborator

Great catch. Yes, needs to be fixed. Thanks for spotting.

@agracio
Copy link

agracio commented Nov 11, 2025

Great catch. Yes, needs to be fixed. Thanks for spotting.

False alarm, this is not a new bug and was present before the change to Send method. Not sure if it was always broken or is a part of another PR. Will continue checking various builds.

@agracio
Copy link

agracio commented Nov 11, 2025

I ran all version all the way back to start of refactoring, all have the same problem with sending Size object as an array.
Pretty sure that it should be working.
It looks like the issue does not affect primitive types like strings or ints.

Tried everything but cannot figure out the reason for this behaviour.

@softworkz
Copy link
Contributor

@agracio - Thanks for the invesigation. Fixed here: #922

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