XfceSMClient is done-ish

[Brian J. Tarricone]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/05/2009 08:03 AM, Nick Schermer wrote:
> 2009/10/5 Brian J. Tarricone <brian at tarricone.org>:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Hi guys,
>>
>> I think I'm pretty happy with the XfceSMClient API, and I'm more or less
>> ready to merge it back to master.  Please let me know if you have
>> comments or suggestions.
> 
> Some random comments:
> 
> - I see you want to include the code in the main library not the
> private library.

Yeah, my general opinion about this is I want to allow non-Xfce-core
apps to use the SM client, and sticking it in a private library would
prevent that (I know some outside people have asked about it, and have
tried to use/improve our existing SM client in libgui with
unsatisfactory results).  I think XfceSMClient has a decent API and is a
pretty good implementation in its own right, so I'm pretty happy
including it as a public API.

> This is fine by me, but then I'd prefer you use the
> same coding style are the other files. I know we don't have a hard
> policy here, but use 1 style in each module is the least we can do....

I've always been of the opinion that per-file style is fine for "shared"
modules.  I know you did the vast majority of the work of putting
libxfce4ui together (though much of the code was written by others), but
I don't see how we can view it any differently than libxfce4util or
libxfcegui4 in the ownership sense.

Frankly, I find the coding style used in the rest of libxfce4ui as
excessively verbose and annoying, and even sometimes difficult to read
(both the two-space indents and weird brace indentation bother me).  I
know coding style is a touchy subject for many people, so please don't
think I'm saying "you're an idiot for coding like that."  It's just that
I personally don't like it, just like you probably don't like mine.
Xfce4-session has very similar style, and honestly I spend far too much
time figuring coding style issues with it that I'd rather spend actually
coding.  (I've considered converting it to my style, but I'm not sure I
want that noise in the VCS.)

I intend to maintain XfceSMClient, so I'd prefer to work with it using a
coding style I'm more comfortable with.  Yes, that means other people
making modifications to it will need to be aware of the style there,
but... well, nobody said life was easy.

> - What about support for suspend/hibernate in the restart hints?

Regular apps shouldn't care about suspend or hibernate; it should be
entirely transparent to them.  There are a few cases where it matters,
like a network manager: it'd need to know about suspend because you
might later wake it back up in a totally different network situation.
But this isn't something a regular app should need to know about.

You might make the argument that *any* app that uses the network (or
another physical-location-specific service) would care because they'll
likely need to e.g. refresh network connections on wakeup, but I don't
think this is the place for it.  XfceSMClient is about maintaining
persistent app state and session lifecycle.

If an app cares about network state, then it likely cares about network
state changes that happen regardless of suspend/hibernate, so it should
watch signals on the org.freedesktop.NetworkManager dbus interface.  I'm
sure I could make a similar parallel for other services.

> - A guchar for the priority feels a bit weird. I userstand you do this
> for the SmProp, but a guint is probably nicer for public api.

I thought about this quite a bit, but I'd rather not (I think my
original version used gint, but I later changed it).  The XSMP spec
restricts the priority to values from 0 to 255.  A guint just makes that
confusing -- at least, you'd have to read (and remember) the doc to know
what the allowed values are.  guchar makes that explicit in the API.  As
much as I hate to expose protocol details in an API, I don't think this
imposes a burden on an API user, and it isn't really a problem for the
future.  Even if someday we switch to a protocol that allows a
ridiculous range of priority values, having that range isn't really useful.

I guess another option would be to use gint/guint and then scale the
provided priority value to "fit" in 8 bits (rather than generating an
error or clamping), but I'd like to keep the hand-wavy magic to a
minimum (there's already enough useful magic in other places in
XfceSMClient).

> - XfceSMClientPriority are just defaults that 'make sense' right? use
> #define (like G_PRIORITY_*) in that case or enum without typedef.

I prefer enums over preprocessor defines where possible when there's
more than one value.  Whether or not it's a typedef doesn't really
matter syntactically, but I like having the descriptive name there, even
if you don't find the type used in any API.

It's questionable to really have any of those there aside from
_DEFAULT... nothing outside Xfce should use anything < _DEFAULT anyway,
so I might remove the whole thing and provide just one #define for
_DEFAULT.  Dunno, tho.  I think they're potentially useful, even if just
for us.

> - Maybe only set the icon and app name in
> xfce_sm_client_set_desktop_file() if g_get_application_name or
> gtk_window_get_default_icon_name (gtk 2.16) are NULL? A lot of apps
> set those already, maybe not a good idea to overwrite them.

Yeah, good idea.  In theory apps are setting up the SM client before
they do much of anything else, so it might not matter.
g_set_application_name() can only be called once, so if it's already set
it'll g_warning, so it's good to check it first anyway.

> - xfce_safe_strcmp -> g_strcmp0

Gah, I *knew* there was a 'safe' function for that, but couldn't
remember the name.

> - sm_client_singleton variable is not used right?

No, it's used in the constructor.

> - Gtk/Glib uses G_CONST_RETURN gchar * G_CONST_RETURN * for returning
> a gchar ** that should not be freed, I know it's ugly but maybe we
> should follow that style...

Yeah, maybe.  The problem is that the only declaration for a variable to
receive the return of that function without a warning is

const gchar * const *foo;

... which is kinda ugly.  If you're just going to iterate over the items
in the returned array, that's fine, but if you want to use g_strdupv()
or something, then you have to cast it (since that takes gchar**).

I dunno, need to think about this a bit more.

> - Maybe make it a real object by making the structs public and use a
> private for the internals?

It is a real object.  The only effect of not making the structs public
is that it's a "final" class (you can't subclass it).  I'd rather keep
it that way.  Can you think of a reason why someone might want to
subclass XfceSMClient?  And if so, are there any member functions that
should be made vfuncs to make subclassing it useful?

	-brian
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.11 (GNU/Linux)

iEYEARECAAYFAkrKWTIACgkQ6XyW6VEeAntSkwCfctoWsAu1Qm/5I6ZhqOjeIjnm
MwAAmwSh6HIJxAidkKtqqGp53Op6BKWY
=njzD
-----END PGP SIGNATURE-----