XfceSMClient is done-ish

[Brian J. Tarricone-3]

-----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.

http://git.xfce.org/xfce/libxfce4ui/tree/libxfce4ui/xfce-sm-client.h?h=kelnos/new-sm-client

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

iEYEARECAAYFAkrJMW0ACgkQ6XyW6VEeAns+8ACfVBo790D03sU59E5BDRTCxpDs
o3QAoO4dBflfzoda3BwRxOhXsHJxEyyP
=VI2/
-----END PGP SIGNATURE-----
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/10/5 Brian J. Tarricone <brian@...>:
> -----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. 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....

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

- 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.

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

- 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.

- xfce_safe_strcmp -> g_strcmp0

- sm_client_singleton variable is not used right?

- 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...

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

Appart from that it looks good.

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

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

On 10/05/2009 08:03 AM, Nick Schermer wrote:

> 2009/10/5 Brian J. Tarricone <brian@...>:
>> -----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-----
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Mike Massonnet]

2009/10/5 Brian J. Tarricone <brian@...>:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 10/05/2009 08:03 AM, Nick Schermer wrote:
>> 2009/10/5 Brian J. Tarricone <brian@...>:
>>> -----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:
>>
>> - 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.

The typedef guint8 can be used for that purpose and makes it clear it
is a numeric value.

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

Hehe, I usually look for functions starting with g_ascii_, in that
case g_ascii_strcasecmp and its case sensitive version
g_ascii_strncasecmp.

Cheers
--
Mike
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

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

On 10/05/2009 10:08 PM, Mike Massonnet wrote:
>
> The typedef guint8 can be used for that purpose and makes it clear it
> is a numeric value.

Ah yeah, not a bad idea.

>>> - xfce_safe_strcmp -> g_strcmp0
>>
>> Gah, I *knew* there was a 'safe' function for that, but couldn't
>> remember the name.
>
> Hehe, I usually look for functions starting with g_ascii_, in that
> case g_ascii_strcasecmp and its case sensitive version
> g_ascii_strncasecmp.

Er, might wanna look at those API docs again ^_~

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

iEYEARECAAYFAkrK1MUACgkQ6XyW6VEeAnsMKQCfYBDnXftTtgg1XZIHHjGUGoRI
+v0AoLMcMGNTwAOLnhqL5TF1Qxjnh2/u
=7sYB
-----END PGP SIGNATURE-----
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Ali Abdallah-2]

Brian J. Tarricone wrote:
>
> 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.
>  
Network Manager exposes two methods, sleep and wake, those are to be
used by power manager or session manager (or any application that does
suspend/hibernate) before performing suspend/hibernate operations, then
as said, network manager takes the rest of the work to emit the
necessary signals for indication state_change, connection_change, etc... .

Cheers,
Ali.
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/10/5 Brian J. Tarricone <brian@...>:

> On 10/05/2009 08:03 AM, Nick Schermer wrote:
>> 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).

A well, I knew this was gone happen, so I won't spend any words on it.

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

A couple of defaults are nice, they also give an idea what values are
used. You could even
use (XFCE_SM_CLIENT_PRIORITY_WM - 1) to start before the window
manager, which is nice.
Only the typedef is confusing since is not used in any of the api like
you already said.

>> - sm_client_singleton variable is not used right?
>
> No, it's used in the constructor.

Yeah, but it is always null...

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

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

On 10/06/2009 01:01 AM, Nick Schermer wrote:
> 2009/10/5 Brian J. Tarricone <brian@...>:
>> On 10/05/2009 08:03 AM, Nick Schermer wrote:
>
>>> - sm_client_singleton variable is not used right?
>>
>> No, it's used in the constructor.
>
> Yeah, but it is always null...

Heh, whoops.  All better now.

        -brian

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

iEYEARECAAYFAkrLgfUACgkQ6XyW6VEeAnt2+gCfeMSsYndbBGCcUnfkAPTfqMir
rg0AniTwC3AUSdPc/KOOLJsGUEe8uMtl
=it7N
-----END PGP SIGNATURE-----
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

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

On 10/06/2009 02:21 AM, Ali Abdallah wrote:

> Brian J. Tarricone wrote:
>>
>> 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.
>>  
> Network Manager exposes two methods, sleep and wake, those are to be
> used by power manager or session manager (or any application that does
> suspend/hibernate) before performing suspend/hibernate operations, then
> as said, network manager takes the rest of the work to emit the
> necessary signals for indication state_change, connection_change, etc... .

Yup.  Thing is, though, a regular app probably won't even care about the
sleep/wake signals.  Watching state_change or connection_change should
be enough, because the app probably just wants to know when the network
is connected or disconnected, which can happen whether or not the
machine goes to sleep.

        -brian

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

iEYEARECAAYFAkrLgm0ACgkQ6XyW6VEeAnuY/ACg4YrPENw1e4OBCQS/N5iUiVY2
ozAAoKUXNpL3Rf6RRlMg/NOAGzGlwitx
=aakk
-----END PGP SIGNATURE-----
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Ali Abdallah-2]

Brian J. Tarricone wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 10/06/2009 02:21 AM, Ali Abdallah wrote:
>  
>> Brian J. Tarricone wrote:
>>    
>>> 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.
>>>  
>>>      
>> Network Manager exposes two methods, sleep and wake, those are to be
>> used by power manager or session manager (or any application that does
>> suspend/hibernate) before performing suspend/hibernate operations, then
>> as said, network manager takes the rest of the work to emit the
>> necessary signals for indication state_change, connection_change, etc... .
>>    
>
> Yup.  Thing is, though, a regular app probably won't even care about the
> sleep/wake signals.  Watching state_change or connection_change should
> be enough, because the app probably just wants to know when the network
> is connected or disconnected, which can happen whether or not the
> machine goes to sleep.
>
>  

Exactly.

Thanks for making XfceSMClient public so other projects (goodies) can
make use of it and drop libxfcegui4 dependency.

> -brian
>  
Cheers,
Ali.

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.11 (GNU/Linux)
>
> iEYEARECAAYFAkrLgm0ACgkQ6XyW6VEeAnuY/ACg4YrPENw1e4OBCQS/N5iUiVY2
> ozAAoKUXNpL3Rf6RRlMg/NOAGzGlwitx
> =aakk
> -----END PGP SIGNATURE-----
> _______________________________________________
> Xfce4-dev mailing list
> Xfce4-dev@...
> http://foo-projects.org/mailman/listinfo/xfce4-dev
>  

_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev