Re: [PATCH weston 02/11] compositor: add API to manage compositor instances

2015-07-08 Thread Pekka Paalanen
On Tue, 23 Jun 2015 18:19:38 -0700
Jon A. Cruz j...@osg.samsung.com wrote:

 
 Oh I forgot an item on the placement of comments.
 
 
 On 06/23/2015 04:31 PM, Jon A. Cruz wrote:
  
  Minor doxygen note: the explicit \brief is not needed, and the brief
  description should end with a '.' to allow the auto-brief to pick it up.
  
  Overall looks good, but missing a few extra management calls. However
  those seem applicable to patch 4/11.
  
  
  On 06/22/2015 01:02 PM, Giulio Camuffo wrote:
  This commit adds three new exported functions:
  - weston_compositor_create() returns a new weston_compositor instance,
  initializing it as the now removed weston_compositor_init() did.
  - weston_compositor_exit(compositor) asks the compositor to tear
  down by calling the compositor's exit vfunc which is set by the
  libweston application.
  - weston_compositor_destroy(compositor) is called by the libweston
  application when tearing down the compositor. The compositor is destroyed
  and the memory freed.
  ---
  
  Reviewed-by: Jon A. Cruz j...@osg.samsung.com
  
   src/compositor-drm.c  |   8 +--
   src/compositor-fbdev.c|   6 --
   src/compositor-headless.c |   9 +--
   src/compositor-rdp.c  |   4 --
   src/compositor-wayland.c  |   9 +--
   src/compositor-x11.c  |   5 +-
   src/compositor.c  | 167 
  +-
   src/compositor.h  |  14 +++-
   8 files changed, 137 insertions(+), 85 deletions(-)

  diff --git a/src/compositor.c b/src/compositor.c
  index 4e91329..1924e6a 100644
  --- a/src/compositor.c
  +++ b/src/compositor.c
  @@ -4500,16 +4500,27 @@ timeline_key_binding_handler(struct weston_seat 
  *seat, uint32_t time,
 weston_timeline_open(compositor);
   }
   
  -WL_EXPORT int
  -weston_compositor_init(struct weston_compositor *ec,
  - int *argc, char *argv[],
  - struct weston_config *config)
 
 Aside from some details on the comments themselves (\brief, \ref...) it
 is more generally expected to see the doxygen comments in the .h file
 where the function is declared, instead of the .c file where it is
 implemented.

Seriously?

Personally I prefer documentation to reside with the implementation, so
that it is less easy to forget to update the docs. The compiler will
catch forgotten declaration, but not docs.

Maybe I picked it up from the Linux coding style.

Yes, I did merge your patch which does it differently than the whole rest
of existing Weston and Wayland. That was an oversight.


Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH weston 02/11] compositor: add API to manage compositor instances

2015-06-23 Thread Jon A. Cruz

Oh I forgot an item on the placement of comments.


On 06/23/2015 04:31 PM, Jon A. Cruz wrote:
 
 Minor doxygen note: the explicit \brief is not needed, and the brief
 description should end with a '.' to allow the auto-brief to pick it up.
 
 Overall looks good, but missing a few extra management calls. However
 those seem applicable to patch 4/11.
 
 
 On 06/22/2015 01:02 PM, Giulio Camuffo wrote:
 This commit adds three new exported functions:
 - weston_compositor_create() returns a new weston_compositor instance,
 initializing it as the now removed weston_compositor_init() did.
 - weston_compositor_exit(compositor) asks the compositor to tear
 down by calling the compositor's exit vfunc which is set by the
 libweston application.
 - weston_compositor_destroy(compositor) is called by the libweston
 application when tearing down the compositor. The compositor is destroyed
 and the memory freed.
 ---
 
 Reviewed-by: Jon A. Cruz j...@osg.samsung.com
 
  src/compositor-drm.c  |   8 +--
  src/compositor-fbdev.c|   6 --
  src/compositor-headless.c |   9 +--
  src/compositor-rdp.c  |   4 --
  src/compositor-wayland.c  |   9 +--
  src/compositor-x11.c  |   5 +-
  src/compositor.c  | 167 
 +-
  src/compositor.h  |  14 +++-
  8 files changed, 137 insertions(+), 85 deletions(-)

 diff --git a/src/compositor-drm.c b/src/compositor-drm.c
 index 6d24f05..4302f40 100644
 --- a/src/compositor-drm.c
 +++ b/src/compositor-drm.c
 @@ -2424,7 +2424,7 @@ update_outputs(struct drm_backend *b, struct 
 udev_device *drm_device)
  
  /* FIXME: handle zero outputs, without terminating */
  if (b-connector_allocator == 0)
 -wl_display_terminate(b-compositor-wl_display);
 +weston_compositor_exit(b-compositor);
  }
  
  static int
 @@ -2845,12 +2845,6 @@ drm_backend_create(struct weston_compositor 
 *compositor,
  
  b-use_pixman = param-use_pixman;
  
 -if (weston_compositor_init(compositor, argc, argv,
 -   config)  0) {
 -weston_log(%s failed\n, __func__);
 -goto err_base;
 -}
 -
  /* Check if we run drm-backend using weston-launch */
  compositor-launcher = weston_launcher_connect(compositor, param-tty,
 param-seat_id, true);
 diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
 index 6a640ee..7117bc5 100644
 --- a/src/compositor-fbdev.c
 +++ b/src/compositor-fbdev.c
 @@ -826,10 +826,6 @@ fbdev_backend_create(struct weston_compositor 
 *compositor, int *argc, char *argv
  return NULL;
  
  backend-compositor = compositor;
 -if (weston_compositor_init(compositor, argc, argv,
 -   config)  0)
 -goto out_free;
 -
  if (weston_compositor_set_presentation_clock_software(
  compositor)  0)
  goto out_compositor;
 @@ -902,8 +898,6 @@ out_udev:
  
  out_compositor:
  weston_compositor_shutdown(compositor);
 -
 -out_free:
  free(backend);
  
  return NULL;
 diff --git a/src/compositor-headless.c b/src/compositor-headless.c
 index 8e40152..b1be3a0 100644
 --- a/src/compositor-headless.c
 +++ b/src/compositor-headless.c
 @@ -218,8 +218,7 @@ headless_destroy(struct weston_compositor *ec)
  static struct headless_backend *
  headless_backend_create(struct weston_compositor *compositor,
  struct headless_parameters *param,
 -const char *display_name, int *argc, char *argv[],
 -struct weston_config *config)
 +const char *display_name)
  {
  struct headless_backend *b;
  
 @@ -228,9 +227,6 @@ headless_backend_create(struct weston_compositor 
 *compositor,
  return NULL;
  
  b-compositor = compositor;
 -if (weston_compositor_init(compositor, argc, argv, config)  0)
 -goto err_free;
 -
  if (weston_compositor_set_presentation_clock_software(compositor)  0)
  goto err_free;
  
 @@ -287,8 +283,7 @@ backend_init(struct weston_compositor *compositor,
  if (weston_parse_transform(transform, param.transform)  0)
  weston_log(Invalid transform \%s\\n, transform);
  
 -b = headless_backend_create(compositor, param, display_name,
 -argc, argv, config);
 +b = headless_backend_create(compositor, param, display_name);
  if (b == NULL)
  return -1;
  return 0;
 diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c
 index 96d4b5f..86c5b2a 100644
 --- a/src/compositor-rdp.c
 +++ b/src/compositor-rdp.c
 @@ -1172,9 +1172,6 @@ rdp_backend_create(struct weston_compositor 
 *compositor,
  return NULL;
  
  b-compositor = compositor;
 -if (weston_compositor_init(compositor, argc, argv, wconfig)  0)
 -goto err_free;
 -
  b-base.destroy = 

Re: [PATCH weston 02/11] compositor: add API to manage compositor instances

2015-06-23 Thread Jon A. Cruz

Minor doxygen note: the explicit \brief is not needed, and the brief
description should end with a '.' to allow the auto-brief to pick it up.

Overall looks good, but missing a few extra management calls. However
those seem applicable to patch 4/11.


On 06/22/2015 01:02 PM, Giulio Camuffo wrote:
 This commit adds three new exported functions:
 - weston_compositor_create() returns a new weston_compositor instance,
 initializing it as the now removed weston_compositor_init() did.
 - weston_compositor_exit(compositor) asks the compositor to tear
 down by calling the compositor's exit vfunc which is set by the
 libweston application.
 - weston_compositor_destroy(compositor) is called by the libweston
 application when tearing down the compositor. The compositor is destroyed
 and the memory freed.
 ---

Reviewed-by: Jon A. Cruz j...@osg.samsung.com

  src/compositor-drm.c  |   8 +--
  src/compositor-fbdev.c|   6 --
  src/compositor-headless.c |   9 +--
  src/compositor-rdp.c  |   4 --
  src/compositor-wayland.c  |   9 +--
  src/compositor-x11.c  |   5 +-
  src/compositor.c  | 167 
 +-
  src/compositor.h  |  14 +++-
  8 files changed, 137 insertions(+), 85 deletions(-)
 
 diff --git a/src/compositor-drm.c b/src/compositor-drm.c
 index 6d24f05..4302f40 100644
 --- a/src/compositor-drm.c
 +++ b/src/compositor-drm.c
 @@ -2424,7 +2424,7 @@ update_outputs(struct drm_backend *b, struct 
 udev_device *drm_device)
  
   /* FIXME: handle zero outputs, without terminating */
   if (b-connector_allocator == 0)
 - wl_display_terminate(b-compositor-wl_display);
 + weston_compositor_exit(b-compositor);
  }
  
  static int
 @@ -2845,12 +2845,6 @@ drm_backend_create(struct weston_compositor 
 *compositor,
  
   b-use_pixman = param-use_pixman;
  
 - if (weston_compositor_init(compositor, argc, argv,
 -config)  0) {
 - weston_log(%s failed\n, __func__);
 - goto err_base;
 - }
 -
   /* Check if we run drm-backend using weston-launch */
   compositor-launcher = weston_launcher_connect(compositor, param-tty,
  param-seat_id, true);
 diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
 index 6a640ee..7117bc5 100644
 --- a/src/compositor-fbdev.c
 +++ b/src/compositor-fbdev.c
 @@ -826,10 +826,6 @@ fbdev_backend_create(struct weston_compositor 
 *compositor, int *argc, char *argv
   return NULL;
  
   backend-compositor = compositor;
 - if (weston_compositor_init(compositor, argc, argv,
 -config)  0)
 - goto out_free;
 -
   if (weston_compositor_set_presentation_clock_software(
   compositor)  0)
   goto out_compositor;
 @@ -902,8 +898,6 @@ out_udev:
  
  out_compositor:
   weston_compositor_shutdown(compositor);
 -
 -out_free:
   free(backend);
  
   return NULL;
 diff --git a/src/compositor-headless.c b/src/compositor-headless.c
 index 8e40152..b1be3a0 100644
 --- a/src/compositor-headless.c
 +++ b/src/compositor-headless.c
 @@ -218,8 +218,7 @@ headless_destroy(struct weston_compositor *ec)
  static struct headless_backend *
  headless_backend_create(struct weston_compositor *compositor,
   struct headless_parameters *param,
 - const char *display_name, int *argc, char *argv[],
 - struct weston_config *config)
 + const char *display_name)
  {
   struct headless_backend *b;
  
 @@ -228,9 +227,6 @@ headless_backend_create(struct weston_compositor 
 *compositor,
   return NULL;
  
   b-compositor = compositor;
 - if (weston_compositor_init(compositor, argc, argv, config)  0)
 - goto err_free;
 -
   if (weston_compositor_set_presentation_clock_software(compositor)  0)
   goto err_free;
  
 @@ -287,8 +283,7 @@ backend_init(struct weston_compositor *compositor,
   if (weston_parse_transform(transform, param.transform)  0)
   weston_log(Invalid transform \%s\\n, transform);
  
 - b = headless_backend_create(compositor, param, display_name,
 - argc, argv, config);
 + b = headless_backend_create(compositor, param, display_name);
   if (b == NULL)
   return -1;
   return 0;
 diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c
 index 96d4b5f..86c5b2a 100644
 --- a/src/compositor-rdp.c
 +++ b/src/compositor-rdp.c
 @@ -1172,9 +1172,6 @@ rdp_backend_create(struct weston_compositor *compositor,
   return NULL;
  
   b-compositor = compositor;
 - if (weston_compositor_init(compositor, argc, argv, wconfig)  0)
 - goto err_free;
 -
   b-base.destroy = rdp_destroy;
   b-base.restore = rdp_restore;
   b-rdp_key =