Hi,
btw. pay attention to patch summaries (the email subject line). We use
a "component: description" format. Some examples:
"ivi application protocol:" ->
protocol: add ivi application extension
"The weston-layout library supports" ->
ivi-shell: add weston-layout library
"ivi-shell supports a type of shell for In-Vehicle Infotainment
system." ->
ivi-shell: add the shell plugin
etc.
Reading the weston git log should give an idea of how to write it.
On Mon, 17 Mar 2014 15:26:38 +0900
Nobuhiko Tanibata <nobuhiko_tanib...@xddp.denso.co.jp> wrote:
> In-Vehicle Infotainment system traditionally manages surfaces with global
> identification. A protocol, ivi_application, supports such a feature by
> implementing a request, ivi_application::surface_creation defined in
> ivi_application.xml. Additionally, it initialize a library, weston-layout,
> to manage properties of surfaces and group surfaces in layer. In detail,
> refer library, weston_layout.
>
> Signed-off-by: Nobuhiko Tanibata <nobuhiko_tanib...@xddp.denso.co.jp>
> ---
>
> Changes for v2:
> - apply review comments of mailing list.
> - squash update of Makefile into this patch.
> - move this patch after patch of weston-layout.
> - support inherit propoerties of id_surface when client attaches another
> wl_surface with id_surface after destroying ivi_surface once.
>
> Changes for v3:
> - squash internal method, configure, to ivi_shell_surface_configure.
>
> Changes for v4:
> - nothing. Version number aligned to the first patch
>
> ivi-shell/Makefile.am | 24 +++-
> ivi-shell/ivi-shell.c | 314
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 337 insertions(+), 1 deletion(-)
> create mode 100644 ivi-shell/ivi-shell.c
>
> diff --git a/ivi-shell/Makefile.am b/ivi-shell/Makefile.am
> index 4d54c2d..d0c0d62 100644
> --- a/ivi-shell/Makefile.am
> +++ b/ivi-shell/Makefile.am
> @@ -1,7 +1,8 @@
> moduledir = $(libdir)/weston
>
> module_LTLIBRARIES = \
> - $(libweston_layout)
> + $(libweston_layout) \
> + $(ivi_shell)
>
> AM_CPPFLAGS = \
> -I$(top_srcdir)/shared \
> @@ -17,6 +18,7 @@ westoninclude_HEADERS =
>
> if ENABLE_IVI_SHELL
> westoninclude_HEADERS += \
> + ivi-application-client-protocol.h \
I'm not sure it makes sense to export the client protocol header,
because client programs do not get a library to link to for getting the
ivi-application-protocol.c part.
> weston-layout.h
>
> libweston_layout = libweston-layout.la
> @@ -27,4 +29,24 @@ libweston_layout_la_SOURCES = \
> weston-layout.c \
> weston-layout.h
>
> +ivi_shell = ivi-shell.la
> +ivi_shell_la_LDFLAGS = -module -avoid-version
> +ivi_shell_la_LIBADD = $(COMPOSITOR_LIBS) $(IVI_SHELL_LIBS)
> ./libweston-layout.la
> +ivi_shell_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS) $(IVI_SHELL_CFLAGS)
> +ivi_shell_la_SOURCES = \
> + ivi-shell.c \
> + weston-layout.h \
This is one more hint, that weston-layout.so should not be a library,
but a part of ivi-shell.so. You link the local build of
weston-layout.so into ivi-shell.so during the build.
Maybe you meant libweston-layout.la to be a convenience library instead?
Not sure that is needed either, nothing else will link to it I believe.
I think ivi-shell.so could export the weston-layout API just like the
weston executable exports the weston core API to plugins.
By all means do keep the code in weston-layout.c in a separate file if
you want, but weston-layout.h would be split into two files: one
private to ivi-shell.so, another the exported API for stuff like
ivi-controller.so. I think a sensible split between weston-layout.c and
ivi-shell.c would be based on ivi-shell.c handling mostly Weston core
callbacks, and weston-layout.c handling the public API.
There is the difference between static, non-static, and WL_EXPORT
symbols.
> + ivi-application-protocol.c \
> + ivi-application-server-protocol.h
> +
> endif
> +
> +BUILT_SOURCES = \
> + ivi-application-protocol.c \
> + ivi-application-server-protocol.h \
> + ivi-application-client-protocol.h
> +
> +CLEANFILES = $(BUILT_SOURCES)
> +
> +wayland_protocoldir = $(top_srcdir)/protocol
> +include $(top_srcdir)/wayland-scanner.mk
> diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
> new file mode 100644
> index 0000000..31d39cc
> --- /dev/null
> +++ b/ivi-shell/ivi-shell.c
> @@ -0,0 +1,314 @@
> +/*
> + * Copyright (C) 2013 DENSO CORPORATION
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> + * its documentation for any purpose is hereby granted without fee, provided
> + * that the above copyright notice appear in all copies and that both that
> + * copyright notice and this permission notice appear in supporting
> + * documentation, and that the name of the copyright holders not be used in
> + * advertising or publicity pertaining to distribution of the software
> + * without specific, written prior permission. The copyright holders make
> + * no representations about the suitability of this software for any
> + * purpose. It is provided "as is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +
> +/**
> + * ivi-shell supports a type of shell for In-Vehicle Infotainment system.
> + * In-Vehicle Infotainment system traditionally manages surfaces with global
> + * identification. A protocol, ivi_application, supports such a feature
> + * by implementing a request, ivi_application::surface_creation defined in
> + * ivi_application.xml.
> + *
> + * Additionally, it initialize a library, weston-layout, to manage
> properties of
> + * surfaces and group surfaces in layer. In detail, refer weston_layout.
> + */
> +
> +#include <sys/wait.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <linux/input.h>
> +
> +#include "compositor.h"
> +#include "ivi-application-server-protocol.h"
> +#include "weston-layout.h"
> +
> +struct ivi_shell;
> +
> +struct ivi_shell_surface
> +{
> + struct ivi_shell *shell;
> + struct weston_layout_surface *layout_surface;
> +
> + struct weston_surface *surface;
> + uint32_t id_surface;
> +
> + int32_t width;
> + int32_t height;
> +
> + struct wl_list link;
> +};
> +
> +struct ivi_shell
> +{
> + struct wl_resource *resource;
> + struct wl_listener destroy_listener;
> +
> + struct weston_compositor *compositor;
> +
> + struct wl_list ivi_surface_list; /* struct ivi_shell_surface::link */
> +};
> +
> +/* -------------------------------------------------------------------------
> */
> +/* common functions
> */
> +/* -------------------------------------------------------------------------
> */
> +
> +/**
> + * Implementation of ivi_surface
> + */
> +
> +static void
> +ivi_shell_surface_configure(struct weston_surface *, int32_t, int32_t);
> +
> +static struct ivi_shell_surface *
> +get_ivi_shell_surface(struct weston_surface *surface)
> +{
> + if (surface->configure == ivi_shell_surface_configure) {
> + return surface->configure_private;
> + } else {
> + return NULL;
> + }
> +}
> +
> +static void
> +ivi_shell_surface_configure(struct weston_surface *es,
Let's call it 'surface' or such in new code, 'es' is just cargo-cult.
> + int32_t sx, int32_t sy)
> +{
> + struct ivi_shell_surface *ivisurf = get_ivi_shell_surface(es);
> + struct weston_view *view = NULL;
> + float from_x = 0.0f;
> + float from_y = 0.0f;
> + float to_x = 0.0f;
> + float to_y = 0.0f;
> +
> + if ((es->width == 0) || (es->height == 0) || (ivisurf == NULL)) {
> + return;
> + }
> +
> + view = weston_layout_get_weston_view(ivisurf->layout_surface);
> + if (view == NULL) {
> + return;
> + }
> +
> + if (ivisurf->width != es->width || ivisurf->height != es->height) {
> +
> + ivisurf->width = es->width;
> + ivisurf->height = es->height;
> +
> + weston_view_to_global_float(view, 0, 0, &from_x, &from_y);
> + weston_view_to_global_float(view, sx, sy, &to_x, &to_y);
> +
> + weston_view_set_position(view,
> + view->geometry.x + to_x - from_x,
> + view->geometry.y + to_y - from_y);
> + weston_view_update_transform(view);
> +
> + weston_layout_surfaceConfigure(ivisurf->layout_surface, es->width,
> es->height);
> + }
> +}
> +
> +static void
> +surface_destroy(struct wl_client *client, struct wl_resource *resource)
> +{
> + struct ivi_shell_surface *ivisurf = wl_resource_get_user_data(resource);
> +
> + if (ivisurf != NULL) {
> + ivisurf->surface->configure = NULL;
> + ivisurf->surface->configure_private = NULL;
> + ivisurf->surface = NULL;
> + weston_layout_surfaceSetNativeContent(NULL, 0, 0,
> ivisurf->id_surface);
> + }
> +
> + wl_resource_destroy(resource);
> +}
> +
> +static const struct ivi_surface_interface surface_implementation = {
> + surface_destroy,
> +};
> +
> +static struct ivi_shell_surface *
> +is_surf_in_surfaces(struct wl_list *list_surf, uint32_t id_surface)
> +{
> + struct ivi_shell_surface *ivisurf;
> +
> + wl_list_for_each(ivisurf, list_surf, link) {
> + if (ivisurf->id_surface == id_surface) {
> + return ivisurf;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +static const struct {
> + uint32_t warning_code; /* enum ivi_surface_warning_code */
> + const char *msg;
> +} warning_strings[] = {
> + {IVI_SURFACE_WARNING_CODE_INVALID_WL_SURFACE, "wl_surface is invalid"},
> + {IVI_SURFACE_WARNING_CODE_SURFACE_ID_IN_USE, "surface_id is already
> assigned by another app"}
> +};
> +
> +/**
> + * Implementation of ivi_application::surface_create.
> + * Creating new ivi_shell_surface with identification to identify the surface
> + * in the system.
> + */
> +static void
> +application_surface_create(struct wl_client *client,
> + struct wl_resource *resource,
> + uint32_t id_surface,
> + struct wl_resource *surface_resource,
> + uint32_t id)
> +{
> + struct ivi_shell *shell = wl_resource_get_user_data(resource);
> + struct ivi_shell_surface *ivisurf = NULL;
> + struct weston_layout_surface *layout_surface = NULL;
> + struct weston_surface *es = wl_resource_get_user_data(surface_resource);
> + struct wl_resource *res;
> + int32_t warn_idx = -1;
> +
> + if (es != NULL) {
> + layout_surface = weston_layout_surfaceCreate(es, id_surface);
> + if (layout_surface == NULL)
> + warn_idx = 1;
> + } else {
> + warn_idx = 0;
I think this case would be a compositor internal error, not a client
error. Userdata of a surface resource should never be NULL. I.e.
assert() kind of thing would be more approriate.
> + }
> +
> + res = wl_resource_create(client, &ivi_surface_interface, 1, id);
> + if (res == NULL) {
> + weston_log("couldn't get surface object");
Use wl_resource_post_no_memory(ivi shell resource).
> + return;
> + }
> +
> + if (warn_idx >= 0) {
> + wl_resource_set_implementation(res, &surface_implementation,
> + NULL, NULL);
> + ivi_surface_send_warning(res,
> + warning_strings[warn_idx].warning_code,
> + warning_strings[warn_idx].msg);
> + return;
> + }
> +
> + ivisurf = is_surf_in_surfaces(&shell->ivi_surface_list, id_surface);
> + if (ivisurf == NULL) {
> + ivisurf = calloc(1, sizeof *ivisurf);
We now have zalloc() for this.
> + if (ivisurf == NULL) {
> + weston_log("fails to allocate memory\n");
> + return;
> + }
> +
> + wl_list_init(&ivisurf->link);
> + wl_list_insert(&shell->ivi_surface_list, &ivisurf->link);
> +
> + ivisurf->shell = shell;
> + ivisurf->id_surface = id_surface;
> + }
> +
> + ivisurf->width = 0;
> + ivisurf->height = 0;
> + ivisurf->layout_surface = layout_surface;
What is the benefit of having ivisurf and layout_surface separate?
Seem like you could just merge these, and simplify the code, on the
assumption that libweston-layout.so is specific to weston, no-one would
want to replace it without also replacing the ivi-shell.so, and hence
weston-layout is actually an implementation detail of ivi-shell.so for
exposing the API that ivi-controller.so expects.
> + ivisurf->surface = es;
> +
> + es->configure = ivi_shell_surface_configure;
> + es->configure_private = ivisurf;
> +
> + wl_resource_set_implementation(res, &surface_implementation,
> + ivisurf, NULL);
> +}
> +
> +static const struct ivi_application_interface application_implementation = {
> + application_surface_create
> +};
> +
> +static void
> +bind_ivi_application(struct wl_client *client,
> + void *data, uint32_t version, uint32_t id)
> +{
> + struct ivi_shell *shell = data;
> + struct wl_resource *resource = NULL;
> +
> + resource = wl_resource_create(client, &ivi_application_interface, 1, id);
> +
> + wl_resource_set_implementation(resource,
> + &application_implementation,
> + shell, NULL);
> +}
> +
> +/**
> + * Initialization/destruction method of ivi-shell
> + */
> +static void
> +shell_destroy(struct wl_listener *listener, void *data)
> +{
> + struct ivi_shell *shell =
> + container_of(listener, struct ivi_shell, destroy_listener);
> + struct ivi_shell_surface *ivisurf, *next;
> +
> + wl_list_for_each_safe(ivisurf, next, &shell->ivi_surface_list, link) {
> + wl_list_remove(&ivisurf->link);
> + free(ivisurf);
> + }
> +
> + free(shell);
> +}
> +
> +static void
> +init_ivi_shell(struct weston_compositor *ec, struct ivi_shell *shell)
> +{
> + shell->compositor = ec;
> +
> + wl_list_init(&ec->layer_list);
> + wl_list_init(&shell->ivi_surface_list);
> +}
> +
> +/**
> + * Initialization of ivi-shell. A library, weston_layout, is also initialized
> + * here by calling weston_layout_initWithCompositor.
> + *
> + */
> +
> +WL_EXPORT int
> +module_init(struct weston_compositor *ec,
'ec' is again cargo-cult, new code should prefer 'compositor'.
> + int *argc, char *argv[])
> +{
> + struct ivi_shell *shell = NULL;
> +
> + shell = calloc(1, sizeof *shell);
> + if (shell == NULL) {
> + return -1;
> + }
> +
> + init_ivi_shell(ec, shell);
> + weston_layout_initWithCompositor(ec);
> +
> + shell->destroy_listener.notify = shell_destroy;
> + wl_signal_add(&ec->destroy_signal, &shell->destroy_listener);
> +
> + if (wl_global_create(ec->wl_display, &ivi_application_interface, 1,
> + shell, bind_ivi_application) == NULL) {
> + return -1;
> + }
You are not setting compositor->shell_interface, but OTOH those are
only used for Xwayland, which probably doesn't make sense here.
> +
> + return 0;
> +}
Thanks,
pq
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
