GNOME Bugzilla – Bug 681104
display-config: learn to save various access information
Last modified: 2016-03-31 14:02:20 UTC
We will save a few useful informations for sorting / statistics: - time spent on a box - number of time using a box - last time access - first time access I wanted to implement the base class as a mixin, but vala/gobject is a bit too limited for that sadly. Not a big deal though. In the future, I would like to extend this approach to support Zeitgeist informations too. Also, it might be interesting that we save these information in libvirt XML too, although it's perhpas preferable to have a Boxes level of thing, that can work with direct or ovirt connections too.
Created attachment 220193 [details] [review] Check that the programmer is doing the right thing The saved property needs to be introspectable.
Created attachment 220194 [details] [review] display-config: learn a few more types
Created attachment 220195 [details] [review] display-config: learn to save various access information We will save a few useful informations for sorting / statistics: - time spent on a box - number of time using a box - last time access - first time access I wanted to implement the base class as a mixin, but vala/gobject is a bit too limited for that sadly. Not a big deal though. In the future, I would like to extend this approach to support Zeitgeist informations too. Also, it might be interesting that we save these information in libvirt XML too, although it's perhpas preferable to have a Boxes level of thing, that can work with direct or ovirt connections too.
Created attachment 220196 [details] [review] vnc: save access time
Created attachment 220197 [details] [review] spice: save access time
Created attachment 220198 [details] [review] display-config: learn to save various access information We will save a few useful informations for sorting / statistics: - time spent on a box - number of time using a box - last time access - first time access I wanted to implement the base class as a mixin, but vala/gobject is a bit too limited for that sadly. Not a big deal though. In the future, I would like to extend this approach to support Zeitgeist informations too. Also, it might be interesting that we save these information in libvirt XML too, although it's perhpas preferable to have a Boxes level of thing, that can work with direct or ovirt connections too.
Created attachment 220199 [details] [review] display-config: learn to save various access information We will save a few useful informations for sorting / statistics: - time spent on a box - number of time using a box - last time access - first time access I wanted to implement the base class as a mixin, but vala/gobject is a bit too limited for that sadly. Not a big deal though. In the future, I would like to extend this approach to support Zeitgeist informations too. Also, it might be interesting that we save these information in libvirt XML too, although it's perhpas preferable to have a Boxes level of thing, that can work with direct or ovirt connections too.
Review of attachment 220193 [details] [review]: ::: src/display-config.vala @@ +62,3 @@ + var property = display.get_class ().find_property (property_name); + if (property == null) { + debug ("You forgot the property '%s' needs to have public getter!", property_name); * From what I can tell this 'display' object belongs to external libraries (spice and vnc) so 'You' won't be very correct here. * For things like these, we shouldn't go too far (otherwise we'll end-up adding way too much debug and at some point it starts to become counter-productive) and just have a 'requires', 'return*_if_fail' or assert.
Review of attachment 220194 [details] [review]: ACK, assuming I'll see this being used in the next patches..
Review of attachment 220199 [details] [review]: Looks good. ::: src/display.vala @@ +3,3 @@ +// too bad we can't make it just a mixin +private abstract class Boxes.DisplayProperties: GLib.Object, Boxes.IPropertiesProvider { Probably missing out something obvious but why do we need a new intermediate class?
Review of attachment 220196 [details] [review]: I think this and the next patch should be squashed with previous patch.
Review of attachment 220197 [details] [review]: Ditto
Review of attachment 220193 [details] [review]: ::: src/display-config.vala @@ +62,3 @@ + var property = display.get_class ().find_property (property_name); + if (property == null) { + debug ("You forgot the property '%s' needs to have public getter!", property_name); * it's a boxes property that needs to be public * there is already return_if_fail() generated by vala but it took me a while to realize what was going on, so I would prefer to have a debug() there. (tbh, I prefer debug() rather than static // comments in general)
Review of attachment 220199 [details] [review]: ::: src/display.vala @@ +3,3 @@ +// too bad we can't make it just a mixin +private abstract class Boxes.DisplayProperties: GLib.Object, Boxes.IPropertiesProvider { IPropertiesProvider is a Gtk thing, it doesn't need to be mixed with DisplayConfig, a simple IConfig. DisplayProperties is specialized to sync-bind Display properies with DisplayConfig file with help of DisplayProperties.SavedProperties. I agree this is not all perfect design, but when there is a natural split, we should follow it.
Attachment 220193 [details] pushed as c23e2c1 - Check that the programmer is doing the right thing Attachment 220194 [details] pushed as 7070931 - display-config: learn a few more types Attachment 220199 [details] pushed as c6576d7 - display-config: learn to save various access information