PICARD-1045: Add program update checking #904

rdswift · 2018-07-17T21:46:37Z

Summary

This is a…
- Bug fix
- Feature addition
- Refactoring
- Minor / simple change (like a typo)
- Other
Adds checking for updated version using the GitHub api.

Problem

There is currently no system included to allow checking for available updates, and for automatically notifying the user when a new version of the software is available.

JIRA ticket: PICARD-1045

Solution

Added update checking class, which calls the GitHub api to get the latest available versions (i.e.: stable and beta) for Picard and compares them to the version of the currently running instance. If an update is available, the user will be shown a dialog box with an option to download the new version.

Added a manual call to the update checker under the Help section of the main toolbar.

Added two new checkboxes to the General options tab to enable automatic checking during program startup, and whether to also check for beta (dev) versions. Automatic checks will be silent and not display check results if no update is available.

Added one new spinbox to the General options tab to select the number of days to wait between automatic update checks during program startup.

Action

No additional action required.

samj1912 · 2018-07-18T01:55:29Z

I would like Picard Website to have an end point to control updates instead. I think you made a PR for it a while back. Why not reopen that?

@zas any thoughts on Gh vs PW for update checking?

zas · 2018-07-18T06:58:19Z

@zas any thoughts on Gh vs PW for update checking?

Using a PW endpoint would make things much more simpler, using GH leads to a lot of extra code.
So yes, i'd prefer the PW endpoint way too.

phw · 2018-07-18T08:09:07Z

picard/util/checkupdate.py

+        self._update_level = update_level
+
+        # Gets list of releases from GitHub website api.
+        output_text = _("Getting release information from GitHub.")


This and below is exactly the code from query_available_updates, should call that instead of reimplementing it here,

You're right. That was a left over from a previous version and was supposed to have been removed. I'll make the change.

Change made, but I ended up having to force push it. You should be able to see the corrected version in the current changes.

phw · 2018-07-18T08:15:33Z

Using a PW endpoint would make things much more simpler, using GH leads to a lot of extra code.
So yes, i'd prefer the PW endpoint way too.

I don't think that's much the case. Given the pull request the actual Github specific code to handle the github request and the response is something below 20 LOC in total. The rest is UI and general update handling + logging.

Our own implementation might be simpler, especially as we could give back an optimized result from PW (e.g. return only the latest version), so the code to handle the response would be simplified a bit. But on the other side there is additional code on PW.

I actually would go with the GH pages. Of course it would also be possible to revive https://github.com/metabrainz/picard-website/pull/112/files and adapt this PR to use that endpoint, I don't think that would require much code changes.

rdswift · 2018-07-18T15:29:18Z

I actually would go with the GH pages. Of course it would also be possible to revive https://github.com/metabrainz/picard-website/pull/112/files and adapt this PR to use that endpoint, I don't think that would require much code changes.

It's funny because the reason that I abandoned that PR was because of zas's comment about using GitHub instead. I assumed that using the GitHub api would result in less ongoing maintenance work because the release information would only need to be maintained in one place (i.e.: GitHub because that's where the releases were being managed). If the consensus is to use PW instead, I'll resurrect the other PR and make the necessary changes here.

zas · 2018-07-21T07:46:28Z

Picard may not be hosted on Github in the future (Microsoft bought it...)
Adding a level of abstraction through the PW web service would ease things a lot if we need to move to another hosting solution and prolly reduce the code needed inside the application.

rdswift · 2018-07-29T18:59:05Z

@phw, The changes have been made to remove the duplicated code as requested, but I ended up having to force push it. You should be able to see the corrected version in the current changes.

rdswift · 2018-07-29T19:09:02Z

@zas / @samj1912, I've changed the update version information source from GitHub to picard-website as requested. I've tested it locally based on the current code in picard-website PR #119, but won't be able to do any production testing until the picard-website PH has been merged. I thought I would post the updated code for this PR now so that we can get a head start on any reviews and requested changes, at your convenience.

phw · 2018-07-30T13:07:58Z

Code is looking good to me. I would like to run the code though to see it in action. Will do so later this week

rdswift · 2018-08-02T17:24:53Z

@phw, the picard-website api PR 119 has now been merged and moved to production, so you should be able to test the code now using the production server rather than a local test server if you like.

samj1912 · 2018-08-08T09:55:14Z

picard/ui/options/general.py

@@ -45,6 +45,10 @@ class GeneralOptionsPage(OptionsPage):
        config.TextOption("persist", "oauth_access_token", ""),
        config.IntOption("persist", "oauth_access_token_expires", 0),
        config.TextOption("persist", "oauth_username", ""),
+        config.BoolOption("setting", "check_for_updates", False),


Maybe set this to true?

I agree - true would be a better default.

I'll make the change.

samj1912 · 2018-08-08T09:55:27Z

picard/ui/options/general.py

@@ -45,6 +45,10 @@ class GeneralOptionsPage(OptionsPage):
        config.TextOption("persist", "oauth_access_token", ""),
        config.IntOption("persist", "oauth_access_token_expires", 0),
        config.TextOption("persist", "oauth_username", ""),
+        config.BoolOption("setting", "check_for_updates", False),
+        config.IntOption("setting", "update_check_days", 0),


And this to 10?

I would suggest 7 or 14 rather than 10.

I'll set the initial value to 7.

zas · 2018-08-08T10:00:52Z

picard/util/checkupdate.py

+            )
+            if self._show_always:
+                msg_title = _("Picard Update")
+                msg_text = _("Unable to retrieve the latest version informarmation.")


informarmation -> information ;)

Oops. Fat fingers... 😁

zas · 2018-08-08T10:10:58Z

picard/util/checkupdate.py

+            important=True
+        )
+
+    def _releases_json_loaded(self, response, reply, error, callback=None):


callback is unused here.
I think it should be called, if set, and error/success passed to it, so you can move config.persist['last_update_check'] = datetime.date.today().toordinal() out of this (dropping dependency on datetime module), and move it to the caller.

Okay, I've made the change as requested. If I've misunderstood, please let me know. Thanks.

zas · 2018-08-08T10:14:44Z

picard/ui/mainwindow.py

@@ -147,6 +147,9 @@ def keyPressEvent(self, event):
    def show(self):
        self.restoreWindowState()
        super().show()
+        if config.setting['check_for_updates'] and datetime.date.today().toordinal() >= config.persist['last_update_check'] + config.setting['update_check_days']:


datetime.date.today().toordinal() >= config.persist['last_update_check'] + config.setting['update_check_days']

If update_check_days is 0, and >= is used then it will check more than once per day.

So i think it should be > + ensure update_check_days is >= 0
Or >= and ensure update_check_days is >= 1

With the zero interval I had intended to allow multiple checks per day, but I see now that might not be appropriate or required in a production environment. I think your second suggested option is better because it allows checks on successive days with the 'update_check_days' setting at 1, whereas the first option will only do checks on successive days if the setting is 0. It all comes down to how you interpret the meaning of 'update_check_days'. The interpretation when I was writing it was that a value of 1 meant to check every 1 day (i.e.: every day). Following this logic, I also suggest that the lower limit in the settings be changed from 0 to 1 for the 'update_check_days' setting.

rdswift · 2018-08-08T20:04:52Z

@zas / @samj1912 / @phw / @Sophist-UK

I think I have now captured all of your requested changes based on my understanding of the discussion. Please have a look at your convenience and let me know if I've misunderstood or missed something. Thanks.

rdswift · 2018-08-14T00:48:40Z

If / when this is accepted, I have also made changes to the documentation on the picard-website and will submit a PR at that time.

phw

Ok, this is looking really good now. Just a minor thing about specifying the parent window for the dialog boxes. Apart from that I am all for merging this.

phw · 2018-08-15T18:04:06Z

picard/util/checkupdate.py

+        if key:
+            msg_text = _("A new version of Picard is available.\n\nOld version: %s\nNew version: %s\n\n"
+                         "Would you like to download the new version?") % (PICARD_FANCY_VERSION_STR, self._available_versions[key]['tag'],)
+            if QMessageBox.information(None, msg_title, msg_text, QMessageBox.Ok | QMessageBox.Cancel,


All the message boxes should specify the main window as parent. Otherwise the dialog might open completely independent of the main window, as it happened to me on my multi-monitor setup.

You can get the main window with self.tagger.window.

Done, I think. Since I wasn't importing tagger into the checkupdate module, I included the parent information in the call to the check_update() method call from the main window. Is this an acceptable alternative?

phw · 2018-08-16T05:27:20Z

I included the parent information in the call to the check_update() method call from the main window. Is this an acceptable alternative?

@rdswift Two answers to this :)

You actually really could have used just self.tagger.window. It is automagically available since Picard monkey patches QObject, see tagger.py#L172. But actually this is not the greatest design decision in regards to coupling :)
I think your approach of explicitly passing the parent is better. But I would change it to pass the parent only once in the constructor, I think this makes the code cleaner instead of passing around the parent internally to all functions needing it. You also missed one QMessageBox in _releases_json_loaded

rdswift · 2018-08-16T16:44:18Z

I never noticed that patch in tagger.py before. I still think I'd rather explicitly pass the parent than rely on a patch. Good point about passing it to the constructor. I'll make the change.

zas · 2018-08-25T20:54:33Z

picard/util/checkupdate.py

+    compare_version_tuples,
+    load_json,
+)
+import picard.util.webbrowser2 as wb2


There's only one occurence of wb2 and everywhere else we use webbrowser2.open(), so just do:

from picard.util import ( compare_version_tuples, load_json, webbrowser2, ) .... webbrowser2.open(self._available_versions[key]['urls']['download'])

You're right. The way I had it doesn't make any sense at all. The hazards of copy 'n' paste from another module.

zas · 2018-08-25T20:59:05Z

picard/util/checkupdate.py

+        # Valid keys include: 'download' and 'changelog'.  The only required key in
+        # the 'urls' dictionary is 'download'.
+
+        # Initialize empty dictionary for 'stable' key


Is this initialization really required ?
I mean you could just initialize to None and do proper tests.

Not required at all. Will set it to None and change the test as suggested.

zas · 2018-08-25T21:01:06Z

picard/util/checkupdate.py

+    @property
+    def available_versions(self):
+        '''Provide a list of the latest version tuples for each update type.'''
+        return self._available_versions


Is this property used somewhere ?
Why not rename self._available_versions to self.available_versions instead ?

Actually, it's not used anywhere, so I'll just remove it.

zas · 2018-08-25T21:02:43Z

picard/util/checkupdate.py

+                self._parent,
+                _("Picard Update"),
+                _("A new version of Picard is available.\n\n"
+                  "Old version: {picard_old_version}\n"


s/Old version/Current version/?

How about 'Your version' or 'This version' instead? When I hear the term 'Current version' I automatically think of the latest version. As well, perhaps change "New version" to "Latest version"?

zas · 2018-08-25T21:05:41Z

picard/util/checkupdate.py

+                _("A new version of Picard is available.\n\n"
+                  "Old version: {picard_old_version}\n"
+                  "New version: {picard_new_version}\n\n"
+                  "Would you like to download the new version?").format(


I'm a bit bothered by this, because if Picard was installed through a package manager it is a bad idea to suggest the user to download it.
I wonder if we should provide an easy way for packagers to disable this feature...

What do you think guys?

We could change this to something like, "Visit the website for more information?", and just link to `https://picard.musicbrainz.org/" or "https://picard.musicbrainz.org/changelog/" for stable releases (set in the PW configuration).

Another option is to leave out the link option entirely, and just have this as a notification only. If the user wants to update, they'll find a way.

Linking to the Changelog with some View recent changes title could do.

I actually would leave it as is for the case that we show update information. As a user I expect the software that has built-in update checks to actually direct me to the download.

But @zas' concern about the need for packagers to disable this is valid. As a packager I ideally would want to have an option when running the install via setup.py to completely disable this feature (no visible option, no update checking performed).

But crippling the displayed message doesn't help both cases: Distribution users still get bothered with update information even though they depend on their distribution to update the packages and have no easy way to update themselves, and users who installed manually get an update information where they then have to hunt for the download page.

Having said this IMHO we should probably merge this PR and open a separate PR for disabling the update check. I am also not really sure how to technically best solve this.

I'm not very familiar with what happens when installing from a packager, but if there is (or could be) a specific file (e.g.: PACKAGER.txt) added to the program directory, we should be able to check for that and disable the update checking and hide the options in the toolbar and settings. Would that work?

I've already coded the changes based on Picard finding a PACKAGER.txt file, and can include them here if you like. Then the other PR could just sort out the INSTALLED_BY_PACKAGER test if we wanted something different.

zas · 2018-08-26T09:23:07Z

picard/ui/mainwindow.py

+        self.check_for_update(True)
+
+    def auto_update_check(self):
+        do_auto_update_check = config.setting['check_for_updates'] and config.setting['update_check_days'] > 0 and datetime.date.today().toordinal() >= config.persist['last_update_check'] + config.setting['update_check_days']


Please use intermediate variables to shorten lines and cache config. values, this part is hard to read.

check_for_updates = config.setting['check_for_updates'] update_check_days = config.setting['update_check_days'] last_update_check = config.persist['last_update_check'] update_level = config.setting['update_level'] today = datetime.date.today().toordinal() do_auto_update_check = check_for_updates and update_check_days > 0 and today >= last_update_check + update_check_days log.debug('{check_status} start-up check for program updates. Today: {today_date}, Last check: {last_check} (Check interval: {check_interval} days), Update level: {update_level} ({update_level_name})'.format( check_status='Initiating' if do_auto_update_check else 'Skipping', today_date=datetime.date.today(), last_check=str(datetime.date.fromordinal(last_update_check)) if last_update_check > 0 else 'never', check_interval=update_check_days, update_level=update_level, update_level_name=PROGRAM_UPDATE_LEVELS[update_level]['name'] if update_level in PROGRAM_UPDATE_LEVELS else 'unknown', ))

Using the intermediate variables is a lot better. I've made the change and will upload shortly.

samj1912 · 2018-08-27T05:50:41Z

The branch looks good to me, just 1 question/doubt of sorts. Do we need to add versioning to the releases API?
Currently it is /api/releases, should it be /api/v2/releases?

zas · 2018-08-27T08:20:41Z

The branch looks good to me, just 1 question/doubt of sorts. Do we need to add versioning to the releases API?
Currently it is /api/releases, should it be /api/v2/releases?

Actually it's a good idea, more future-proof, and consistent with existing stuff.
The thing is that current API was thought for plugins only, and adding releases information is a bit hacky (in the sense it wasn't really planned).

But i agree, /api/v2/releases would be safer.

phw · 2018-08-27T12:01:39Z

But i agree, /api/v2/releases would be safer.

Why start with v2?

phw · 2018-08-27T12:04:29Z

Ah, I think I see, you consider current /api/ v1, right? Then I don't really see the point in now starting with v2 since this API update does not provide any breaking change, it is a rather clean addition to the existing API. You can still introduce v2 if something actually requires an incompatible change. But appart from that I don't really care :)

zas · 2018-08-27T15:59:04Z

Why start with v2?

It is just because we re-use API made for plugins, and last iteration is v2 ... but the main issue here is this re-use. Perhaps we need to think about this stuff, especially because plugins stuff is likely to change (again) soon...

rdswift · 2018-08-27T16:16:26Z

Are we talking about the releases endpoint on the PW api? At one point I had it set to both api/v1/releases and api/v2/releases and it was decided to remove the api version number and just go with api/releases. It's a simple change, however you decide to go.

- Change default values. Use callback for updating last check date. - Add parent window information to dialog box calls - Pass the parent information to the UpdateCheckManager constructor rather than at each call to the check_update() method. - Correct date test to allow automatic update checking on consecutive days when an intervall of 1 day is selected. - Add a new (constant) dictionary of standard update levels - Reformat debug log messages - Use new dictionary to populate update check level combobox

- Move PROGRAM_UPDATE_LEVELS to picard.constants - Logging changes - Formatting and translation clean-up - Change picard.const.PROGRAM_UPDATE_LEVELS to numeric keys - Add releases endpoint to picard.const.PLUGINS_API - Refactor to remove duplicated code in mainwindow.py - General cleanup of debug log translations - Use intermediate variables to cache config information

zas · 2018-08-28T09:52:29Z

Are we talking about the releases endpoint on the PW api? At one point I had it set to both api/v1/releases and api/v2/releases and it was decided to remove the api version number and just go with api/releases. It's a simple change, however you decide to go.

@samj1912 @phw : Re-add api/v1/releases and api/v2/releases ?

phw · 2018-08-28T11:36:41Z

@zas I vote for yes. At first I had though the plugin API would be versionless and we would only introduce this version for the releases API, but now I see we have both v1 and v2 for the plugin API. It just makes sense to have this in from the start, because it is not unlikely we will have changes to the structure.

samj1912 · 2018-08-28T19:57:55Z

Do we need v1/releases?

rdswift · 2018-08-28T20:04:06Z

In my opinion, it should start with v2/releases because that's when it was first implemented (even though it could be included as v1/releases without breaking the current v1 api). I also agree that it should be versioned.

If you all agree, I can prepare the picard-website PR and update this PR to match if you like.

zas · 2018-08-28T20:57:53Z

Do we need v1/releases?

I don't think so. Plus, apart Picard, nothing will use those endpoints for a while.
@rdswift : can you re-introduce v2/releases endpoint and update this PR accordingly ? That's the sole thing remaining before a merge.

phw requested changes Jul 18, 2018

View reviewed changes

rdswift force-pushed the update_check branch from b6ab683 to 6f7f22b Compare July 18, 2018 15:43

rdswift mentioned this pull request Jul 22, 2018

PW-62: Add latest program version information to api metabrainz/picard-website#119

Merged

5 tasks

rdswift force-pushed the update_check branch from 6f7f22b to 054a934 Compare July 29, 2018 18:49

rdswift force-pushed the update_check branch 2 times, most recently from 9c39575 to bb947be Compare August 1, 2018 19:28

samj1912 reviewed Aug 8, 2018

View reviewed changes

zas requested changes Aug 8, 2018

View reviewed changes

rdswift force-pushed the update_check branch from 09f0d02 to ecbaeb9 Compare August 8, 2018 19:34

zas previously approved these changes Aug 8, 2018

View reviewed changes

rdswift dismissed zas’s stale review via 985b648 August 10, 2018 15:52

rdswift force-pushed the update_check branch 2 times, most recently from 985b648 to 2843f7e Compare August 11, 2018 18:31

phw requested changes Aug 15, 2018

View reviewed changes

rdswift force-pushed the update_check branch from 4d15af0 to 357ede7 Compare August 15, 2018 19:13

zas requested changes Aug 25, 2018

View reviewed changes

zas requested changes Aug 26, 2018

View reviewed changes

zas previously approved these changes Aug 26, 2018

View reviewed changes

phw previously approved these changes Aug 27, 2018

View reviewed changes

rdswift dismissed stale reviews from phw and zas via e362b0e August 27, 2018 16:07

rdswift force-pushed the update_check branch from 9820441 to e362b0e Compare August 27, 2018 16:07

rdswift added 4 commits August 27, 2018 14:06

PICARD-1045: Add program update checking

1f870e4

Change update information source to picard-website api

a8171e1

rdswift force-pushed the update_check branch from e362b0e to 6447e9e Compare August 27, 2018 20:07

rdswift mentioned this pull request Aug 28, 2018

Move 'releases' endpoint to api/v2 metabrainz/picard-website#129

Merged

5 tasks

Change 'releases' api endpoint to new 'v2/releases' api endpoint

d630e3d

phw approved these changes Aug 29, 2018

View reviewed changes

Merge branch 'master' into update_check

191d7b1

zas merged commit 9d8fb10 into metabrainz:master Aug 29, 2018

rdswift deleted the update_check branch August 29, 2018 16:02

phw mentioned this pull request Nov 11, 2018

Picard 2.1.0 metabrainz/picard-website#137

Merged

17 tasks

PICARD-1045: Add program update checking #904

PICARD-1045: Add program update checking #904

Conversation

rdswift commented Jul 17, 2018

Summary

Problem

Solution

Action

samj1912 commented Jul 18, 2018

zas commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phw commented Jul 18, 2018

rdswift commented Jul 18, 2018 • edited

zas commented Jul 21, 2018

rdswift commented Jul 29, 2018 • edited

rdswift commented Jul 29, 2018

phw commented Jul 30, 2018

rdswift commented Aug 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdswift commented Aug 8, 2018

rdswift commented Aug 14, 2018

phw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phw commented Aug 16, 2018

rdswift commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdswift Aug 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samj1912 commented Aug 27, 2018 • edited

zas commented Aug 27, 2018

phw commented Aug 27, 2018

phw commented Aug 27, 2018

zas commented Aug 27, 2018

rdswift commented Aug 27, 2018

zas commented Aug 28, 2018

phw commented Aug 28, 2018

samj1912 commented Aug 28, 2018

rdswift commented Aug 28, 2018 • edited

zas commented Aug 28, 2018

rdswift commented Jul 18, 2018 •

edited

rdswift commented Jul 29, 2018 •

edited

rdswift commented Aug 2, 2018 •

edited

rdswift Aug 25, 2018 •

edited

samj1912 commented Aug 27, 2018 •

edited

rdswift commented Aug 28, 2018 •

edited