vslavik / winsparkle

App update framework for Windows, inspired by Sparkle for macOS
http://winsparkle.org
Other
1.31k stars 267 forks source link

Added user callback to intercept and/or bypass default update installer behavior. #221

Closed patrickkidd closed 3 years ago

patrickkidd commented 4 years ago

Here is a cleaned up pull request. It may not be a perfect use of all of the data structures and concepts you have, but it should be workable if you want to clean it up from here.

vslavik commented 4 years ago

Thanks a lot!

The only thing I'm unsure about is how to handle errors — the one thing unaccounted for is failure to apply the update in the callback. This is currently silently ignored in the PR.

What do you think makes more sense?

  1. Don't make it possible for the callback to return "use default handling"; if the callback is set, it is always used
  2. Return a status code instead of 0/1 — "ok", "use default handler", "failure"
  3. Return 0/1 and have a separate winsparkle_* function to invoke the default handler

Personally, I think falling back to default handling is too useful to do 1, and am leaning towards 2 (to avoid convoluted calls back and forth), but as you're the one using this in practice, I'd like to know your opinion. (To be clear: I'm not asking you to do it, I'm happy to do it myself, this PR is very useful as it is already.)

patrickkidd commented 4 years ago

The callback concept actually avoids these dilemmas. It is intended to be something more of an event filter in which the user can do whatever they want and also halt the default WinSparkle workflow if they want.

...unless I am mis-hearing you. If I am, what use case are you thinking of that would require the callback to be more integrated into WinSparkle's default workflow?

It seems that the ideal design principle would be that WinSparkle would know how to handle each of the possible download payloads out there, or at least a majority of them. So I don't see it worth the effort to make this callback concept fit beautifully into the holistic WinSparkle design, but instead just to allow the user a simple escape hatch for edge cases without having to think really hard about how it fits into the flow. In short, changing the PR seems like a lot of thinking now and for future users for little gain.

vslavik commented 4 years ago

If it were a filter (i.e. delete some events, leave the rest as-is), I would agree, but it isn't: it's an override handler that performs the action instead of WinSparkle. And the action is something potentially complicated than can fail (e.g. bad update file, unforeseen permissions etc.).

Put differently: if the custom update callback fails, WinSparkle has no way of informing the user (OK, you can do it yourself, but it needlessly duplicates the UI) and more importantly, act accordingly (which differs in successful and failed cases).

patrickkidd commented 4 years ago

What would WinSparkle do with the error?

vslavik commented 4 years ago

Again: a) tell the user (kinda important) and b) see the link in my previous reply.