Abstract out osu specific stuff from UpdateManager #24484
Conversation
The first step in allowing rulesets to use the same GitHub update mechanism as an interim remedy to providing timely updates to players.
| /// <remarks> | ||
| /// Implementers are expected to set <see cref="Version"/> to something that matches their release versioning/tagging system. | ||
| /// </remarks> |
There was a problem hiding this comment.
This is a sign of bad design and needs to be handled better. Constructor, abstract property, whatever, but consumers should be forced to set this - you shouldn't need to rely on a loose setter only mentioned in passing in the remarks section.
| // For lack of a better term | ||
| protected abstract string UpdatedComponentName { get; } |
There was a problem hiding this comment.
The lack of a better term implies overengineering. It means that it's no longer really clear what's being abstracted over.
|
|
||
| return bestAsset?.BrowserDownloadUrl ?? release.HtmlUrl; | ||
| } | ||
| protected virtual string GetBestURL(GitHubRelease release) => release.HtmlUrl; |
There was a problem hiding this comment.
Why virtual? Why is both this and UpdateActionOnClick() virtual? Means that overriding the latter potentially makes this completely useless.
| /// <summary> | ||
| /// An update manager which only shows notifications after a game update completes. | ||
| /// </summary> | ||
| public partial class GameUpdateManager : UpdateManager |
There was a problem hiding this comment.
These classes just keep on coming...
- Why does this exist at all?
- Why is
SquirrelUpdateManagernot aGameUpdateManager, given it updates the game and shows the notification afterwards?
This class needs to not exist, ideally.
| /// </summary> | ||
| public partial class NoActionGameUpdateManager : SimpleGameUpdateManager | ||
| { | ||
| protected override string DownloadMessage => "Check with your package manager / provider to bring osu! up-to-date!"; |
There was a problem hiding this comment.
These types of overrides are horrible for localisation purposes later on. It would be better to allow derived classes to specify the full message they are supposed to show (at the cost of copy duplication) rather than try and splice localisable strings together.
|
As for more constructive feedback after pointing out the issues: What seems to be desired here is to reuse "fetch releases for arbitrary repo from github and show a notification" logic, and also have a way to hook that into the game. If that goal is what is desired here, then I'd strongly urge considering composition over inheritance. Have something like a I'm not even really sure we need to rope any ruleset updaters - if we're even doing that, since this entire series is completely unprompted and has been preceded by no discussion - into the game update flow. Maybe it can be a separate thing without introducing a common giga-abstraction. |
|
Your suggestions do indeed sound like a better fit, and I'll look into that direction. As for the discussion before implementation part, I'll keep that in mind the next time I make a big change like this. 🙇 |
|
Loose thought, bu it isn't really obvious to me that we want to have ruleset updates potentially restart or close the game the same way an actual game update does. It would be very neat to get ruleset update-and-hot-reload to work. I'm not sure if it's feasible, but if it is, it may end up being a completely different API to the game updater. I'd probably explore the options there and see what ends up happening with that. |
|
So I redone this with composition in mind, and indeed it does seems much cleaner than my initial attempt. A few things of note:
I read into the possibility of having hot reload. To be able to unload an ruleset, we would need to not have any active references to any type/code from the ruleset. I think a simple way to allow for seamless updates of rulesets would be to run the update process as part of the loading process, since at that point we can be sure that we are not using any types from the ruleset assembly such as icons or settings subsections. Example flow for the above: For light and relaxing read: https://learn.microsoft.com/en-us/dotnet/standard/assembly/unloadability |
Opening this PR mainly for discussion on direction and implementation.
This serves as an initial step to allow rulesets to use the GitHub release notification code to announce that they have updates. (As an interim solution while waiting for ruleset listing to be available)
UpdateManageris the abstract base class for all implementations, and doesn't provide any functionality whatsoever. The post-update notification is now implemented withinGameVersionUpdater(not a fan of the name), which is used by(*)GameUpdateManager.I also took the liberty of simplifying
NoActionUpdateManager, as that shared a lot of code withSimpleUpdateManager.Rudimentary manual testing of
SimpleGameUpdateManagerseems to show that everything seems to be behaving as expected.