[Xfce-bugs] [Bug 5377] libxfce4gui session-client bug fixes, improvements, and some optimizations

[bugzilla-daemon at xfce.org]

http://bugzilla.xfce.org/show_bug.cgi?id=5377





--- Comment #1 from Brian J. Tarricone <bjt23 at cornell.edu>  2009-05-19 19:42:15 UTC ---
(In reply to comment #0)
> Created an attachment (id=2361)
 --> (http://bugzilla.xfce.org/attachment.cgi?id=2361) [details]
> All the changes listed above
> 
> I wanted to add X11 session support to mousepad, but ran into some gui4
> session-client bugs and deficiencies, and had to hack it a bit.

Yay!  Our session client really sucks.  Thanks for looking into this.

> client_session_new_full() sets the restart, clone etc. commands _by directly
> assigning to them_ instead of duplicating the arguments passed. So any call
> to set_whatever_command() or client_session_free() crashes when attempting
> to g_strfreev() the command(s), unless the **values passed to new_full() are
> allocated-and-not-freed by the caller. However, client_session_new() passes
> the same array for both restart and clone, and the array elements are taken
> directly from argv[], with obvious results.

Odd, I thought we'd already accepted a patch that fixes this, but I guess not. 
Must be floating around in bugzilla somewhere...

> When client_session_new() generates the initial restart command, it does not
> strip the (previous) client id, if any, so set_clone_restart_commands() does
> not append the (newly given) client id. Of course, a when previous id
> exists, the new id will normally be the same - unless, for example, previous
> is invalid.

Good...

> If client_session_free() is called before session_init(), or after an
> unsuccessful session_init(), it calls disconnect(), which calls
> SmcCloseConnection() with session_connection = NULL and thus crashes.

Yeah, I recall this got fixed in that same patch, but I guess no one checked it
in.

> There was no way to cancel the shutdown from interact. For compatibility,
> I extended the session client structure with gboolean interact_2.

You can't do this.  Unfortunately, that struct is public.  If an app is
compiled against libxfcegui4 with the smaller structure size, and then
libxfcegui4 gets upgraded to a version with a larger structure size, they'll
crash.

Of course, *most* users of the library probably use _new_full() which will
somewhat get around this problem, but, regardless, adding this struct member
changes ABI.  No good.

> The restart, clone etc. commands are set before invoking the application's
> save_yourself/save_phase_2 callbacks. SM spec says that interact and
> save_phase_2 should SetProperties, and the session client code does than,
> but without giving the application a change to set/alter them. And without
> that, for example, mousepad will have to set_restart_command() on each
> possible filename change (4 calls in callbacks.c and one more in main.c).
> So I moved the set_clone_restart_commands() invocation immediately before
> SmcSaveYourselfDone().

Yeah, that sounds right.

> The --display (if missing) was appended only to the automatically generated
> client_session_new() restart/clone commands, and not to any commands set
> by the application. So I moved appending to set_clone_restart_commands().

I don't think this is correct.  If the caller sets a clone/restart command, the
library should send *exactly* what was passed to it.  If you want a --display
parameter in your clone/restart command, and you're setting one manually, you
should add --display.  The session client can't assume that the app in question
even supports the --display option.

(Of course, one could also make this argument about the --sm-client-id
argument; but that's just a poor design decision that we're now stuck with.)

> client_session_new() initializes both clone and restart commands, so if the
> application sets a new restart command, it can not rely on the automatic
> restart -> clone copy. Altered client_session_new() to pass NULL, so the
> latest restart command is copied to clone.

That sounds reasonable, as long as the code will always use the restart command
(minus --sm-client-id) as the clone command if clone is NULL.

> Added the missing client_session_set_interact_style() function. Currently,
> all SessionClient fields are accessed via functions, except for
> interact_style which must be directly assigned to.

Not a big deal; most session managers seem to ignore the interact style.

> Optimizations:
> 
> The largely identical code for setting the different commands in
> set_clone_restart_commands() is rebased a single function:
> 
> set_property_from_command(client, command [x11 command name], ptr [command
> value], add_sm_id, add_display)
> 
> The identical set_whatever_command()-s are now one-liners invoking the new
> copy_command() function.

Sounds good.

> Notes:
> 
> Using g_strdupv() instead of safe_strvdup() seems to work just fine. Not
> knowing why safe_strvdup() stands for, I left it as is.
> 
> IMHO, SessionClient should only include a single session id variable, namely
> client_id, which should be equal to the previous id before init and to the
> new [given] id after; given_client_id should be a local variable of
> session_init(). But I didn't want to break compatibility.

Yeah, that seemed to be a questionable design decision from the start; not sure
why you'd ever care about the original session id if the session manager gives
you something different... and if you do, you can always keep a copy on your
own and compare later.  But yeah, this needs to stay.

> I tried to keep the code simple, since the session management is quite
> unpleasant by itself (to avoid a harsher expression).

Oh yeah, definitely...

> Last but not least, I compiled libxfcegui4.so and replaced my original
> shared library. No crashes, and the new library removed --sm-client-id from
> the clone commands of xfdesktop and xfwm.

Sounds good.

Honestly, our session client is pretty awful.  The inner implementation is ok
(and your patch improves that greatly), but we should really have a new proper
GObject-ified XfceSessionClient with a cleaner API, signals instead of
callbacks, and of course an opaque struct.  But who knows when anyone will have
time to do that... so incremental improvements to the current session client
will have to do ^_^.

-- 
Configure bugmail: http://bugzilla.xfce.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.