vslavik / winsparkle

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

Added limited support to the sparkle:tag criticalupdate #244

Closed edwinclement08 closed 2 years ago

edwinclement08 commented 2 years ago

Updated the xml parser with a simple checker for whether criticalupdate tag is present at present, it only detects the tag in this form, though it can be extended to support the way sparkle 2 does with a bit of effort.

 <sparkle:tags>
     <sparkle:criticalupdate/>
 </sparkle:tags>

p.s. this does not parse the specific version string embedded in the criticalupdate tag. it also doesn't deal with the case where criticalupdate tag appears outside of sparkle:tags

On detecting this particular tag in the appcast.xml, the installer wizard will make the following changes to stateupdateavailable

edwinclement08 commented 2 years ago

Sorry about this, its not ready for review. Made a PR by mistake. Will clean it up and reopen

edwinclement08 commented 2 years ago

Re-opening this, fixed code according to your comments, @Youw. Also created this wiki entry as docs

vslavik commented 2 years ago

Updated the xml parser with a simple checker for whether criticalupdate tag is present at present, it only detects the tag in this form, though it can be extended to support the way sparkle 2 does with a bit of effort.

What is the reason for doing it this way instead of supporting only Sparkle 2 way?

edwinclement08 commented 2 years ago

@vslavik , honestly speaking, that was just time constraint and not a very good handle on writing C code.

vslavik commented 2 years ago

@vslavik , honestly speaking, that was just time constraint and not a very good handle on writing C code.

@edwinclement08 So if I update the code to use Sparkle 2 way only, that wouldn't cause you any issues?

edwinclement08 commented 2 years ago

Would be grateful for the help, but I would like it if this is merged first. We need it in our workplace and the effort might take a couple of weeks to get sorted out.

vslavik commented 2 years ago

Would be grateful for the help, but I would like it if this is merged first.

I'd rather not merge things only to immediately remove them. Looking at the docs, it's actually simpler to extract <sparkle:criticalUpdate> from one level up than descending into now-obsolete <sparkle:tags>, so I see little reason to support the legacy syntax.

edwinclement08 commented 2 years ago

@vslavik, If you are busy otherwise, can you please merge my PR? That, even in its current limited form, will be able to help some users.

Youw commented 2 years ago

If your project is blocked by this change, you can always use your own fork instead of the original upstream. Assepting half-solution is usually a bad idea and a nightmare for the developer to support.

vslavik commented 2 years ago

@vslavik, If you are busy otherwise, can you please merge my PR?

I added some code review comments now. If you want this merged faster, please address them. They're minor details, though, the major issues preventing merging of this PR are the ones already discussed:

  1. We should not use the legacy <sparkle:tags>, but should use the Sparkle 2 interface.
  2. Disabling the close button is not OK.
vslavik commented 2 years ago

VS2022 project support cherry-picked in b246977b.

vslavik commented 2 years ago

We should not use the legacy , but should use the Sparkle 2 interface.

Oh, the code actually already does recognize both of the variants.

I committed this as 7ca9b991, with changes from the review. I also simplified the appcast parsing parts (the code tracked descent into <sparkle:tags> but never actually used that information — turns out it supported the Sparkle 2 way already!). I fixed some typos. I radically simplified the UI part where much fewer changes were required. I also removed the hiding of close button — it really is too user hostile. The diff is much smaller now and may not look like it, but it wouldn't get there without your v1 of it.

Thanks for the code and thanks @Youw for your review.