[PATCH Weston 1/1] desktop-shell: implement autolaunch

[Pekka Paalanen]

On Thu, 23 Oct 2014 15:07:01 -0400
Frederic Plourde <frederic.plourde at collabora.co.uk> wrote:

> +1 for refactoring in a separate patch.
> by merging the change with panel_add_launcher in the same patch, it gets *really* hard to read because of the intertwining.

Yeah, please split it.

Btw. it was Fred who actually sent the patch, but made a slight mistake
in trying to preserve my authorship in the process, so it ended
up looking like I sent the patch. :-P

I did write the original feature[1], but Fred has been cleaning it up
since. Credit where credit is due. :-)

Fred, since you're going to split the patch, you should take the
authorship of the parse_launcher_path patch, since I never did that.
Refactor first, new feature second - less code to review.

> now, for exit(<int>) instead of using EXIT_FAILURE/SUCCESS, currently there is roughly 50 usages of exit(<int>) out of 100 total calls to exit() in Weston codebase.
> So we can't really say one if more trendy than the other.
> I'd suggest we leave that as is for now in this patch and open up another bug to change all of them separately someday.

Do make a difference between compositor vs. other programs' exit()
usage. There are only a few exit() calls in the compositor, and note
that the test suite also uses the Weston exit codes to relay test
success/skip/failure IIRC.

Fred has been writing a way to return an exit code from Weston also
with a normal shutdown, so that will interact here. I think a graceful
shutdown would be "better" than calling exit(), but that needs to be
verified. OTOH, calling exit() should not have any adverse effects like
leaving a VT unusable, but somehow I'm not quite convinced over all
possible cases (e.g. running weston directly as root for debugging on
DRM - or what about restoring KMS state?).

Oh, but the exit() call in this particular case is not Weston but the
forked process. So nevermind yet.

> On 14-10-22 11:57 AM, Derek Foreman wrote:
> I'd prefer to see the refactor and the new feature in separate patches,
> but this is pretty trivial.
> 
> I also have a slight preference for exit(EXIT_FAILURE), which is already
> used somewhere else in that file - though there's also precedent for
> exit(1), so you make the call.  :)
> 
> I'd not seen printf's %m until today - do we want to depend on a gnuism?
>  I've seen at least some activity towards a freebsd port - I don't
> believe %m is supported there?

Yeah, we already use that a lot, and I think we can continue adding
those, until someone comes and complains it doesn't work on
Android/BSD/Windows/whatever/...

Can do a mass change then.

> That said, it runs nicely here and does what it says on the tin...
> 
> Reviewed-by: Derek Foreman <derekf at osg.samsung.com>
> 
> On 22/10/14 08:53 AM, Pekka Paalanen wrote:
> Process a new section 'autolaunch' from weston.ini, launching all
> programs given there on desktop start-up.
> 
> [Frederic Plourde: cut redundancy between do_autolaunch and panel_add_launcher]
> ---
>  clients/desktop-shell.c | 97 ++++++++++++++++++++++++++++++++++++++++++-------
>  man/weston.ini.man      | 17 +++++++++
>  weston.ini.in           |  3 ++
>  3 files changed, 103 insertions(+), 14 deletions(-)


Thanks,
pq


[1] http://cgit.collabora.com/git/user/pq/weston.git/commit/?h=next&id=8a0e419fb33c3ab7d4036f25b0c33482e1059d45