Add dynamic tray menu update functionality

[davidroth]

Introduced a SetMenuItems method in Tray.cs to enable dynamic updates to the tray's context menu. This method clears the existing menu items, adds new ones, and registers the necessary click handlers.

Previously, updating menu item properties (such as changing the enabled state) required deleting and recreating the entire tray. Although this was functional, it caused noticeable flickering of the tray icon because the icon was briefly removed and then re-added.

With this change, the tray icon remains constant, and only the menu items themselves are replaced.

With the existing api

await Electron.Tray.Show(ImagePath("Assets/electron.ico"), menuItems)

// Disabling an existing menu item in the tray by dropping and recreating the entire tray:
menuItems[0].Enabled = false;

await Electron.Tray.Destroy()
await Electron.Tray.Show(ImagePath("Assets/electron.ico"), menuItems)

With the new api

await Electron.Tray.Show(ImagePath("Assets/electron.ico"), menuItems)

// Disabling an existing menu item in the tray by  replacing the menu items  only.
menuItems[0].Enabled = false;
await Electron.Tray.SetMenuItems(items);

[FlorianRappl]

Must be rebased to develop.

[davidroth]

rebase done

[FlorianRappl]

For me this is fine - I'd like to have @softworkz opinion especially regarding potential cleanups / memory leaks on this one.

[davidroth]

For me this is fine - I'd like to have @softworkz opinion especially regarding potential cleanups / memory leaks on this one.

Yeah, I thought about leaks too, but I think it is safe. The c# sides socket callback gets remove fist (BridgeConnector.Socket.Off), so there we have proper cleanup of the previous handler.

On the node side, replacing the trays menu via tray.value.setContextMenu(menu) should make the old object unreachable, which should gc it including the created closures in addMenuItemClickConnector.

In any case, if @softworkz can look at it as well, that would be super.

[davidroth]

@FlorianRappl Is there any docu regarding the branching strategy? What is main used for vs develop? When are the branches synced? And why shoud we target develop if main is the default branch? (Couldnt find any docs ... ) Thanks! 👍

[davidroth]

Looks like the tests are flaky 😕

[FlorianRappl]

@FlorianRappl Is there any docu regarding the branching strategy? What is main used for vs develop? When are the branches synced? And why shoud we target develop if main is the default branch? (Couldnt find any docs ... ) Thanks! 👍

If I could change it I would make develop the default branch. main is the branch for the latest release, while develop is the active development branch.

I wanted to reference the CONTRIBUTING.md, but - much to my surprise - found out we don't have one... Shame on me! Good point - that should be added (it will be similar to, e.g., https://github.com/AngleSharp/AngleSharp/blob/devel/.github/CONTRIBUTING.md) soon. Thanks for bringing this up @davidroth !

[FlorianRappl]

Looks like the tests are flaky 😕

Some of the npm downloads did not go well (npm responded with a 503). This can (and realistically, unfortunately, will) always happen. No software is developed in a vacuum.

Besides such errors (npm / package download not working) the runners are also not 100% reliable regarding execution of headless GUIs. So far the tests are rather stable in that region, but we have seen some issues in the past, too.

[softworkz]

For me this is fine - I'd like to have @softworkz opinion especially regarding potential cleanups / memory leaks on this one.

Please give me a little bit more time

[softworkz]

All good! Thanks a lot for the contribution.

[softworkz]

Looks like the tests are flaky 😕

Some of the npm downloads did not go well (npm responded with a 503). This can (and realistically, unfortunately, will) always happen. No software is developed in a vacuum.

My suspicion is that this (503 gateway timeout) might happen due to protection at the side of npm which is limiting to many concurrent requests from the same source. For our tests, 14 action runners are working in parallel and - at least same OS types - may get to the moment doing 'npm install' more or less simultaneously.

My PR #977 adds a random delay of 0-20 seconds - maybe this helps a bit...

It also adds another "retry failed" workflow which does the same as when you manually go and click "re-run failed jobs" (but only once).
This shoiuld help exactly for this situation. It's not about failed tests. Normally those jobs are done in a way that they fail when a single test fails, but I've configured it in a way that the jobs will only fail when there's an execution error. Failed tests are surfaced in the Test Results job only - which is failing when one or more tests have failed.
The auto-rerun only looks for the matrix tests, not for the latter.

Besides such errors (npm / package download not working) the runners are also not 100% reliable regarding execution of headless GUIs. So far the tests are rather stable in that region, but we have seen some issues in the past, too.

I had thought the same, but it has turned out that all these were about timing only.

[FlorianRappl]

I'm not sure about the timing. If it would be rate limiting then 503 is the wrong status code. Also in a matrix test each run is performed on a different agent, so in general different IPs. If npm would go crazy because 15 machines install packages in parallel then we can shut down the JS ecosystem.

[softworkz]

If it would be rate limiting then 503 is the wrong status code.

Holding the connection and timing out after some time is a well-known technique for defeating attacks (leaving a tcp connection open to see how long the other party will hold on to it)

If npm would go crazy because 15 machines install packages in parallel then we can shut down the JS ecosystem.

The runners are behind a NAT of course, they don't have all their own public IPs.

[softworkz]

But of course I'm not sure either wether the delay will be of any help...

[FlorianRappl]

The runners are behind a NAT of course, they don't have all their own public IPs.

Sure not everyone has a dedicated IP but in general you will find multiple public IPs for the whole range of runners. So the likelihood of having at least 2 or 3 IPs when running on 15 different agents parallel is quite high I'd say (even though I am not sure how GitHub does that - for the Azure Pipelines agents this is true).

While I agree on the holding the connection technique it is still not common to have a 503 here. It still would rather be a 504 or something else. Granting that npm went briefly down in the last couple of days (with 503 errors - I've seen those in some other pipelines that are not hosted on GitHub) I'd rather point the issue there.

[softworkz]

I have read somewhere that they have proxies for the DevOps runners to avoid downloading the same things a thousand times, and "gateway timeout" is a typical proxy error - which could masquerade the actual error from npm. But it could also be a problem with npm's CDN (also something where´"gateway timeout" can be seen). But well - it's all just speculation...