GNOME Bugzilla – Bug 680826
Order by most recently used
Last modified: 2016-03-31 13:57:34 UTC
Boxes collection is currently alphabetically ordered. It is more sensible and more consistant with the rest of GNOME application design to order the machines by most recently used.
Created attachment 227198 [details] [review] Move the properties sync code to DisplayConfig
Created attachment 227199 [details] [review] Move access information to DisplayConfig
Created attachment 227200 [details] [review] Rename s/DisplayConfig/BoxConfig
Created attachment 227201 [details] [review] Get rid of DisplayProperties
Created attachment 227202 [details] [review] Order by most recently used
this is mostly refactoring, for which I hope I don't have to justify myself too much, since I wrote it, and I was not so happy about it in the first place. The problem with the current code is that it started to show it's age, and some of the early design turned to be not flexible enough. Now we have a BoxConfig class that can sync properties from various objects, and we don't need to instantiate a Display object to get/set some settings, such as access time. Also we get rid of an extra weird class DisplayProperties, and we make some method part of a Display instance and protected, such as access_start () and access_finish (), which is better since only the Display should update access information.
Created attachment 227205 [details] [review] box-config: make {load,save}_property() private
Review of attachment 227198 [details] [review]: 1. Looking at this patch, I wonder if we really need so many indirections: DisplayProperties keeps a list of all saved props and then saving/loading code in display-config is given a name for which it tries to find a gobject prop, checks its type and then call the appropriate method on keyfile. Can't this saving/loading methods be virtual methods in DisplayProperties hierarchy, where each implementation simply calls the appropriate method on the keyfile as they know the type of the property being saved and which ones to save? 2. I think SavedProperty is more descriptive. SyncedProperty is not bad either.
Review of attachment 227199 [details] [review]: Seems like a simple move from class A to B (though I don't know if these classes really should be separate) so ACK, depending on outcome of review or parent commit.
Review of attachment 227199 [details] [review]: scratch what i said about these classes being separate, next patch explains what i keep on missing. :)
Review of attachment 227201 [details] [review]: Good stuff
Review of attachment 227200 [details] [review]: Yup, another good simplification of code. Keep 'em coming :)
Review of attachment 227202 [details] [review]: ::: src/collection-view.vala @@ +361,3 @@ + if (machine_a.config.access_last_time > + machine_b.config.access_last_time) + return -1; shouldn't there be: else return 1; here? @@ -357,1 +357,17 @@ - return item_a.name.collate (item_b.name); + var machine_a = item_a as Machine; + var machine_b = item_b as Machine; + if (machine_a != null && machine_b != null) { ... 14 more ... shouldn't this one return "1" and not "-1"?
Review of attachment 227198 [details] [review]: After looking at the rest of the patches, I think the complexity/indirections is much reduced but I still wonder if the code can be further simplified by IPropertiesProvider implementors saving/loading props themselves. Am I making any sense here? :) If not, ignore me on this one and just address #2 in previous comment.
Review of attachment 227205 [details] [review]: Looks good as per the previous patches.
Review of attachment 227198 [details] [review]: regarding #1, we can always improve later, this it is beyond the point of this refactoring for #2, Sync is more appropriate than Saved. I don't see why Synced would be better than Sync, no more than Binded or Bound would be better than Bind ..
Review of attachment 227202 [details] [review]: ::: src/collection-view.vala @@ +361,3 @@ + if (machine_a.config.access_last_time > + machine_b.config.access_last_time) + return -1; not here, as we order them by alphabetical order next (if they really have same access_last_time (probably 0) @@ -357,1 +357,17 @@ - return item_a.name.collate (item_b.name); + var machine_a = item_a as Machine; + var machine_b = item_b as Machine; + if (machine_a != null && machine_b != null) { ... 14 more ... we want empty name to be last, so b come after a, so return should be negative (a < b).
Review of attachment 227202 [details] [review]: ::: src/collection-view.vala @@ +361,3 @@ + if (machine_a.config.access_last_time > + machine_b.config.access_last_time) + return -1; but we are preceding to comparison by alphabetical order also in case access_time_a < access_time_b, not only if they are equal. @@ -357,1 +357,17 @@ - return item_a.name.collate (item_b.name); + var machine_a = item_a as Machine; + var machine_b = item_b as Machine; + if (machine_a != null && machine_b != null) { ... 14 more ... ah ok, i think its just a bit confusing with both ifs returning the same value.
Review of attachment 227198 [details] [review]: Depending on the context, bound/binded might actually be better than bind. :) present tense of verb is usually a good choice for function names as they are actions. Anways, nitpicks. :)
Review of attachment 227202 [details] [review]: ::: src/collection-view.vala @@ +361,3 @@ + if (machine_a.config.access_last_time > + machine_b.config.access_last_time) + return -1; hmm.. right, I need to update that.. it's late @@ -357,1 +357,17 @@ - return item_a.name.collate (item_b.name); + var machine_a = item_a as Machine; + var machine_b = item_b as Machine; + if (machine_a != null && machine_b != null) { ... 14 more ... hey, I think you are right, if this condition is reached, a is not set, and b is, so b < a, and we should return 1.
Attachment 227198 [details] pushed as 887316a - Move the properties sync code to DisplayConfig Attachment 227199 [details] pushed as c4a9eb8 - Move access information to DisplayConfig Attachment 227200 [details] pushed as 720086b - Rename s/DisplayConfig/BoxConfig Attachment 227201 [details] pushed as f4568fb - Get rid of DisplayProperties Attachment 227205 [details] pushed as 7893ba8 - box-config: make {load,save}_property() private
Created attachment 227346 [details] [review] Order by most recently used
Review of attachment 227346 [details] [review]: Looks good otherwise. ::: src/box-config.vala @@ -184,0 +184,5 @@ + + public int compare (BoxConfig other) { + // sort first by last time used ... 2 more ... don't see the need for diffing and keeping the diff: If (access_last_time > other.access_last_time) return -1; else if (access_last_time < other.access_last_time) return 1; ::: src/collection-view.vala @@ +360,3 @@ + return machine_a.config.compare (machine_b.config); + + // then by name * this 'then' seems misplaced here, i'm guessing c&p mistake.. * Do we need this code here? We don't have any other kinds of items than Machine?
Review of attachment 227346 [details] [review]: ::: src/box-config.vala @@ -184,0 +184,5 @@ + + public int compare (BoxConfig other) { + // sort first by last time used ... 2 more ... I beg to differ, using access_diff help readin the code. ::: src/collection-view.vala @@ +360,3 @@ + return machine_a.config.compare (machine_b.config); + + // then by name * "then by name", because we first order by machine.config.compare. * other kind of items: directories / groups, whatever else we may want to put there
Review of attachment 227346 [details] [review]: ::: src/box-config.vala @@ -184,0 +184,5 @@ + + public int compare (BoxConfig other) { + // sort first by last time used ... 2 more ... Never seen anyone doing a diff and comparing the diff against 0 for readability and i dont see any readability improvement but if you prefer it this way... ::: src/collection-view.vala @@ +360,3 @@ + return machine_a.config.compare (machine_b.config); + + // then by name * You want to also put a "First by..." comment above to make the conversation more complete. :) * Yeah i thought of containers while thinking more about this. Why not have a 'compare' method in Item then and have Machine override it? Machines' implementation would just be: return config.compare (other_machine.config);
Created attachment 237711 [details] [review] Order by most recently used In this version I acted on my own review comments: * CollectionItem provides a virtual 'compare' method that Machine overrides. * Compare values directly rather than comparing the diff against 0.
Review of attachment 237711 [details] [review]: ::: src/machine.vala @@ +513,3 @@ + return config.compare ((other as Machine).config); + else + return base.compare (other); I don't think this is a valid comparison. It doesn't seem to define a total order, more like a partial order. I.e. the order of the result depends in what order the machine and non-machine items appears in during the sort. For instance, if a non-machine item appears between two machines the order only depends on the basic stuff (name) whereas if the two machines happened to be next to each other it depends on the use date.
Created attachment 237853 [details] [review] collection: Order machines by last access time This version should take care of listing machines before non-machines.
I was able to test with this hack: http://paste.fedoraproject.org/4121 that adds dummy items after each machine. Will attach screenshots of this hack with and without the latest patch.
Created attachment 237854 [details] Boxes without latest patch
Created attachment 237855 [details] Boxes with latest patch
Review of attachment 237853 [details] [review]: otherwise it looks good ::: src/box-config.vala @@ +189,3 @@ + return -1; + else if (access_last_time < other.access_last_time) + return 1; Isn't this supposed to fall back to name if the access times are equal?
Review of attachment 237853 [details] [review]: ::: src/box-config.vala @@ +189,3 @@ + return -1; + else if (access_last_time < other.access_last_time) + return 1; And it does, doesn't it? if (access times are set) { if (a.access > b.access) return.. else if (a.access < b.access) return.. } The above code won't return if access times are not set or they are equal so the following code will be executed, which compares names.
Review of attachment 237853 [details] [review]: ::: src/box-config.vala @@ +189,3 @@ + return -1; + else if (access_last_time < other.access_last_time) + return 1; ah, true. I just misread the second else. sorry. Ack then
Attachment 237853 [details] pushed as 563bf0d - collection: Order machines by last access time