[PATCH Weston 1/1] desktop-shell: implement autolaunch
ppaalanen at gmail.com
Thu Oct 23 23:42:36 PDT 2014
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, 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
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
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(-)
More information about the wayland-devel