On Thu, 23 Oct 2014 15:07:01 -0400 Frederic Plourde <frederic.plou...@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 <der...@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 _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
