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 681104 - display-config: learn to save various access information
display-config: learn to save various access information
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: --
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2012-08-03 02:20 UTC by Marc-Andre Lureau
Modified: 2016-03-31 14:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Check that the programmer is doing the right thing (1.18 KB, patch)
2012-08-03 02:20 UTC, Marc-Andre Lureau
committed Details | Review
display-config: learn a few more types (1.73 KB, patch)
2012-08-03 02:20 UTC, Marc-Andre Lureau
committed Details | Review
display-config: learn to save various access information (6.44 KB, patch)
2012-08-03 02:20 UTC, Marc-Andre Lureau
none Details | Review
vnc: save access time (801 bytes, patch)
2012-08-03 02:20 UTC, Marc-Andre Lureau
reviewed Details | Review
spice: save access time (1.11 KB, patch)
2012-08-03 02:20 UTC, Marc-Andre Lureau
reviewed Details | Review
display-config: learn to save various access information (6.44 KB, patch)
2012-08-03 02:23 UTC, Marc-Andre Lureau
none Details | Review
display-config: learn to save various access information (6.45 KB, patch)
2012-08-03 02:24 UTC, Marc-Andre Lureau
committed Details | Review

Description Marc-Andre Lureau 2012-08-03 02:20:26 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.
Comment 1 Marc-Andre Lureau 2012-08-03 02:20:29 UTC
Created attachment 220193 [details] [review]
Check that the programmer is doing the right thing

The saved property needs to be introspectable.
Comment 2 Marc-Andre Lureau 2012-08-03 02:20:34 UTC
Created attachment 220194 [details] [review]
display-config: learn a few more types
Comment 3 Marc-Andre Lureau 2012-08-03 02:20:37 UTC
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.
Comment 4 Marc-Andre Lureau 2012-08-03 02:20:40 UTC
Created attachment 220196 [details] [review]
vnc: save access time
Comment 5 Marc-Andre Lureau 2012-08-03 02:20:44 UTC
Created attachment 220197 [details] [review]
spice: save access time
Comment 6 Marc-Andre Lureau 2012-08-03 02:23:32 UTC
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.
Comment 7 Marc-Andre Lureau 2012-08-03 02:24:13 UTC
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.
Comment 8 Zeeshan Ali 2012-08-08 23:54:49 UTC
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.
Comment 9 Zeeshan Ali 2012-08-08 23:57:29 UTC
Review of attachment 220194 [details] [review]:

ACK, assuming I'll see this being used in the next patches..
Comment 10 Zeeshan Ali 2012-08-09 00:14:50 UTC
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?
Comment 11 Zeeshan Ali 2012-08-09 00:15:25 UTC
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?
Comment 12 Zeeshan Ali 2012-08-09 00:16:25 UTC
Review of attachment 220196 [details] [review]:

I think this and the next patch should be squashed with previous patch.
Comment 13 Zeeshan Ali 2012-08-09 00:16:56 UTC
Review of attachment 220197 [details] [review]:

Ditto
Comment 14 Marc-Andre Lureau 2012-08-09 12:07:46 UTC
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)
Comment 15 Marc-Andre Lureau 2012-08-09 12:17:11 UTC
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.
Comment 16 Marc-Andre Lureau 2012-08-09 12:22:37 UTC
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