Created attachment 172878 [details] [review]
Use non recursive automake for the library

Switching to non recursive automake, so that all header files are defined
in one Makefile.
This is a necessary step for GObjectIntrospection to work, but should also
help future maintenance of the library (no need to distinguish libgnome vs
libgnomeui).

Created attachment 172879 [details] [review]
Use non recursive automake for the library

Switching to non recursive automake, so that all header files are defined
in one Makefile.
This is a necessary step for GObjectIntrospection to work, but should also
help future maintenance of the library (no need to distinguish libgnome vs
libgnomeui).

Created attachment 172942 [details] [review]
Turn GnomeRRScreen into a GObject

Rework GnomeRRScreen so that it is a full GObject, and all its data
is moved to GnomeRRScreenPrivate.
GObject's are more supported when it comes to introspection and bindings,
in particular wrt constructors. Therefore, this is a necessary step
for useful introspection generation.

Review of attachment 172942 [details] [review]:

This is a very interesting patch.  It would be perfect if GnomeRR were a public API, but it is intended to be used only by gnome-settings-daemon's RANDR plugin and gnome-display-properties from the control center.

I don't think we need to worry about sanitizing GnomeRR for public consumption (or at least, not just yet).  The public API for doing RANDR stuff is the D-Bus API (see #623223 for a proposed extension).

Well, in addition to gnome-control-center and gnome-settings-daemon, another user of GnomeRR may be gnome-shell, which will show the screen indicator in the system status area.
This is why it is particularly important for GnomeRR to be introspected and accessed from bindings.

Of course, a native DBus based binding would fit, but why would you introduce a new API, with IPC overhead, when there is a library that works just as well?

Oh, yeah, I forgot about gnome-shell...

Would you have time to make the corresponding changes in gnome-settings-daemon/plugins/xrandr and gnome-control-center/capplets/display (or whatever the directory is called these days), to be able to see how the new API looks like from clients?

(I like your patch and it should go in, if gnome-shell will end up using it.  For now I don't want to break anything in g-s-d and g-c-c, so let's get that working before pushing the patch to gnome-desktop.)

Actually, since the whole contents of the GnomeRRScreen struct is private, and the only way to create it is gnome_rr_screen_new, which is preserved with the same signature and behaviour, no API changes are introduced by this patch, which should just work on master g-s-d and g-c-c

OK, that's good to know.

Hmm, now that I think about it, the really important API there is GnomeRRConfig.  *That* is what g-s-d and g-c-c use to look at the RANDR configuration.  The GnomeRRScreen API is lower-level stuff; it is also used, but the main entry points are in GnomeRRConfig.

If gnome-shell wants to implement its own status icon for displays, then it will definitely want to use GnomeRRConfig.  I wonder how many changes that needs to be binding-friendly - it uses things like arrays in structs that you iterate by hand, etc.

(Maybe it's easier to just bind that by hand in gnome-shell itself?  No idea.)

Created attachment 173191 [details] [review]
Add support for GObjectIntrospection

Turned all GnomeRR structures into boxed types, then added the needed
annotations and the Makefile.am bits.
Does not yet include API changes, but should bind (awfully) all of
libgnome-desktop.

Created attachment 173831 [details] [review]
Turn GnomeRRConfig and GnomeOutputInfo into GObjects

For easier binding and introspectability, rework GnomeRRConfig and
GnomeOutputInfo to be GObjects (GInitables, actually) instead of
boxed types. This commit *does* break API, as previous API just
accessed fields in the public structs, while now everything has been
moved to private structures and accessors must be used.
Also, rework GnomeRRLabeler to use both a public and a private structure,
so that gobject-introspection can find instance/class sizes.
Modifications to gnome-control-center and gnome-settings-daemon will
follow.

Need to have patches for gnome-settings-daemon as well.

(In reply to comment #2)
> Created an attachment (id=172879) [details] [review]
> Use non recursive automake for the library
> 
> Switching to non recursive automake, so that all header files are defined
> in one Makefile.
> This is a necessary step for GObjectIntrospection to work, but should also
> help future maintenance of the library (no need to distinguish libgnome vs
> libgnomeui).

Gah, that's something I dislike a bit -- could be because I'm not used to this, but my experience with non-recursive automake has been a bit painful in the past :/

I'm not sure how it's really required for g-i support. In the worst case, we can move the headers in the same directory as the c files (which makes sense anyway).

(In reply to comment #12)
> (In reply to comment #2)
> > Created an attachment (id=172879) [details] [review] [details] [review]
> > Use non recursive automake for the library
> > 
> > Switching to non recursive automake, so that all header files are defined
> > in one Makefile.
> > This is a necessary step for GObjectIntrospection to work, but should also
> > help future maintenance of the library (no need to distinguish libgnome vs
> > libgnomeui).
> 
> Gah, that's something I dislike a bit -- could be because I'm not used to this,
> but my experience with non-recursive automake has been a bit painful in the
> past :/
 
Why so?
Non recursive automake is actually easier than recursive one, IMHO.

> I'm not sure how it's really required for g-i support. In the worst case, we
> can move the headers in the same directory as the c files (which makes sense
> anyway).

You need all the headers defined in one place, and that place must be the same of sources for g-i to work.
Anyway, will move the headers to libgnome-desktop/.

(As a side note, maybe it's time to drop libgnome/ and libgnomeui/ and just install in $(includedir)/gnome-desktop-3/)

Created attachment 173984 [details] [review]
Turn GnomeRRConfig and GnomeOutputInfo into GObjects

For easier binding and introspectability, rework GnomeRRConfig and
GnomeOutputInfo to be GObjects (GInitables, actually) instead of
boxed types. This commit *does* break API, as previous API just
accessed fields in the public structs, while now everything has been
moved to private structures and accessors must be used.
Also, rework GnomeRRLabeler to use both a public and a private structure,
so that gobject-introspection can find instance/class sizes.
Modifications to gnome-control-center and gnome-settings-daemon will
follow.

Created attachment 173985 [details] [review]
Move include files to libgnome-desktop/

gobject-introspection requires all header and source files to be
defined in one place. This is one way to achieve that, without
using non recursive automake.

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #2)
> > > Created an attachment (id=172879) [details] [review] [details] [review] [details] [review]
> > > Use non recursive automake for the library
> > > 
> > > Switching to non recursive automake, so that all header files are defined
> > > in one Makefile.
> > > This is a necessary step for GObjectIntrospection to work, but should also
> > > help future maintenance of the library (no need to distinguish libgnome vs
> > > libgnomeui).
> > 
> > Gah, that's something I dislike a bit -- could be because I'm not used to this,
> > but my experience with non-recursive automake has been a bit painful in the
> > past :/
> 
> Why so?
> Non recursive automake is actually easier than recursive one, IMHO.

Well, the fact that I'm used to type make in subdirs to build a subpart of a modules, and my experience with webkitgtk which just takes ages to build ;-)

> > I'm not sure how it's really required for g-i support. In the worst case, we
> > can move the headers in the same directory as the c files (which makes sense
> > anyway).
> 
> You need all the headers defined in one place, and that place must be the same
> of sources for g-i to work.
> Anyway, will move the headers to libgnome-desktop/.
> 
> (As a side note, maybe it's time to drop libgnome/ and libgnomeui/ and just
> install in $(includedir)/gnome-desktop-3/)

Yes, we'll want to do that. We just need to do it just after a release (like after 2.91.2) so we give time for people to fix it.

Review of attachment 173985 [details] [review]:

Thanks, this mostly looks good, feel free to commit after the next release with the following changes. Could you send a mail to d-d-l about the includedir change once this is done?

::: libgnome-desktop/Makefile.am
@@ +49,3 @@
+        gnome-rr-config.h               \
+        gnome-rr-labeler.h
+

As you suggested, let's put them all in the same directory. That would be $(includedir)/gnome-desktop-3.0/libgnome-desktop.

::: libgnome-desktop/libgnomeui/gnome-bg.h
@@ +33,3 @@
 #include <gconf/gconf-client.h>
+#include "gnome-desktop-thumbnail.h"
+#include "gnome-bg-crossfade.h"

You need to use <> since it's a header that is going to be used by external apps.

Which makes me believe we might want to put those files in /usr/include/gnome-desktop-3.0/libgnome-desktop/ so that we can use #include <libgnome-desktop/gnome-bg.h>

::: libgnome-desktop/gnome-desktop-utils.c
@@ +32,2 @@
 #define GNOME_DESKTOP_USE_UNSTABLE_API
+#include <gnome-desktop-utils.h>

Use "" instead of <>, to be consistent with the change in other .c files (unless there's a good reason for this that I missed)

::: libgnome-desktop/libgnomeui/gnome-rr-config.h
@@ +31,2 @@
 #include <glib.h>
+#include "gnome-rr.h"

Same comment as for gnome-bg.h

::: libgnome-desktop/libgnomeui/gnome-rr-labeler.h
@@ +31,3 @@
 #endif
 
+#include "gnome-rr-config.h"

Same comment as for gnome-bg.h

(In reply to comment #17)
> Review of attachment 173985 [details] [review]:
> 
> Thanks, this mostly looks good, feel free to commit after the next release with
> the following changes. Could you send a mail to d-d-l about the includedir
> change once this is done?
> 
> ::: libgnome-desktop/Makefile.am
> @@ +49,3 @@
> +        gnome-rr-config.h               \
> +        gnome-rr-labeler.h
> +
> 
> As you suggested, let's put them all in the same directory. That would be
> $(includedir)/gnome-desktop-3.0/libgnome-desktop.
> 
> ::: libgnome-desktop/libgnomeui/gnome-bg.h
> @@ +33,3 @@
>  #include <gconf/gconf-client.h>
> +#include "gnome-desktop-thumbnail.h"
> +#include "gnome-bg-crossfade.h"
> 
> You need to use <> since it's a header that is going to be used by external
> apps.
> 
> Which makes me believe we might want to put those files in
> /usr/include/gnome-desktop-3.0/libgnome-desktop/ so that we can use #include
> <libgnome-desktop/gnome-bg.h>

The problem is that libgnome-desktop source files want to include gnome-bg.h, and they don't have a libgnome-desktop directory to search in.
We can either:
- reproduce the libgnome-desktop directory layout (and use non-recursive automake)
- install everything in /usr/include/gnome-desktop-3.0 and gradually move to including only a single <gnome-desktop.h>
- use "" in public headers (so that dependent headers are always looked for in the same folder as the including one)

(In reply to comment #18)
> The problem is that libgnome-desktop source files want to include gnome-bg.h,
> and they don't have a libgnome-desktop directory to search in.

They do have. Look at the directory the sources are in :-)

(In reply to comment #19)
> (In reply to comment #18)
> > The problem is that libgnome-desktop source files want to include gnome-bg.h,
> > and they don't have a libgnome-desktop directory to search in.
> 
> They do have. Look at the directory the sources are in :-)

Oh. Right. Patch coming :D

Created attachment 174074 [details] [review]
Move include files to libgnome-desktop/

gobject-introspection requires all header and source files to be
defined in one place. This is one way to achieve that, without
using non recursive automake.
Also, remove the outdated references to libgnome and libgnomeui

As agreed, I will commit as soon as 2.91.2 is released, and I will
send an email to desktop-devel-list with the changes.

Created attachment 174076 [details] [review]
Turn GnomeRRScreen into a GObject

Rework GnomeRRScreen so that it is a full GObject, and all its data
is moved to GnomeRRScreenPrivate.
GObject's are more supported when it comes to introspection and bindings,
in particular wrt constructors. Therefore, this is a necessary step
for useful introspection generation.

Created attachment 174077 [details] [review]
Add support for GObjectIntrospection

Turned all GnomeRR structures into boxed types, then added the needed
annotations and the Makefile.am bits.
Does not yet include API changes, but should bind (awfully) all of
libgnome-desktop.

Created attachment 174078 [details] [review]
Turn GnomeRRConfig and GnomeOutputInfo into GObjects

For easier binding and introspectability, rework GnomeRRConfig and
GnomeOutputInfo to be GObjects (GInitables, actually) instead of
boxed types. This commit *does* break API, as previous API just
accessed fields in the public structs, while now everything has been
moved to private structures and accessors must be used.
Also, rework GnomeRRLabeler to use both a public and a private structure,
so that gobject-introspection can find instance/class sizes.
Modifications to gnome-control-center and gnome-settings-daemon will
follow.

Has some of the gnome-desktop maintainers started to review this?
Not a full thorough review, just to know if there is interest in cleaning up GnomeRR (even though the gnome-shell display status has been rejected).
I'm asking this because continous advances in gnome-control-center and gnome-settings-daemon are bitrotting the related patches, so if there is interest in this, it should be pushed soon.

See bug #631995 about supporting a status icon for monitors in gnome-shell.

Bug #634332 is another one about gnome-shell.

I know about bug 634332 (I am the reporter...), in fact it was decided that this feature, if it should exist at all, should be an extension.
But besides that, I am asking: is it worth improving libgnome-desktop the way I proposed?
Basically, is clean code for the sake of clean code useful?

To answer your last question about clean code - yes, clean code is useful, but GObject-kosher code is hard to read ;)  I'd like to see the corresponding patches to gnome-settings-daemon and to the capplet.  See my comments below about possibly not hiding all the struct fields, but nevertheless adding accessors for the benefit of bindings.

Can you please push a branch with the modified patches?  We can merge master into that branch periodically to keep it fresh, until we figure out what to do for gnome-shell et al.

Here is a detailed review for each commit (I see that you reverted them).

"Turn GnomeRRScreen into a GObject"

+ fill_out_screen_info() - g_return_val_if_fail() is wrong here; keep the g_assert()s.

+ lots of foo->blah->priv->baz - pull the priv into a variable and use that:
  some_func (screen)
  {
    Priv *priv = screen->priv;

    priv->somefield = 42;
  }

+ The version checks are iffy.  Look for rr_major_version and rr_minor_version.  In all cases, we want to say "too old" if major < 1 or (major == 1 && minor < 2)".  Be careful for the checks that *do* need 1.3 or later, as when calling XRRGetScreenResourcesCurrent().

+ gnome_rr_screen_finalize() - needs to chain up to the parent class handler

+ no get_property() in GnomeRRScreen?  It would be nice to fetch the GdkScreen back.

+ gnome_rr_screen_new() - Don't pass the callback anymore. Have callers connect to the signal instead.

+ gnome_rr_screen_destroy() - Remove it?  Do we need immediate disposal?  If so, add a ::dispose() and shut down things there, instead of in ::finalize().

+ gnome_rr_screen_set_primary_output() - Fix the version check so major > 1 will work.

+ GnomeRRScreenClass - no slot for the "changed" vmethod?  I don't expect anyone to subclass this, but anyway...

"Add support for GObjectIntrospection"

+ output_copy() - this is not a deep-copy (it just copies the pointers to crtcs, but doesn't copy the crtcs themselves).  Is that intended?

+ crtc_copy() - Same as above.

"Turn GnomeRRConfig and GnomeOutputInfo into GObjects"
	+ I don't like using a "current" property to say whether you want to init a GnomeRRConfig from a GnomeRRScreen or from a file.  I'd rather have a gnome_rr_config_new() that gives you an empty configuration (essentially a wrapper for g_object_new()).  And then, to have gnome_rr_config_set_from_screen (config, rr_screen) and a gboolean gnome_rr_config_set_from_file(config, filename, gerror), which would be just like the old gnome_rr_config_new_stored() but with a filename.

+ I don't really like hiding all the fields in outputs/crtcs/modes.
The code in gnome-settings-daemon is going to get a lot uglier.  If
bindings really need accesors, can we have accessors *and* public
struct fields?

I pushed rebased patches to "gobject-gnomerr" branch.
I'll improve them with your comments soon.

(In reply to comment #29)
> To answer your last question about clean code - yes, clean code is useful, but
> GObject-kosher code is hard to read ;)  I'd like to see the corresponding
> patches to gnome-settings-daemon and to the capplet.  See my comments below
> about possibly not hiding all the struct fields, but nevertheless adding
> accessors for the benefit of bindings.
> 
> Can you please push a branch with the modified patches?  We can merge master
> into that branch periodically to keep it fresh, until we figure out what to do
> for gnome-shell et al.
> 
> Here is a detailed review for each commit (I see that you reverted them).
> 
> "Turn GnomeRRScreen into a GObject"

All of this incorporated. Thanks!

> "Add support for GObjectIntrospection"
> 
> + output_copy() - this is not a deep-copy (it just copies the pointers to
> crtcs, but doesn't copy the crtcs themselves).  Is that intended?
> 
> + crtc_copy() - Same as above.

Yes, because {output,crtc}_free don't free the depending object (also, it would mess with recursion because they reference each other freely).
 
> "Turn GnomeRRConfig and GnomeOutputInfo into GObjects"
>     + I don't like using a "current" property to say whether you want to init a
> GnomeRRConfig from a GnomeRRScreen or from a file.  I'd rather have a
> gnome_rr_config_new() that gives you an empty configuration (essentially a
> wrapper for g_object_new()).  And then, to have gnome_rr_config_set_from_screen
> (config, rr_screen) and a gboolean gnome_rr_config_set_from_file(config,
> filename, gerror), which would be just like the old
> gnome_rr_config_new_stored() but with a filename.

Will do that.
Do you mind if gnome_rr_config_new_current and gnome_rr_config_new_stored are kept?

> + I don't really like hiding all the fields in outputs/crtcs/modes.
> The code in gnome-settings-daemon is going to get a lot uglier.  If
> bindings really need accesors, can we have accessors *and* public
> struct fields?

We could, but:
- I'd like to drop some repetitive code in gnome-control-center and gnome-settings-daemon by adding more methods to GnomeRROutputInfo
- I'd like to add properties on GnomeRROutputInfo / GnomeRRConfig at some point 
- There is no distinction of readable and read/write fields, so the comment on top of GnomeOutputInfo would be back

Actually, code is not that bad, and this is no different than what for example Gtk has been doing for a while.

A month passed.
Any progress on reviewing this?
Or were you expecting anything from me?

Federico: do you want somebody else to review the patches?

(In reply to comment #31)

> Will do that.
> Do you mind if gnome_rr_config_new_current and gnome_rr_config_new_stored are
> kept?

I see that you did that; it is fine.  They are the convenience functions that client code already uses, anyway.

It's kind of weird not to have a gnome_rr_config_new() and still have load_filename() and load_current().  However, I guess only the bindings will use the g_object_new() equivalent plus those functions...

> - I'd like to drop some repetitive code in gnome-control-center and
> gnome-settings-daemon by adding more methods to GnomeRROutputInfo

Interesting.  What are you thinking about?

> - There is no distinction of readable and read/write fields, so the comment on
> top of GnomeOutputInfo would be back

Well, that's the thing.  You query for a bunch of stuff.  Then you modify things that make sense (output positions, etc.) and leave alone those that don't make sense to change (CRTC IDs, etc.).  Then you ask for things to magically be changed to your specifications.

I guess you could be *very* neurotic and properly dissect the read-only vs. read-write fields, but it sounds like overkill :)


I've pushed a few fixes to your branch.  One is to fix the version checks for good, and the others are stylistic changes.

I just quickly checked the gobject-gnomerr branches in gnome-settings-daemon and gnome-control-center; they look good.

One final change before you merge all three branches - can you please rename gnome_rr_output_info_get_connected() to gnome_rr_output_info_is_connected()?  That's the convention for boolean values.

After that, please merge all three branches to their modules :)

Thanks for working on this!

(In reply to comment #34)
> (In reply to comment #31)
> 
> > - I'd like to drop some repetitive code in gnome-control-center and
> > gnome-settings-daemon by adding more methods to GnomeRROutputInfo
> 
> Interesting.  What are you thinking about?

I was thinking about functions to find the primary output, to find the laptop, to make clone modes. Anyway, it is another feature, so if and when it will done, it will be in a different bug.
> 
> > - There is no distinction of readable and read/write fields, so the comment on
> > top of GnomeOutputInfo would be back
> 
> Well, that's the thing.  You query for a bunch of stuff.  Then you modify
> things that make sense (output positions, etc.) and leave alone those that
> don't make sense to change (CRTC IDs, etc.).  Then you ask for things to
> magically be changed to your specifications.
> 
> I guess you could be *very* neurotic and properly dissect the read-only vs.
> read-write fields, but it sounds like overkill :)

I tried to do exactly that, by adding setters only for some fields, and I looks that they're enough (for gnome-control-center and gnome-settings-daemon, at least)

> 
> I've pushed a few fixes to your branch.  One is to fix the version checks for
> good, and the others are stylistic changes.
> 
> I just quickly checked the gobject-gnomerr branches in gnome-settings-daemon
> and gnome-control-center; they look good.
> 
> One final change before you merge all three branches - can you please rename
> gnome_rr_output_info_get_connected() to gnome_rr_output_info_is_connected()? 
> That's the convention for boolean values.
> 
> After that, please merge all three branches to their modules :)
> 
> Thanks for working on this!

Merged, fixed, tested, pushed!

This breaks the gnome-screensaver build - see bug #630913. Could anyone review/confirm the trivial patch posted there?

(In reply to comment #36)
> bug #630913.

Wrong bug number?

--> bug 638867

Sorry, I meant bug #638867 in comment 36.