-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add auto updater #117
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
base: main
Are you sure you want to change the base?
Conversation
|
||
bool IUIFactory.HideReleaseNotes { get; set; } | ||
bool IUIFactory.HideSkipButton { get; set; } | ||
bool IUIFactory.HideRemindMeLaterButton { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these bools IUIFactor
instead of just normal public bool
?
Is there some reason we only want them accessed via the interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just like the pattern, discovered it while looking at some of the NetSparkle code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine if u like it, but it means you have to do the IUIFactory factory = this
and then factory.Field
instead of Field
, so seems like just more typing IMO.
|
||
public ValueTask DisposeAsync() | ||
{ | ||
_sparkle?.Dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to also de-register itself as a handler from the UserNotifier, otherwise it can't be GC'd.
Alternatively, do we really need this to be disposable?
{ | ||
if (!Handlers.TryGetValue(handlerName, out _)) | ||
{ | ||
logger.LogWarning("no action handler found for notification with name {HandlerName}, ignoring", handlerName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an exception, since it's a bug to try to show a notification with an unregistered handler.
|
||
bool IUIFactory.HideReleaseNotes { get; set; } | ||
bool IUIFactory.HideSkipButton { get; set; } | ||
bool IUIFactory.HideRemindMeLaterButton { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine if u like it, but it means you have to do the IUIFactory factory = this
and then factory.Field
instead of Field
, so seems like just more typing IMO.
public class AppDispatcherQueueManager : IDispatcherQueueManager | ||
{ | ||
private static DispatcherQueue? DispatcherQueue => | ||
((App)Application.Current).TrayWindow?.DispatcherQueue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the IDispatchQueueManager
idea, but seems like this could just extend the TrayWindow
and have it be the IDispatchQueueManager
, rather than having it on the App and having to deal with the fact that it might be null.
We have the TrayWindow as a "transient" in the dependency injection right now, but the App creates an instance that it holds forever, so we are lying to ourselves about it being transient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with this is if we define it as a singleton the DI instance tries to create it outside of the UI thread, which will crash the app. That's why all of the stuff that must be instantiated on the UI thread is marked transient, even if it typically only gets instantiated once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shit is so dumb.
Q: How do you get on the main UI thread?
A: You need a DispatcherQueue.
Q: How do you get a DispatcherQueue?
A: You have to get it from a UI Window
Q: How do you get a UI Window?
A: You have to create it from the main UI thread.
Should we be moving all this initialization of dependencies to OnLaunched
instead so it's on the main UI thread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
truly a classic Microsoft® Moment™
We could try doing that. I don't know what the DI behavior will be for a singleton <IDispatcherQueueManager, TrayWindow>
if it can still be accessed as both IDispatcherQueueManager
and TrayWindow
, but I'll give it a shot
Fixes issue where DownloadWithXOriginalContentLength and DownloadWithMismatchedContentLength tests would hang due to HTTP response streams not being properly flushed after writing data. The tests were causing CI failures with 2-minute timeouts because the HTTP client would wait indefinitely for more data when the response stream wasn't explicitly flushed. Co-authored-by: ibetitsmike <203725896+ibetitsmike@users.noreply.github.com>
thanks blink lmfao |
Adds an auto updater utility using NetSparkle.
Most of the UI matches the way it's done in NetSparkle for the built-in UIs (I referenced the Avalonia and WPF ones mostly), including most of the copy and implementation (incl. quirks). I did this in case we ever want to contribute this upstream. The only major difference is how I handle the release notes stuff.
For release notes, I use the
<description>
tag to generate HTML for the web view. There's a hardcoded """template""" in the source code and a copy of sindresorhus/github-markdown-css in the repo that gets included as an asset in the binary. This matches what we do on macOS but diverges from the way NetSparkle does release notes rendering using<sparkle:releaseNotesUrl>
(or something like that).TODO:
Follow up:
Closes #122
Closes coder/internal#703 (thanks Blink)