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

PICARD-1045 Add update checking framework #876

Closed
wants to merge 2 commits into from
Closed

PICARD-1045 Add update checking framework #876

wants to merge 2 commits into from

Conversation

rdswift
Copy link
Collaborator

@rdswift rdswift commented Mar 11, 2018

Summary

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Add a framework for checking program updates by scraping the latest release version information from the Picard home page. Add manual check to toolstrip under the "Help" section.

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.

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:
Copy link
Member

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.

Copy link
Collaborator Author

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 = '''
Copy link
Member

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.

Copy link
Collaborator Author

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'])
Copy link
Member

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.

Copy link
Collaborator Author

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.

@rdswift
Copy link
Collaborator Author

rdswift commented Mar 11, 2018

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',)
Copy link
Collaborator

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)

Copy link
Collaborator Author

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)
Copy link
Collaborator

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...")

Copy link
Collaborator Author

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a global mandatory ?

Copy link
Collaborator Author

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.

@rdswift
Copy link
Collaborator Author

rdswift commented Mar 12, 2018

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.

@zas
Copy link
Collaborator

zas commented Mar 13, 2018

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.

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 ?
Also "There are JIRA tickets requesting that Picard provide some form of new version availablity checking." please link to actual tickets...

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.

@rdswift
Copy link
Collaborator Author

rdswift commented Mar 13, 2018

Will re-think the process and create a new PR after the 2.0 release.

@rdswift rdswift closed this Mar 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants