Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix restart notifications appearing every 30 minutes in some cases #29787

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

peppy
Copy link
Member

@peppy peppy commented Sep 8, 2024

If a user was to manually check for updates via the button, the recheck would have been fired. This is a recent regression.

I kinda want to reorganise this code (the button press for check for udpates shouldn't even get close to the recheck code IMO) but for now this seems like one we should quickly fix.

I've tested normal update flow still works okay. I haven't waited 30 minutes to check this works, but don't see why it wouldn't.

Addresses #29774.

If a user was to manually check for updates via the button, the recheck
would have been fired. This is a recent regression.

I kinda want to reorganise this code (the button press for check for
udpates shouldn't even get close to the recheck code IMO) but for now
this seems like one we should quickly fix.

Addresses ppy#29774.
@peppy peppy changed the title Fix restart notifications appearing every 30 minutes Fix restart notifications appearing every 30 minutes in some cases Sep 8, 2024
@bdach
Copy link
Collaborator

bdach commented Sep 8, 2024

Logic seems fine on a read-through, but after the last fiasco I'm reluctant to approve this without testing. That said I'm not sure how much of this is even testable. Even with a few local modification velopack protests that it's not an installed app, so I'll probably have to do nine-yards testing with osu-deploy.

@peppy
Copy link
Member Author

peppy commented Sep 8, 2024

Even with a few local modification velopack protests that it's not an installed app, so I'll probably have to do nine-yards testing with osu-deploy.

For me all I had to do was dotnet run in osu-deploy (maybe different on windows vs macOS though). And to reiterate, I did test this except for waiting 30 minutes.

@bdach
Copy link
Collaborator

bdach commented Sep 8, 2024

In testing with osu-deploy with this applied:

diff --git a/osu.Desktop/Updater/VelopackUpdateManager.cs b/osu.Desktop/Updater/VelopackUpdateManager.cs
index c2965428f7..848153d772 100644
--- a/osu.Desktop/Updater/VelopackUpdateManager.cs
+++ b/osu.Desktop/Updater/VelopackUpdateManager.cs
@@ -45,6 +45,7 @@ private void load(INotificationOverlay notifications)
 
         private async Task<bool> checkForUpdateAsync(UpdateProgressNotification? notification = null)
         {
+            Logger.Log("Checking for updates.");
             // whether to check again in 30 minutes. generally only if there's an error or no update was found (yet).
             bool scheduleRecheck = false;
 
@@ -119,7 +120,7 @@ private async Task<bool> checkForUpdateAsync(UpdateProgressNotification? notific
             {
                 if (scheduleRecheck)
                 {
-                    Scheduler.AddDelayed(() => Task.Run(async () => await checkForUpdateAsync().ConfigureAwait(false)), 60000 * 30);
+                    Scheduler.AddDelayed(() => Task.Run(async () => await checkForUpdateAsync().ConfigureAwait(false)), 5000);
                 }
             }
 

this does appear to spam notifications less.

So this is probably fine. One thing I'm not 100% on is whether this code path should set scheduleRecheck to true too:

// In the case of an error, a separate notification will be displayed.
notification.FailDownload();
Logger.Error(e, @"update failed!");

but maybe fine?

@peppy peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Sep 9, 2024
@peppy peppy merged commit 02f8f22 into ppy:master Sep 10, 2024
13 checks passed
@peppy peppy deleted the fix-restart-notifications branch September 17, 2024 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:installer next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants