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

Expose why new update is unavailable #1877

Merged
merged 4 commits into from
Jun 27, 2021
Merged

Expose why new update is unavailable #1877

merged 4 commits into from
Jun 27, 2021

Conversation

zorgiepoo
Copy link
Member

@zorgiepoo zorgiepoo commented Jun 27, 2021

I want to expose the reason why a new update is unavailable to the user driver and provide the latest appcast item found. Additionally for fun I'm also trying out to expose some of this to the user in the standard user driver.

If the latest appcast item has release notes available and the user is up to date, we can provide a link to the change-log (not done for inline release notes and I don't want to expose another web view).

If the latest appcast item has a link, we can provide a link to the website if the user is on a non-eligible OS version.

Would potentially resolve #1604

Checklist:

  • My change is being tested and reviewed against the Sparkle 2.x branch. New changes must be developed on the 2.x development branch first.
  • My change is being backported to master branch (Sparkle 1.x). Please create a separate pull request for 1.x, should it be backported. Note 1.x is feature frozen and is only taking bug fixes, localization updates, and critical OS adoption enhancements.
  • I have reviewed and commented my code, particularly in hard-to-understand areas.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • My change is or requires a documentation or localization update - yes, for providing a link to appcast items in the min/max OS case at least. New localization string too for version history.

Testing

I tested and verified my change by using one or multiple of these methods:

  • Sparkle Test App
  • Unit Tests
  • My own app
  • Other (please specify)

Modified test app to test both cases. Example:

Screen Shot 2021-06-26 at 6 34 17 PM

macOS version tested: 11.5 Beta (20G5042c)

When user is on version >= latest appcast feed item, we give them an option to view version history.

When user can't update because of a min/max OS requirement, we give them an option to view product's website.
@zorgiepoo zorgiepoo merged commit 6639fea into 2.x Jun 27, 2021
@zorgiepoo zorgiepoo deleted the update-not-found-why branch June 27, 2021 22:13
@aonez
Copy link
Contributor

aonez commented Oct 27, 2021

@zorgiepoo I love this enhancement, thanks for that. Let me suggest a possible improvement to the "Version History" button.

In my use case releaseNotesLink points to a crafted page with the latest version info only and limited styling that better fits the Sparkle dialog. With your feature the user goes to that page when clicking on the "Version History" button. In my case is: https://u.keka.io/changelog.php

I'm suggesting to create a new versionHistoryLink key or similar and if it's informed use that one for that button. In my case will be: https://changelog.keka.io

I think this will enhance you already great enhancement.

Let me know what you think. I can create a PR if needed.

@billymeltdown
Copy link
Contributor

I’ve also been wondering how to best handle the Version History button for about the same reason (and agree this enhancement in 2.x is really nice!)

Like @aonez, our releaseNotesLink also “points to a crafted page with the latest version info only and limited styling that better fits the Sparkle dialog.” We’ve had one user email us to say they thought this behavior (not presenting a full version history) was incorrect. However, rather than an online URL with the full version history, our app has an in-app release notes window with the full version history, and it would be nice if we could present that in response to the Version History button’s IBAction. I took a look at that spot in the code wondering if it might be a good place for delegation, but perhaps a versionHistoryLink property in the appcast.xml is cleaner, I could point it at our app with a custom URL scheme to do the right thing (e.g. myapp://version-history/).

@zorgiepoo
Copy link
Member Author

zorgiepoo commented Oct 28, 2021

Ahh thanks :), it's great to finally receive feedback on this. Let's try to address these cases with this new feature.

Both a new appcast element, and SPUStandardUserDriver delegation approaches sound good to me on their own. Maybe we can do both if needed (one allows server to be in control of link, other allows client to use custom behavior.. arguably Sparkle could show release notes in the app but well.. Sparkle doesn't and this would be out of scope).

@billymeltdown Do you always ship offline release notes in your app? Just curious what other use case you use it for and how common this may be.

We can't use a custom URL scheme (eg myapp://) approach, Sparkle blocks unknown handlers as far as I recall. That seems hacky to me too.

"versionHistoryLink" might be too synonymous sounding to "releaseNotesLink", not sure if "fullReleaseNotesLink" is better distinguishing, but this is just a naming problem (unrelatedly I chose "Version History" over "Changelog" in the alert button as I thought it would be more user-friendly..). A new element may need generate_appcast support to adding the property and a small website documentation update too.

Not sure if we need to care about localization here too like we do with releaseNotesLink currently (it would only be complicated in generate_appcast, not the appcast spec itself). We could potentially leave that omitted for now.

PRs are welcome here.

@zorgiepoo
Copy link
Member Author

We can't use a custom URL scheme (eg myapp://) approach, Sparkle blocks unknown handlers as far as I recall. That seems hacky to me too.

Err never mind this is only from a web view which isn't the case here.. This would work, actually... But it's roundabout and delegate would probably be better in offline case.

@aonez
Copy link
Contributor

aonez commented Oct 28, 2021

I could do the fullReleaseNotesLink PR.

I wasn't aware of the localization in the link... maybe we can skip this and leave it to the server to decide what language to show, since the link it's opened on the browser instead of in-app.

@billymeltdown
Copy link
Contributor

Err never mind this is only from a web view which isn't the case here.. This would work, actually... But it's roundabout and delegate would probably be better in offline case.

Yeah, a delegate method would be preferable, agree that it's a better solution. Will try to take a look at this today or tomorrow morning.

@billymeltdown Do you always ship offline release notes in your app? Just curious what other use case you use it for and how common this may be.

Since we started using Sparkle 1.x way back when, been a long time now! It's nothing fancy, and just in this one app. I had wanted to have full version history available in the app, separate from the html content set up for displaying information about the current update in the sparkle updater window, and I didn't want to use a web view. In retrospect I might have been better off maintaining this as a long html file to publish on our website and host in a web view. Some of the versions listed display a more info button that links out to a longer release notes post about a major update, and some versions display a feature highlight button that launches a new window with more information, like a wizard.

Hope I'm not being spammy by including a screenclip, feel free to remove:

Screen Shot 2021-10-28 at 11 55 52 AM

@zorgiepoo
Copy link
Member Author

I could do the fullReleaseNotesLink PR.

I wasn't aware of the localization in the link... maybe we can skip this and leave it to the server to decide what language to show, since the link it's opened on the browser instead of in-app.

Sounds good. It's not a problem in the appcast btw, you'll get <fullReleaseNotesLink xml:lang=".."> for free.

Yeah, a delegate method would be preferable, agree that it's a better solution. Will try to take a look at this today or tomorrow morning.

Also sounds good and cool to see your use case. Thinking more, I'll probably even want to block/ignore non-http(s) URLs when constructing the appcast items as it could be nefarious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it easier to access update notes, especially after an update has been downloaded/installed
4 participants