After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 641960 - Windows version auto-update
Windows version auto-update
Status: RESOLVED FIXED
Product: banshee
Classification: Other
Component: general
git master
Other Windows
: Normal enhancement
: 1.x
Assigned To: Banshee Maintainers
Banshee Maintainers
Depends on: 619684 641988
Blocks:
 
 
Reported: 2011-02-09 19:35 UTC by Ján Sokoly
Modified: 2011-03-22 21:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
The first pass on auto-update functionality. Without progress bar and i18n. (5.47 KB, patch)
2011-02-20 02:08 UTC, Ján Sokoly
needs-work Details | Review
Adds the "Check for Updates" menu item to Help menu on Windows. (8.43 KB, patch)
2011-02-22 23:07 UTC, Ján Sokoly
needs-work Details | Review
Adds the auto-update functionality with basic UI. (13.96 KB, patch)
2011-03-14 22:42 UTC, Ján Sokoly
committed Details | Review

Description Ján Sokoly 2011-02-09 19:35:37 UTC
It's a good habit to offer an auto-update functionality in Windows apps these days.
Comment 1 Matt Sturgeon 2011-02-09 21:28:05 UTC
As far as the backend is concerned, a simple function to read a text file on http://download.banshee.fm should be good enough for checking for updates (the maintainer updates the text file depending on the latest Stable and Development release).

A variable should be set in banshee's config depending on whether the user wants Stable (e.g. 1.8.x) updates or Development (e.g. 1.9.x) updates.


Also another function (switched on and off by a variable) should periodically call the first function. First at startup, then, say, every 2hrs after that (a small text file is negligible bandwidth).


For user interface we should look at what other programs have done and take inspiration.
Comment 2 Matt Sturgeon 2011-02-09 22:28:11 UTC
Actually, looking in http://download.banshee.fm, the directory structure already lends it's self to this:

/banshee/unstable/LATEST-IS-1.9.3/ is a symlink to /banshee/unstable/1.9.3/

Which means, all we have to do is check that banshee/unstable/LATEST-IS-*/


I'm working on a regex (never done one before) to filter out the version.
Comment 3 Matt Sturgeon 2011-02-10 00:03:53 UTC
ok (\d+\.)+\d should work as a regex.

what we want to do is filter "/banshee/unstable/LATEST-IS-1.9.3/" with (\d+\.)+\d and hopefully get back "1.9.3".

Except it's not quite that simple:
We need the actual download.banshee.fm server's directory structure, so we need to connect to the server, and find the literal name of ("/banshee/unstable/LATEST-IS-" + regex("(\d+\.)+\d") + "/") and put the regex bit into a variable somehow...


Need someone who knows something about C# to take over really
Comment 4 Ján Sokoly 2011-02-10 00:10:26 UTC
Well, I guess that we could read this from http://download.banshee.fm/banshee/banshee.doap and get both the latest stable and the latest unstable version numbers and download urls in an easily readable XML.
Comment 5 Matt Sturgeon 2011-02-10 00:16:14 UTC
The only problem with that is it links to the source, but if we knew the version number then we can use that alone to go to "/banshee/" + variable1 + "/LATEST-IS-" + variable2 + "/banshee-" + variable2 + "-windows.msi".

Also to fix this we need to make sure that the MSI can be used for updates, and still be intuitive for users.
Comment 6 Ján Sokoly 2011-02-10 00:24:04 UTC
That's correct, I didn't realize those are links to the source tarballs. Anyway, if we read both version numbers from the XML, we can construct the download links directly, e.g. "/banshee/[variable1]/[variable2]/banshee-1-[variable2].msi". No need to use the "/LATEST-IS-*" symlink.
Comment 7 Matt Sturgeon 2011-02-10 00:28:37 UTC
That's a point ;)
Comment 8 Alexander Kojevnikov 2011-02-10 00:38:56 UTC
I have this functionality in Spek, feel free to borrow ideas. It's in Vala which is very similar to C#.

The check:
http://gitorious.org/spek/spek/blobs/master/src/spek-window.vala#line271

The widget:
http://gitorious.org/spek/spek/blobs/master/src/spek-message-bar.vala#line25
Comment 9 Matt Sturgeon 2011-02-13 07:29:40 UTC
I the below C# example on the web. Apparently it "Retrive the xml information contained within XML document using the url to the document itself (urlString)". Unfortunately I don't understand C# :-( , so I don't know what to tweak.


XmlDocument document = new XmlDocument();
document.Load(urlString);
XmlElement root = doc.DocumentElement;
XmlNodeList nodes = root.SelectNodes("/root/order");
 
foreach (XmlNode node in nodes)
{
    string idStr = node["order_id"].InnerText;
    string dateStr = node["order_date"].InnerText;
 
    XmlElement root2 = doc.DocumentElement;
    XmlNodeList nodes2 = root.SelectNodes("/root/order/item");
     foreach (XmlNode nodex in nodes)
    {
          string sku = nodex["item_sku"].InnerText;
    }
}


All I know is we want to extract both Release/Version/Revision fields from http://download.banshee.fm/banshee/banshee.doap, and then work out which is the development release, and which is stable - and put each into appropriate variables.
Comment 10 Ján Sokoly 2011-02-20 02:08:21 UTC
Created attachment 181374 [details] [review]
The first pass on auto-update functionality. Without progress bar and i18n.

As Bug #619684 is a blocker, this won't really work until Banshee.ServiceStack.Application.Version returns "Unknown" on Windows.
Comment 11 Matt Sturgeon 2011-02-22 13:38:04 UTC
Banshee builds fine with the patch applied, but obviously I can't test the functionality.


I didn't look particularly hard at the code, did you add any interface features? Is there a way to call up a dialog or something to update it?

I think a button to the right of the Menubar (aligned to the right edge of the Window) seams like a good idea (as we don't have to worry about Global Menubars on Windows). And also a Menu Item (something like "Check for Updates - F12") in either Tools or Help.


Should we be thinking about making this work with Mac OS X as well?
Comment 12 Ján Sokoly 2011-02-22 13:49:53 UTC
Matt, the next patch I'll upload in a few hours will add a "Check for Updates" menu item to "Help" menu right above "About". It will only show up on Windows and call the same method which gets called on Banshee startup.
Comment 13 Matt Sturgeon 2011-02-22 16:46:03 UTC
Could you also have the check occur once every 12 hrs automatically after launch (incase users never close Banshee, it never crashes, and they never find the check for updates menu entry)?

And if your feeling really creative could you ad a button to the right of the menubar (inline with it) which does the same as the menu item?
Comment 14 Ján Sokoly 2011-02-22 23:07:08 UTC
Created attachment 181652 [details] [review]
Adds the "Check for Updates" menu item to Help menu on Windows.

I've also revised the code style of the previous patch to better comply with the style guide.
Comment 15 Alexander Kojevnikov 2011-02-24 14:50:15 UTC
Review of attachment 181374 [details] [review]:

Does the CheckForUpdates() run in the main thread? If it does, it shouldn't -- the doap download can potentially block it. If it doesn't, then the MessageDialog cannot be launched from it.

By the way, are the message catalogues broken on Windows? If they are not, fix your TODOs.

Also fix code formatting, e.g. K&R brackets and spaces before '(', see HACKING for details.
Comment 16 Gabriel Burt 2011-02-24 19:10:21 UTC
Yeah, go ahead and wrap your i18n strings in Catalog.GetString (using Mono.Posix;)

Please obsolete these two attached patches, and rebase them into one new patch for review.  Thanks, getting close!
Comment 17 Matt Sturgeon 2011-02-27 00:30:29 UTC
Is this structured as an extension?

Really we want an auto-update extension (which is runtime enabled by default) so that we can just enable/disable building it for different OS's.

It might be nice to make this compatible with Mac, and Linux, so thatLlinux users can get updates strait from Banshee (if they build/download the extension).

It would be nice to make the core of this extension vaguely compatible with any binary build, so that at a later date we can adjust the infrastructure and a few settings and, for example, Mac users suddenly get auto updates.

Also this should support different release series, e.g. Nightly(latest git), Beta/RC (1.9), Stable (1.8). Ofcourse we would need to distribute those builds on the server as well for that to work, but it would be nice to have the options in the client.

So
 * Support auto-updating binary releases for any OS
 * Support allowing user to choose Nightly, Beta/RC or Stable updates
 * Structure as extension to allow dis/enable build for different OS's.

I don't know how much of the above is already implemented, but it would be nice to see it... Then we can move onto UI


 * For Mac, it would simply download the latest build image for nightly,beta,or stable to a tmp folder, then restart Banshee (in the process replacing the old image with the new). Simples.

 * For Windows, download the latest MSI for nighlty,beta,or stable and run it (it will install over the top and close automatically).

 * For linux, download a tgz of latest nightly,beta,or stable linux binaries to /tmp. Close Banshee, delete files to be replaced/removed by upgrade, and extract new binaries (depending on how the archive is formatted).
Comment 18 Gabriel Burt 2011-02-27 02:43:19 UTC
Matt, it's out of scope to even discuss Linux/Mac support for auto-updating on this bug.  This is a big enough feature on it's own, and there are so many issues with doing it on those other platforms, it's just a distraction.
Comment 19 Matt Sturgeon 2011-02-27 02:46:10 UTC
Sorry, I got carried away. What I really meant was don't put anything in that will make it too difficult to do that in the future.

On the point of this already being big/complex, would it be worth doing the UI as a separate bug when the time comes?
Comment 20 Gabriel Burt 2011-02-27 02:48:32 UTC
No, I think the UI is in scope -- shouldn't be too hard -- just a dropdown in Preferences -> General:

Check for updates to Banshee: [Stable | Unstable | Never]

Or something better than that.
Comment 21 Ján Sokoly 2011-03-14 22:42:41 UTC
Created attachment 183391 [details] [review]
Adds the auto-update functionality with basic UI.

I've pulled the code to a separate class. It now uses the Hyena Downloader (which is awesome btw.) instead of System.Net.WebClient, all the operations are non-blocking and the code style is hopefully sufficient now.
It still lacks the Preferences UI changes.
Comment 22 Matt Sturgeon 2011-03-15 10:58:02 UTC
I can't test this until later today, but I'd vote we get the preferences checkboxes (ie stable or unstable, check so often, or only manually - and hopefully an option for download automatically or ask) preferably in it's own tab, and then commit. We can.always build on the UI later. :-) good work man
Comment 23 Gabriel Burt 2011-03-15 14:55:24 UTC
Review of attachment 183391 [details] [review]:

Looks really good, Ján!  I've got quite a few style nitpicks for you...

::: src/Backends/Banshee.Windows/Banshee.Windows/VersionUpdater.cs
@@ +59,3 @@
+            downloader.Uri = new Uri (download_url + doap_filename);
+            downloader.TempPathRoot = Path.GetTempPath ();
+            downloader.Finished += new Action<HttpDownloader> (DoapDownloader_Finished);

two style things:  1) don't need the "new Action<..>" part, it's implied,  2) rename method to OnDoapDownloaderFinished

@@ +62,3 @@
+            downloader.Start ();
+
+            temp_doap_path = downloader.LocalPath;

why do you assign this here, and also above?

@@ +69,3 @@
+            if (obj.State.FailureException != null) {
+                if (verbose) {
+                    DisplayMessage (Catalog.GetString ("Can't check for updates"), Catalog.GetString ("We're currently not able to check if there's a new version available. Please try again later."), MessageType.Error);

Wrap lines at 120 chars

@@ +72,3 @@
+                }
+            }
+            else {

put else { on same line as }

@@ +73,3 @@
+            }
+            else {
+                XDocument doap = XDocument.Load (temp_doap_path);

use 'var' for more readable code

@@ +74,3 @@
+            else {
+                XDocument doap = XDocument.Load (temp_doap_path);
+                XNamespace nsDoap = XNamespace.Get ("http://usefulinc.com/ns/doap#");

name variables with_underscores not camelCase.

@@ +75,3 @@
+                XDocument doap = XDocument.Load (temp_doap_path);
+                XNamespace nsDoap = XNamespace.Get ("http://usefulinc.com/ns/doap#");
+                unstable_version = (from p in doap.Descendants (nsDoap + "Version")

Can you rewrite this LINQ in the method form, eg .Where ().Select ().First () ?

@@ +111,3 @@
+
+                HttpFileDownloader downloader = new HttpFileDownloader ();
+                downloader.Uri = new Uri (downloadUrl);

use 'var' here, and the property/ctor style, eg var downloader = new HFD () {\n Uri = ..,\n TempPathRoot = ..

@@ +121,3 @@
+
+                job = new DownloadManagerJob (this)
+                {

put this { on same line as `new`

@@ +122,3 @@
+                job = new DownloadManagerJob (this)
+                {
+                    Title = Catalog.GetString (String.Format ("Downloading Banshee-{0}.msi", unstable_version)),

You can't Catalog.GetString anything but a literal and have it get translated right.  Change this to

// Translators: {0} is the filename, eg Banshee-1.9.5.msi
Title = String.Format (Catalog.GetString ("Downloading {0}"), String.Format ("Banshee-{0}.msi", unstable_version"));

@@ +133,3 @@
+        {
+            string downloaded = Math.Round (obj.State.TotalBytesRead / 1024d / 1024d, 1).ToString ("F1");
+            string todownload = Math.Round (obj.State.TotalBytesExpected / 1024d / 1024d, 1).ToString ();

change todownload to total

@@ +175,3 @@
+            else {
+                // delete the downloaded installer as the user does not want to update now
+                File.Delete (temp_installer_path);

Just because they don't want to update now, we probably shouldn't delete the installer.  Conversely, we should delete the installer after we update to it.  Maybe we can make these changes in a subsequent patch though.
Comment 24 Gabriel Burt 2011-03-22 21:10:17 UTC
Comment on attachment 183391 [details] [review]
Adds the auto-update functionality with basic UI.

Ok, I pushed this and a couple fixup commits on top of it.