Grouping publisher-ids together and then sorting each of those by release date makes sense to me.

So, it has been pointed out that release dates are stored out of band in a database and not in the ISO itself. So, this won't help us sort or differentiate nightly snapshot type or similar frequent releases.

(In reply to William Jon McCann from comment #2)
> So, it has been pointed out that release dates are stored out of band in a
> database and not in the ISO itself. So, this won't help us sort or
> differentiate nightly snapshot type or similar frequent releases.

Yeah but I think for those we'd just want to look at the file's timestamps.

    I tried to sort the OSs based on the release dates provided by libosinfo. It was just a one line fix and all went well, except the fact that the "get_release_date" method belonging to the OS object doesn't return any value. I have tested it using mint, xubuntu, fedora and windows xp. Is this happening because of missing data from the database? 
    Also, is the publisher_id of the OSs saved somewhere or I should make a resources file with it?

(In reply to Visarion Alexandru from comment #4)
>     I tried to sort the OSs based on the release dates provided by
> libosinfo. It was just a one line fix and all went well, except the fact
> that the "get_release_date" method belonging to the OS object doesn't return
> any value. I have tested it using mint, xubuntu, fedora and windows xp. Is
> this happening because of missing data from the database? 

That's very likely the case. The osinfo database is in the form of XML files (in the source code, they are .xml.in that you'd want to edit though) that you can easily add/edit data to/in. I wrote this guide once about that:

http://zee-nix.blogspot.co.uk/2012/06/adding-information-to-libosinfo.html

Things have changed a bit since then but most of it should still apply.

Created attachment 324597 [details] [review]
Sort OSs based on publisher-id and release-date

Patch 1/2

Display in a correct manner any OS older than 5 months. 

Next step - update the database.

Created attachment 324635 [details] [review]
Sort newest OS first. Group by publisher

*Fiexed minor coding style issues

(In reply to Visarion Alexandru from comment #6)
> Created attachment 324597 [details] [review] [review]
> Sort OSs based on publisher-id and release-date
> 
> Patch 1/2

The patch to libosinfo database will have to go to libosinfo mailing-list, since it's a separate (non-GNOME) project where they prefer patches on mailing-list.

Review of attachment 324635 [details] [review]:

Shortlog:

* More meaningful log please (What new method?).
* Space after ":"

Description:

* "Use present tense" for changes by the patch itself.
* Addition of a private method itself is not worthy of commit log mention, you should summarize in English the change itself.

::: src/media-manager.vala
@@ -217,3 @@
         }
     }
-    private static int compare_media_by_publisher_id_and_release_date(InstallerMedia media_a, InstallerMedia media_b) {

So this commit actually assumes the existing of your previous commit. I'm guessing you are very new to git. What you are supposed to do is to modify the existing commit. Please read-up on usage of `git commit --fixup`,  `git commit --ammend` and `git rebase -i --autosquash`.

Created attachment 324687 [details] [review]
Sort newest OS first. Group by publisher

It groups the OSs based on it's family {"linux","microsoft"}.
I am not sure if you actually meant the distros {"fedora","solaris","windows"} ?

Review of attachment 324687 [details] [review]:

Short log is starting to improve quite a lot already :) but it's still very vague. "Fix" can mean a lot of things and you're not really fixing it, but rather just improving, which is also pretty vague btw so don't just use that. :) Also you are not changing OSs but rather media list.

::: src/media-manager.vala
@@ +217,3 @@
         }
     }
+    //Sorts the media by it's publisher-id and after that, by release date 

* i don't see the code looking at release date at all, which it's supposed to and this comment claims that it does.

* Many coding-style issues, mainly lack of spaces and separation between functions.

* This function does not arrange anything so you should name it _____ ? (HINT: existing code).

@@ +227,3 @@
+            return -1;
+        else {
+            if(strcmp(media_a.os.get_family(),media_b.os.get_family()) == 0)

Not family, publisher as in vendor on OsinfoOs associated with the media.

(In reply to Zeeshan Ali (Khattak) from comment #11)
> Review of attachment 324687 [details] [review] [review]:
> 
> ::: src/media-manager.vala
> @@ +217,3 @@
>          }
>      }
> +    //Sorts the media by it's publisher-id and after that, by release date 
> 
> * i don't see the code looking at release date at all, which it's supposed
> to and this comment claims that it does.

You probably missed it :)

+if(strcmp(media_a.os.get_family(),media_b.os.get_family()) == 0)
+                return -compare_media_by_release_date(media_a,media_b);


The method which compares the media by release date is already implemented, so I just made use of it.
The only issue here was to group the media by vendor, so I [should] first sort by vendors and only if the vendor is the same, I sort the media by the release date

Created attachment 325049 [details] [review]
Sort the media list by vendor and release date

Created attachment 325052 [details] [review]
Sort the media list by vendor and release date

*small changes

Review of attachment 325052 [details] [review]:

* Shortlog
  * Has wrong prefix.
  * Use '&' to try to keep under 50 chars.
  * On the same note, you can ditch 'date' from the end.

We are not changing the way we compare media but rather how we sort them, comparison is the way we do it.

A better description would be "Sort media by vendors, and by release dates within each vendor." IMO.

Please don't forget '.' at the end of sentences. I know that's not important at the end of last sentence but it just gives perfectionists like me, a headache. :)

::: src/media-manager.vala
@@ +218,3 @@
     }
 
+    private static int compare_media_by_vendor (InstallerMedia media_a, InstallerMedia media_b) {

A short comment here specifying the top-level logic here, would be nice. i-e what comes first etc. See similar comments elsewhere in this class.

@@ +227,3 @@
+            return -1;
+        else {
+            if(strcmp (media_a.os.get_vendor (), media_b.os.get_vendor ()) == 0)

* Would be efficient to use a variable here to avoid comparing strings twice.
* space after 'if'. Since spaces are something you struggle with a lot, I'd advice to put that down as a checklist to compare each line with, before submitting your patches. After a few times, you'll not need the checklist. :)

@@ +228,3 @@
+        else {
+            if(strcmp (media_a.os.get_vendor (), media_b.os.get_vendor ()) == 0)
+                //In case of duplicate, prefer earliest release date

* Same vendor doesn't mean media is duplicate.
* Space after //.

@@ +229,3 @@
+            if(strcmp (media_a.os.get_vendor (), media_b.os.get_vendor ()) == 0)
+                //In case of duplicate, prefer earliest release date
+                return -compare_media_by_release_date (media_a, media_b);

why are you inverting the results? the compare functions are already written for the kind of comparison needed here.

@@ +230,3 @@
+                //In case of duplicate, prefer earliest release date
+                return -compare_media_by_release_date (media_a, media_b);
+            else return -strcmp (media_a.os.get_vendor (), media_b.os.get_vendor ());

codying-style: Bodies are never on the same line as associated if/else.

Created attachment 325559 [details] [review]
Sort media by vendors, and by release dates within each vendor

Created attachment 325993 [details] [review]
media-manager: Sort media by vendors and release

Sort the media by vendors and, in case of identical vendors,
by the release dates within each vendor.
---

Re-uploading to see if it makes any difference to Splinter somehow.

(In reply to Zeeshan Ali (Khattak) from comment #17)
> Created attachment 325993 [details] [review] [review]
> media-manager: Sort media by vendors and release
> 
> Sort the media by vendors and, in case of identical vendors,
> by the release dates within each vendor.
> ---
> 
> Re-uploading to see if it makes any difference to Splinter somehow.

Interesting, so this works and also it seems that you are not only one whose patches are having this issue. I was informed that your patches are in the wrong format. If you had simply used git-bz as I suggested, we would have avoided this.

Is it possible that you are doing anything to the patch files before uploading them (e.g openning them in some editor that modifies them)?

Created attachment 326167 [details] [review]
media-manager: Sort media by vendors and release

Sort the media by vendors and, in case of identical vendors,
by the release dates within each vendor.

You were right, the errors were probably coming from the fact that I was giving the patches one last view with my editor to see if any other changes should be made. (I wasn't actually modyfing them, just opening and closing with Sublime).
Thank you for your patience.. it seems the changes are visible in the splinter now.

Review of attachment 326167 [details] [review]:

Looks good otherwise.

::: src/media-manager.vala
@@ +231,3 @@
+
+            if (vendor_comparison == 0)
+                // In case of identical vendor, prefer earliest release date

* We want latest first. The fact that you had to invert the return value of 
compare_media_by_release_date() below, gives you a hint that you are doing it opposite to what we want. :)

* Comment(and commit log) makes it sound like vendor being identical is an issue, while it's the norm. You could say "Within each vendor, list latest first".

Review of attachment 326167 [details] [review]:

::: src/media-manager.vala
@@ +231,3 @@
+
+            if (vendor_comparison == 0)
+                // In case of identical vendor, prefer earliest release date

It may be counterintuitive, but it seems to be the exact opposite. The GLib.Date.compare method chooses the "smaller" date to be in front of the list (this can be understood from its documentation).
My intuition and the tests I have made led me to the conclusion that "smaller" date means an older date.
Our compare_media_by_release_date method never inverts the results, so that logic is passed on.

It may be a dormant bug that couldn't be seen because the compare_media_by_release_date wasn't used until now.

Review of attachment 326167 [details] [review]:

::: src/media-manager.vala
@@ +231,3 @@
+
+            if (vendor_comparison == 0)
+                // In case of identical vendor, prefer earliest release date

* compare_media_by_release_date() *is* already used. If it wasn't, vala would warn about a private API never being used.

* You are correct about return value of GLib.Date.compare() but it should be compare_media_by_release_date() that should be fixed then as it's supposed to list newest first as you can tell from the comment on top of it's only current usage:

                        // In case of duplicate media, prefer:
                        // * released OS over unreleased one
                        // * latest release

Where unreleased is assumed to be one without a release date.

So a patch to correct compare_media_by_release_date() to go before this patch, would be desirable here. :)

Created attachment 326348 [details] [review]
media-manager: Fix compare method

The problem is that the method that compares media by release date
does not prefer the latest releases, as intended, but rather the
oldest ones.

Invert the return value of the method that compares media by release
date, when dealing with released media.

Created attachment 326359 [details] [review]
media-manager: Sort media by vendors and release

Sort the media by vendors and, within each vendor,
by the latest release dates.

Created attachment 326400 [details] [review]
media-manager: Invert release-date based sorting

We were sorting media in chronological order rather than in (intended)
reverse-chronological order.

Co-author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org>

Created attachment 326401 [details] [review]
media-manager: Sort media by vendor & release date

Sort the media by vendor and, within each vendor, by the release date.

Attachment 326400 [details] pushed as 3469fe9 - media-manager: Invert release-date based sorting
Attachment 326401 [details] pushed as 6b5c666 - media-manager: Sort media by vendor & release date