-
-
Notifications
You must be signed in to change notification settings - Fork 400
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
PICARD-1045 Add update checking framework #876
Conversation
Add a framework for checking program updates by scraping the latest release version information from the Picard home page. Add manual check to toolbar under the "Help" section. Partially address PICARD-1045: Check for new version
''' | ||
latest_version = get_latest_version_number() | ||
msg_title = "Picard Update" | ||
if latest_version > PICARD_VERSION_STR_SHORT: |
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.
Please don't do ordered comparison on strings containing version data, it will not work:
In [1]: "3.1" > "3.10"
Out[1]: False
In [2]: "3.2" > "3.10"
Out[2]: True
Unfortunately I don't know of any library that makes comparing version numbers in Python easy.
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'll come up with something based on the version_to_string and version_from_string code in the picard/init.py file.
|
||
_RE_TEST = r'.*infoText\s*=\s*\"v([0-9\.]*)' | ||
|
||
_NOTICE_TEXT = ''' |
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 value of this variable should be wrapped in _()
, so it can be translated.
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.
Will do. Should I also do something similar with the strings for the error messages?
global latest_version | ||
if not latest_version: | ||
try: | ||
page = urllib.request.urlopen(PICARD_URLS['home']) |
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.
Please only use Picards webservice
module for performing HTTP requests. It's the only way to ensure a not-so-fast request won't block the whole application.
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.
Will do. Sorry, I should have thought of that myself.
I'm going to put the changes on hold pending the outcome of PW-62 (picard-website PR # 112). That will likely result in a significant rewrite of this PR. |
else: | ||
if show_always: | ||
msg_text = "There is no update currently available. The latest release is %s." % ( | ||
'v' + latest_version if latest_version else 'Unknown',) |
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.
Buillding strings like this doesn't play well with i18n and 'Unknown'
isn't translated here.
You have to do something like:
if latest_version:
msg_text = _("There is no update currently available. The latest release is v%s.") % latest_version
else:
msg_text = _("There is no update currently available. The latest release is unknown")
QMessageBox.information(None, msg_title, msg_text, QMessageBox.Ok, QMessageBox.Ok)
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.
Will do. I'll also translate the error message text lines.
latest_version = get_latest_version_number() | ||
msg_title = "Picard Update" | ||
if latest_version > PICARD_VERSION_STR_SHORT: | ||
msg_text = _NOTICE_TEXT % (PICARD_VERSION_STR_SHORT, latest_version) |
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.
If _NOTICE_TEXT
is only used once, move the text here and get rid of this variable.
For translation, use _("...your...string...")
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.
Will do.
|
||
def get_latest_version_number(): | ||
'''Scrapes the Picard home page to extract the latest release version number.''' | ||
global latest_version |
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.
Is a global mandatory ?
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 was using it to hold the value so that subsequent calls are not made to the web site. I'm open to suggestions for an alternate way of doing this. I don't particularly like using a global either.
It seems that not everyone on the Picard leadership team shares the same vision of the end product, which makes trying to provide code a challenging task. I've tried to summarize the issues and proposal in the gist at https://gist.github.com/rdswift/6d6651a0a6c5ae1b5d619f6faab32a9b and would appreciate your comments and input. Thanks. |
Commenting a gist is a challenging task too. Why don't you post this on forum or as google docs so it is easier to comment / cite ? If you refer to recent "beta1" it is an external version i used as a marketing tool... internal version is 2.0.0dev4, since Picard doesn't actually support beta in the name. Since we are in feature freeze stage, any change to version naming or handling is very unlikely to happen before 2.0. But if you want to contribute, you may want to look at recent bug reports with high priority, about Picard not working at all, crashing, etc... that would be much more useful at this stage. Please discuss https://tickets.metabrainz.org/browse/PICARD-1045 in issue comments, and attach documents there if any. |
Will re-think the process and create a new PR after the 2.0 release. |
Summary
Problem
Picard does not include a system to check for program updates.
Solution
Add a framework for checking program updates by scraping the latest release version information from the Picard home page. This partially addresses PICARD-1045: Check for new version
This framework includes a new module with two functions. The first function gets the latest release version number by scraping the Picard home page. The information is stored in a module-level variable for re-use, thus avoiding additional calls to the web site during the same Picard session.
The second function compares the latest release version number (calling the first function if necessary) with the version number of the currently running program. If the latest release version is higher than the currently running version, a dialog box is displayed informing the user of the new version available and offering to open the Picard website to facilitate the download. Normally, nothing is displayed if there is no new version available (to allow for scheduled automatic update checks as requested in PICARD-1045); however, there is an override option to force displaying a dialog box with the check results even if there is no update available (for use when an update check is manually requested by the user).
In addition, an item has been added to the toolstrip under the "Help" section to allow the user to manually request an update check.
Action
None. An option for automatic checking, as requested in PICARD-1045, will be covered in a future pull request.