Initial refactoring of dotnet API #905
Conversation
|
Actually much appreciated - with more time this is something on my list (really cumbersome code in some areas, could benefit a lot from some refactoring... - so great job 🚀 !). |
FlorianRappl
left a comment
FlorianRappl
left a comment
There was a problem hiding this comment.
Yes, definitely appreciated and goes in the right direction. Just let me know when you are done / want to see it merged in.
|
Almost done with events but would also want to clean up at least some of the tasks code. I will let you know when events are done. |
|
Events are done. Even managed to find an error in one of them. Task optimisation is next but you can merge event changes first if you want. |
|
Tasks will take significantly longer. There are many different types and of course different naming. On subject of naming - is SocketIO case sensitive, there are some tasks that end in |
|
Yeah so in general these strings are all case sensitive - personally, I'd make it consistent (I've seen that across the codebase many things in that area a bit inconsistent to write the least... - so using the opportunity to make the naming / transported names consistent would be appreciated). Many thanks for your efforts - so we'll merge this one right now and do another PR at some point in time later? @agracio |
|
Yes I would suggest to merge these changes first while I work on tasks. There is more than I initially thought. |
|
Alternatively I can refactor |
OK let's add this then merge! Thanks! |
|
All done. Please give it a thorough review. |
FlorianRappl
left a comment
FlorianRappl
left a comment
There was a problem hiding this comment.
LGTM - at least I could not see anything going wrong here in the review pane. Let's merge and test with the preview version.
|
I have an off-topic question, currently supported Electron versions are 37 - 39. While it is possible to change version during development should a default version be one of the supported ones? |
It's a good question - I'd say yes (preferably the most recent LTS; but I don't think the follow that model (all their releases have the same supported time range). So let's try to go for the most recent one then (39, even though 40 is on the horizon)? |
|
Electron supports 3 latest releases, unfortunately they do not have a real LTS cycle like lets say Node.js. Each release is supported for around 6 months depending on Chromium release cycle. Current version is 39 which was released on 28th of October. Current releases, version 36 is now EOL with version 40 stated for release on 13th of Jan next year.
Note how EOL date lines up with +3 release version so 36 EOL date equals 39 release date. They are very strict in their support policy, any issues raised for EOL versions of Electron get an immediate reply along the lines of "this version of Electron is no longer supported try to reproduce the issue using supported version". Electron releases: https://releases.electronjs.org/ |
|
One note about version selection: The selection dropdown is just a convenience feature, you can also specify different (=newer) versions in the .csproj file and it will work fine (just shown empty then in the project designer). Only requirement: It must be specified as a 3-part version number. Specifying just two parts (like 37.1) will make electron-builder choke (unable to download), that's where the dropdown comes-in handy, allowing you to see only the available versions. Getting the selection list dynamically updated at the user side is not possible without delivering a full project system (that would need to be delivered as a Visual Studio Extension MSIX), so the next best possible way is the Python script that is next to the XAML definition. It can be run manually or we could also have it been executed by Ninja as part of the nuget package build process. |
|
I believe you are referring to a feature that is only available in Visual Studio, there are no version dropdowns in other IDEs and not sure where they are configured. Could you point to a place in the code where the versions are listed? All I can find in solution files is version 30.4.0 specified in multiple location but no list. I am probably looking in the wrong location. My suggestion was to move from version 30.4.0 to a supported version of Electron, 39 would be ideal since it is the latest version and will have support until May next year. Later versions can be incremented. |
|
In Visual Studio, all the Electron.NET MSBuild properties can be set nicely in the project designer: 
. This UI is defined in the 
. The defaults are defined in the Summary of where the files are effective
|
|
I see, unfortunately other IDEs do not pick up those options. 
Options for setting Electron version are documented however my suggestion was to have the default to be Electron 37-39, preferably 39. Not entirely sure if there is a point to have every single Electron revision listed in .xaml file would it not make more sense to have the latest revision of a specific major version only? |
|
Generally, if you ever want to work on any of the MSBuild stuff, I think it is mandatory to do this from within Visual Studio. It is easy to break things and also difficult to understand how this stuff is working. You also need to have and use the BinLog viewer. Finally, the "Project System Tools 2022" Visual Studio Extension is useful to get logs from design-time builds. By hitting the "Show all files" button in the Solution Explorer, you get to see the "Imports" node, which allows you to inspect the whole tree of .props and .targets that are used, and in which order they are imported: 
. This tree is not VS specific - it's MSBuild logic and the same for all IDEs (but only VS shows it to you like this). |
I do not mind changing the default.
Generally, the point is that each version has been the latest one at some point in time, and there can be good reasons for why you don't want to change that, that's my point of listing all versions and never remove any, because then it would no longer be shown in the app designer page. It's just a cosmetic thing, not a functional one and for the first-time release, we could surely drop all the older ones. |
|
I think it is important to include all developers that use different tools and especially different OSes for dev. Visual Studio is only available on Windows.
To be fair version selector is not something that is absolutely essential and documentation makes it rather clear how to change version.
I would not say that it is hugely important the only up side is a much cleaner version dropdown showing only latest major versions. I do not mean that all old versions should be removed but having just the latest for the older version would make it more readable. On a much lighter note I wish the dropdown was an actual issue - working on refactoring tasks and it's giving me a major headache with naming conventions seemingly changing between each file 🥲 |
I'd say that primarily important for consumers of Electron.NET, not that much for working on the project itself, but anyway, you'll probably not be doing major MSBuild code changes and for everything else it's fine not to have VS., |
|
@softworkz I believe you mentioned something about optimising Electron app lifetime. I know that you have written about it but not sure which of the very long threads to look at. Is this something you also added to your commit? |
softworkz
left a comment
softworkz
left a comment
There was a problem hiding this comment.
This PR wasn't ready for merge, but no big deal, it can be fixed.
|
|
||
| internal static class ApiEventManager | ||
| { | ||
| internal static void AddEvent(string eventName, object id, Action callback, Action value, string suffix = "") |
There was a problem hiding this comment.
Object ids should be int, not object. int is what GetHashCode() returns
| { | ||
| if (callback == null) | ||
| { | ||
| BridgeConnector.Socket.On(eventName + id, () => { callback(); }); |
There was a problem hiding this comment.
You need int.ToString(CultureInfo.InvariantCulture), otherwise, for some languages, integers will have punctuations (throusand separators) with the default .ToString() - which is called for + concatenations.
| { | ||
| if (callback == null) | ||
| { | ||
| BridgeConnector.Socket.On(eventName + id, () => { callback(); }); |
There was a problem hiding this comment.
callback invocation needs null-check - like callback?.Invoke()
|
|
||
| internal static class ApiEventManager | ||
| { | ||
| internal static void AddEvent(string eventName, object id, Action callback, Action value, string suffix = "") |
There was a problem hiding this comment.
The major problem here is that callback will always be null when entering this function, because callback += value; will not modify the delegate of the caller, only the local one.
This means effectively, that once a second event registration is made, the first one will stop working.
| BridgeConnector.Socket.Emit($"register-{eventName}{suffix}", id); | ||
| } | ||
| callback += value; | ||
| } |
There was a problem hiding this comment.
Generally, the pattern here is not suitable, you should rather build a dictionary of event delegates - ideally a ConcurrentDiictionary like I did for the properties, because that will also help with concurrency (the current pattern is not thread-safe).
There was a problem hiding this comment.
Thinking about it, a ConcurrentDiictionary doesn't help here, because it doesn't allow you to synchronize removals (to make sure you unregister socket events without that a new registration happens in parallel).
You'll need a regular Dictionary with loccking instead.
3 participants
Currently events and tasks have a lot of repetitive code, this RP attempts to remove some of the repetition starting with events.
Changes summary
ApiEventManagerto wrap events code.App.cs,BrowserWindow.cs,Tray.cs,AutoUpdater.cs,NativeTheme.cs,PowerMonitor.cs,Screen.cs,WebContents.csTrayandScreenAPI.Different event naming and Tray API changes
Currently Electron.NET API has multiple event naming schemes making it harder to implement a single method for wrapping events, examples below. Event names in .NET API correspond to event names in TypeScript/JS implementation.
BrowserWindowAPI does not add any additional names to events.AppAPI adds-eventtoEmit()method.TrayandScreenAPI was adding-eventtoOn()method.In order to keep different method signatures to a minimum I have renamed Tray and Screen API events in line with BrowserWindow.
This is a work in progress but would like to get some feedback to make sure the change is acceptable.