search

Floogen / Stardrop

Stardrop is an open-source, cross-platform mod manager for the game Stardew Valley.
https://floogen.gitbook.io/stardrop/
GNU General Public License v3.0
110 stars 30 forks source link

Add downloads panel #192

Closed pingzing closed 2 months ago

pingzing commented 3 months ago

Hello! I recently discovered Stardrop, and in the process of getting my mods set up with the new Stardew Valley 1.6 update, noticed a distinct lack of feedback when installing the relatively large SDV: Expanded mod. Since I'm familiar with C# and Avalonia, I thought I'd take a crack at adding an in-progress Downloads panel. And, well, here we are.

I figure that a picture is worth a thousand words, so before I continue, have a look at what I've wrought: downloadspanel_finished

And the GIF seems to compress colors a bit, so here's a more accurate look at the panel, in all five states a download can be in: image

Apologies in advance for the frankly massive PR. I'll do my best to add some guiding notes here in the description, and inline in the files themselves.

The Big Stuff

Nexus Refactoring

Strictly speaking this wasn't entirely necessary. But I saw a lot of opportunity to simplify it, and improve performance. The big thing here is that the Nexus static class is now a thin wrapper around NexusClient, which is now an instanced class, and does all the communication with Nexus's backend. The win here is that we instantiate one HttpClient, and reuse that over the application's lfietime, per Microsoft's performance guidelines. It also means we don't need to pass API keys around anymore--they get embedded into the created NeuxsClient.

NexusClient's DownloadFileAndGetPath() got a pretty significant rewrite to support progress reporting and cancellation.

NexusClient no longer takes a direct reference to MainViewModel, instead preferring to fire events about DailyRequests information that anyone else can choose to listen to.

MainWindow.axaml.cs Refactoring

A lot of centralization and consolidation happened here. There was a lot of duplication around handling the connection to Nexus, and initializing both that and the client that talked to Nexus. I did my best to de-duplicate as much as I could. Most of it now happens in a combination of SetupNexusConnection() and NexusClientChanged(). In addition, checking for a valid connection to Nexus is now a little more implicit--if Nexus.Client is non-null, any other part of the program is allowed to assume that things are good to go. CheckForNexusConnection() does still call /validate against Nexus periodically, and will null out the NexusClient if it detects that the key is no longer valid.

I also did some minor UI refactoring in the status bar--the "Check for Mod Updates" button is now a <Button> with the hyperlink class, rather than a MenuItem, and is now inside the grid with the rest of the status bar elements.

All the new Download panel stuff

I'll add iniline notes for these, but the basic flow goes like this:

The new UI should support all the existing themes, and I think got all the strings properly localized.

If you've got any questions, do please ask--I know this thing is massive, and will be a beast to review. I'd be happy to help.

Miscellaneous thoughts

It looks like the project doesn't have a central way of registering or obtaining services, so I did my best to work within those constraints--this PR is big enough, and I didn't want to add a bunch of infrastructure. As a result, the code I wrote uses lots of C# events to communicate across application boundaries. And they're a fine mechanism, but they have a few drawbacks: subscribers need a hard reference to the thing firing the event, and need to explicitly unregister their listeners, or you get memory leaks.

I think I was careful enough to make sure anything that needs to unsubscribe its event listeners does so properly, but it's definitely possible I missed some places. Keep an eye out for that.

EDIT: Now that I think about it further, this project does use ReactiveUI, and I probably could have gone the Observable route. I've found that making Observables cross boundaries between different application concerns tends to be kind of viral though. Once you use them in one place, you have to use them in everything that touches that. And Observables tend to be a big ol' rabbit hole of complexity unless you're reasonably familiar with them (and even then...). They do have a lot of upsides though, if you're okay with the complexity. They're a lot more composable than events, and easier to pass around. They do still suffer from the "you have to unsubscribe" problem, though it manifests a little differently.

In most other MVVM frameworks, I'd use some flavor of MessengerService to pass information around in a less tightly-coupled way, but it seems that ReactiveUI considers that an approach of last-resort. So I guess in a general sense, it comes to down to preference: events, RxUI's MessageBus, or follow RxUI's guidance, and let the Observables flow.

Floogen commented 3 months ago

Firstly, thank you for the pull request!

This looks amazing and is a much needed feature! Stardrop is a mess internally (I started this project to learn Avalonia UI) and I know my code must have been a challenge to work around.

I am unfortunately a bit short on time until the weekend, but I will start reviewing this late Friday / early Saturday. Thank you again for all your efforts!

pingzing commented 2 months ago

Awesome! Made the converter name change, and did some shenanigans to make sure the "Downloads" button listens for language selection changes. It basically just listens for both the PropertyChanged callback from Translation, and the current download count changing, but it does so WITH OBSERVABLES =D

(see what I mean about how they're viral?)

Floogen commented 2 months ago

Wonderful! Those Observables are definitely useful; I will have to make use of them in the future!

I have merged this over and will have it included in the next update, likely tomorrow.

Thank you again for all your efforts in adding this feature to Stardrop!