[PATCH weston 2/3] ivi-shell: introduce ivi_layout_view
Pekka Paalanen
ppaalanen at gmail.com
Mon Jun 13 11:07:23 UTC 2016
On Tue, 7 Jun 2016 09:40:20 +0000
"Ucan, Emre (ADITG/SW1)" <eucan at de.adit-jv.com> wrote:
> This patch introduces ivi_layout_view data struct,
> which is a wrapper of weston_view.
>
> There is always only one ivi_layout_view for an
> ivi_layout_surface and ivi_layout_layer pair.
> A surface could have many views with different
> geometry and transformations, so that a surface
> can be shown on:
> 1. On many screens
> 2. On the same screen with different positions
>
> The geometry of a view is modified, when properties of
> its layer are changed through ivi_layout_interface.
> Users of ivi_layout_interface does not have direct access
> to ivi_layout_view structure.
>
> Signed-off-by: Emre Ucan <eucan at de.adit-jv.com>
> ---
> ivi-shell/ivi-layout-private.h | 26 +++-
> ivi-shell/ivi-layout.c | 280 ++++++++++++++++++++++++----------------
> 2 files changed, 189 insertions(+), 117 deletions(-)
Hi Emre,
this looks pretty good on the surface, but there are some details you
might want to consider.
> diff --git a/ivi-shell/ivi-layout-private.h b/ivi-shell/ivi-layout-private.h
> index 8fca68f..87a7e47 100644
> --- a/ivi-shell/ivi-layout-private.h
> +++ b/ivi-shell/ivi-layout-private.h
> @@ -29,6 +29,22 @@
> #include "compositor.h"
> #include "ivi-layout-export.h"
>
> +struct ivi_layout_view {
> + struct wl_list link;
> + struct wl_list surf_link;
> + struct wl_list pending_link;
> + struct wl_list order_link;
These four could use comments on which lists they are used with. Also
all the list heads (see below) could use comments on which struct::link
is contained in them. Especially the latter information is an important
invariant for code correctness.
It would be really nice to have patches adding such comments to
existing members of type wl_list, too.
> +
> + struct weston_view *view;
> + struct weston_transform transform;
> +
> + struct ivi_layout_surface *ivisurf;
> + struct ivi_layout_layer *on_layer;
> +
> + /*It is true, if the ivi_view is on the render order
> + * list of the layer.*/
> + bool active;
Could we use a more descriptive name for this member? I think "active"
is getting a little overloaded.
Since you have separate links for pending and order, could this member
be replaced with the following?
static bool
ivi_layout_view_is_rendered(struct ivi_layout_view *view)
{
return !wl_list_empty(&view->order_link);
}
Assuming that the condition matches your purpose and the link is always
kept valid, e.g. after remove you always call init (except in the
destructor). An init'd link counts as an empty list.
> +};
> struct ivi_layout_surface {
> struct wl_list link;
> struct wl_signal property_changed;
> @@ -36,11 +52,8 @@ struct ivi_layout_surface {
> uint32_t id_surface;
>
> struct ivi_layout *layout;
> - struct ivi_layout_layer *on_layer;
> struct weston_surface *surface;
>
> - struct weston_transform transform;
> -
> struct ivi_layout_surface_properties prop;
>
> struct {
> @@ -52,6 +65,8 @@ struct ivi_layout_surface {
> struct wl_list link;
> struct wl_list layer_list;
> } order;
> +
> + struct wl_list view_list;
This is an example of a list head I referred to above.
> };
>
> struct ivi_layout_layer {
> @@ -66,13 +81,13 @@ struct ivi_layout_layer {
>
> struct {
> struct ivi_layout_layer_properties prop;
> - struct wl_list surface_list;
> + struct wl_list view_list;
> struct wl_list link;
> } pending;
>
> struct {
> int dirty;
> - struct wl_list surface_list;
> + struct wl_list view_list;
> struct wl_list link;
> } order;
>
> @@ -85,6 +100,7 @@ struct ivi_layout {
> struct wl_list surface_list;
> struct wl_list layer_list;
> struct wl_list screen_list;
> + struct wl_list view_list;
>
> struct {
> struct wl_signal created;
> diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c
> index 9ce0ab0..9ce587d 100644
> --- a/ivi-shell/ivi-layout.c
> +++ b/ivi-shell/ivi-layout.c
> @@ -139,20 +139,69 @@ get_layer(struct wl_list *layer_list, uint32_t id_layer)
> return NULL;
> }
>
> -static struct weston_view *
> -get_weston_view(struct ivi_layout_surface *ivisurf)
> +static void
> +ivi_view_destroy(struct ivi_layout_view *ivi_view)
> +{
> + wl_list_remove(&ivi_view->transform.link);
> + wl_list_remove(&ivi_view->link);
> + wl_list_remove(&ivi_view->surf_link);
> + wl_list_remove(&ivi_view->pending_link);
> + wl_list_remove(&ivi_view->order_link);
> +
> + weston_view_destroy(ivi_view->view);
> +
> + free(ivi_view);
> +}
> +
> +static struct ivi_layout_view*
> +ivi_view_create(struct ivi_layout_layer *ivilayer,
> + struct ivi_layout_surface *ivisurf)
> +{
> + struct ivi_layout_view *ivi_view;
> +
> + ivi_view = calloc(1, sizeof *ivi_view);
We have a zalloc() wrapper for this.
> + if (ivi_view == NULL) {
> + weston_log("fails to allocate memory\n");
> + return NULL;
> + }
> +
> + ivi_view->view = weston_view_create(ivisurf->surface);
> + if (ivi_view->view == NULL) {
> + weston_log("fails to allocate memory\n");
> + return NULL;
> + }
> +
> + weston_matrix_init(&ivi_view->transform.matrix);
> + wl_list_init(&ivi_view->transform.link);
> +
> + ivi_view->ivisurf = ivisurf;
> + ivi_view->on_layer = ivilayer;
> + ivi_view->active = false;
> + wl_list_insert(&ivilayer->layout->view_list,
> + &ivi_view->link);
> + wl_list_insert(&ivisurf->view_list,
> + &ivi_view->surf_link);
> +
> + wl_list_init(&ivi_view->pending_link);
> + wl_list_init(&ivi_view->order_link);
> +
> + return ivi_view;
> +}
Ok, and looking at the hunk below, when a ivi_layout_surface is
destroyed then all views related to that are destroyed, and when a
ivi_layout_layer is destroyed then all views on that are destroyed as
well. Sounds good.
> +
> +static struct ivi_layout_view *
> +get_ivi_view(struct ivi_layout_layer *ivilayer,
> + struct ivi_layout_surface *ivisurf)
> {
> - struct weston_view *view = NULL;
> + struct ivi_layout_view *ivi_view;
>
> assert(ivisurf->surface != NULL);
>
> - /* One view per surface */
> - if(wl_list_empty(&ivisurf->surface->views))
> - view = NULL;
> - else
> - view = wl_container_of(ivisurf->surface->views.next, view, surface_link);
> + wl_list_for_each(ivi_view, &ivisurf->view_list, surf_link) {
> + if (ivi_view->on_layer == ivilayer)
> + return ivi_view;
> + }
>
> - return view;
> + return NULL;
> }
>
> static struct ivi_layout_screen *
> @@ -176,17 +225,21 @@ void
> ivi_layout_surface_destroy(struct ivi_layout_surface *ivisurf)
> {
> struct ivi_layout *layout = get_instance();
> + struct ivi_layout_view *ivi_view ,*next;
>
> if (ivisurf == NULL) {
> weston_log("%s: invalid argument\n", __func__);
> return;
> }
>
> - wl_list_remove(&ivisurf->transform.link);
> wl_list_remove(&ivisurf->pending.link);
> wl_list_remove(&ivisurf->order.link);
> wl_list_remove(&ivisurf->link);
>
> + wl_list_for_each_safe(ivi_view, next, &ivisurf->view_list, surf_link) {
> + ivi_view_destroy(ivi_view);
> + }
> +
> wl_signal_emit(&layout->surface_notification.removed, ivisurf);
>
> ivi_layout_remove_all_surface_transitions(ivisurf);
> @@ -258,15 +311,13 @@ init_surface_properties(struct ivi_layout_surface_properties *prop)
> */
> static void
> update_opacity(struct ivi_layout_layer *ivilayer,
> - struct ivi_layout_surface *ivisurf)
> + struct ivi_layout_surface *ivisurf,
> + struct weston_view *view)
> {
> - struct weston_view *tmpview = NULL;
> double layer_alpha = wl_fixed_to_double(ivilayer->prop.opacity);
> double surf_alpha = wl_fixed_to_double(ivisurf->prop.opacity);
>
> - tmpview = get_weston_view(ivisurf);
> - assert(tmpview != NULL);
> - tmpview->alpha = layer_alpha * surf_alpha;
> + view->alpha = layer_alpha * surf_alpha;
> }
>
> static void
> @@ -563,20 +614,19 @@ calc_surface_to_global_matrix_and_mask_to_weston_surface(
> static void
> update_prop(struct ivi_layout_screen *iviscrn,
> struct ivi_layout_layer *ivilayer,
> - struct ivi_layout_surface *ivisurf)
> + struct ivi_layout_view *ivi_view)
Is it guaranteed that ivi_view->on_layer == ivilayer?
If yes, maybe this could be simplified a bit, or at least add an assert
to ensure it.
> {
> - struct weston_view *tmpview;
> + struct ivi_layout_surface *ivisurf;
> struct ivi_rectangle r;
> bool can_calc = true;
>
> + ivisurf = ivi_view->ivisurf;
> +
> /*In case of no prop change, this just returns*/
> if (!ivilayer->prop.event_mask && !ivisurf->prop.event_mask)
> return;
>
> - update_opacity(ivilayer, ivisurf);
> -
> - tmpview = get_weston_view(ivisurf);
> - assert(tmpview != NULL);
> + update_opacity(ivilayer, ivisurf, ivi_view->view);
>
> if (ivisurf->prop.source_width == 0 || ivisurf->prop.source_height == 0) {
> weston_log("ivi-shell: source rectangle is not yet set by ivi_layout_surface_set_source_rectangle\n");
> @@ -589,22 +639,22 @@ update_prop(struct ivi_layout_screen *iviscrn,
> }
>
> if (can_calc) {
> - wl_list_remove(&ivisurf->transform.link);
> - weston_matrix_init(&ivisurf->transform.matrix);
> + wl_list_remove(&ivi_view->transform.link);
> + weston_matrix_init(&ivi_view->transform.matrix);
>
> calc_surface_to_global_matrix_and_mask_to_weston_surface(
> - iviscrn, ivilayer, ivisurf, &ivisurf->transform.matrix, &r);
> + iviscrn, ivilayer, ivisurf, &ivi_view->transform.matrix, &r);
>
> - weston_view_set_mask(tmpview, r.x, r.y, r.width, r.height);
> - wl_list_insert(&tmpview->geometry.transformation_list,
> - &ivisurf->transform.link);
> + weston_view_set_mask(ivi_view->view, r.x, r.y, r.width, r.height);
> + wl_list_insert(&ivi_view->view->geometry.transformation_list,
> + &ivi_view->transform.link);
>
> - weston_view_set_transform_parent(tmpview, NULL);
> + weston_view_set_transform_parent(ivi_view->view, NULL);
> }
>
> ivisurf->update_count++;
>
> - weston_view_geometry_dirty(tmpview);
> + weston_view_geometry_dirty(ivi_view->view);
>
> weston_surface_damage(ivisurf->surface);
> }
> @@ -614,7 +664,7 @@ commit_changes(struct ivi_layout *layout)
> {
> struct ivi_layout_screen *iviscrn = NULL;
> struct ivi_layout_layer *ivilayer = NULL;
> - struct ivi_layout_surface *ivisurf = NULL;
> + struct ivi_layout_view *ivi_view = NULL;
>
> wl_list_for_each(iviscrn, &layout->screen_list, link) {
> wl_list_for_each(ivilayer, &iviscrn->order.layer_list, order.link) {
> @@ -625,15 +675,15 @@ commit_changes(struct ivi_layout *layout)
> if (ivilayer->prop.visibility == false)
> continue;
>
> - wl_list_for_each(ivisurf, &ivilayer->order.surface_list, order.link) {
> + wl_list_for_each(ivi_view, &ivilayer->order.view_list, order_link) {
> /*
> * If ivilayer is invisible, weston_view of ivisurf doesn't
> * need to be modified.
> */
> - if (ivisurf->prop.visibility == false)
> + if (ivi_view->ivisurf->prop.visibility == false)
> continue;
>
> - update_prop(iviscrn, ivilayer, ivisurf);
> + update_prop(iviscrn, ivilayer, ivi_view);
> }
> }
> }
> @@ -744,9 +794,9 @@ commit_surface_list(struct ivi_layout *layout)
> static void
> commit_layer_list(struct ivi_layout *layout)
> {
> + struct ivi_layout_view *ivi_view = NULL;
> struct ivi_layout_layer *ivilayer = NULL;
> - struct ivi_layout_surface *ivisurf = NULL;
> - struct ivi_layout_surface *next = NULL;
> + struct ivi_layout_view *next = NULL;
>
> wl_list_for_each(ivilayer, &layout->layer_list, link) {
> if (ivilayer->pending.prop.transition_type == IVI_LAYOUT_TRANSITION_LAYER_MOVE) {
> @@ -765,23 +815,22 @@ commit_layer_list(struct ivi_layout *layout)
> continue;
> }
>
> - wl_list_for_each_safe(ivisurf, next, &ivilayer->order.surface_list,
> - order.link) {
> - ivisurf->on_layer = NULL;
> - wl_list_remove(&ivisurf->order.link);
> - wl_list_init(&ivisurf->order.link);
> - ivisurf->prop.event_mask |= IVI_NOTIFICATION_REMOVE;
> + wl_list_for_each_safe(ivi_view, next, &ivilayer->order.view_list,
> + order_link) {
> + ivi_view->active = false;
> + wl_list_remove(&ivi_view->order_link);
> + wl_list_init(&ivi_view->order_link);
> + ivi_view->ivisurf->prop.event_mask |= IVI_NOTIFICATION_REMOVE;
> }
>
> - assert(wl_list_empty(&ivilayer->order.surface_list));
> + assert(wl_list_empty(&ivilayer->order.view_list));
>
> - wl_list_for_each(ivisurf, &ivilayer->pending.surface_list,
> - pending.link) {
> - wl_list_remove(&ivisurf->order.link);
> - wl_list_insert(&ivilayer->order.surface_list,
> - &ivisurf->order.link);
> - ivisurf->on_layer = ivilayer;
> - ivisurf->prop.event_mask |= IVI_NOTIFICATION_ADD;
> + wl_list_for_each(ivi_view, &ivilayer->pending.view_list,
> + pending_link) {
> + ivi_view->active = true;
> + wl_list_remove(&ivi_view->order_link);
> + wl_list_insert(&ivilayer->order.view_list, &ivi_view->order_link);
> + ivi_view->ivisurf->prop.event_mask |= IVI_NOTIFICATION_ADD;
> }
>
> ivilayer->order.dirty = 0;
> @@ -794,8 +843,7 @@ commit_screen_list(struct ivi_layout *layout)
> struct ivi_layout_screen *iviscrn = NULL;
> struct ivi_layout_layer *ivilayer = NULL;
> struct ivi_layout_layer *next = NULL;
> - struct ivi_layout_surface *ivisurf = NULL;
> - struct weston_view *tmpview = NULL;
> + struct ivi_layout_view *ivi_view = NULL;
>
> /* Clear view list of layout ivi_layer */
> wl_list_init(&layout->layout_layer.view_list.link);
> @@ -830,17 +878,14 @@ commit_screen_list(struct ivi_layout *layout)
> if (ivilayer->prop.visibility == false)
> continue;
>
> - wl_list_for_each(ivisurf, &ivilayer->order.surface_list, order.link) {
> - if (ivisurf->prop.visibility == false)
> + wl_list_for_each(ivi_view, &ivilayer->order.view_list, order_link) {
> + if (ivi_view->ivisurf->prop.visibility == false)
> continue;
>
> - tmpview = get_weston_view(ivisurf);
> - assert(tmpview != NULL);
> -
> weston_layer_entry_insert(&layout->layout_layer.view_list,
> - &tmpview->layer_link);
> + &ivi_view->view->layer_link);
>
> - ivisurf->surface->output = iviscrn->output;
> + ivi_view->view->output = iviscrn->output;
This change raises an eyebrow. Previously it was forcing the
weston_surface::output, now it is forcing the weston_view::output. Is
that an intentional change?
Forcing outputs is usually done as part of mapping a surface or view,
because weston_{surface,view}_is_mapped() looks at the output. This is
something Armin in looking at to fix, since output assignment is not
quite the same as being mapped.
It is fairly hard to understand what is happening right now with the
mappedness state as it is confused with the output assignments, so if
this change works in practise, that's enough for me.
> }
> }
> }
> @@ -893,29 +938,15 @@ send_prop(struct ivi_layout *layout)
> }
>
> static void
> -clear_surface_pending_list(struct ivi_layout_layer *ivilayer)
> -{
> - struct ivi_layout_surface *surface_link = NULL;
> - struct ivi_layout_surface *surface_next = NULL;
> -
> - wl_list_for_each_safe(surface_link, surface_next,
> - &ivilayer->pending.surface_list, pending.link) {
> - wl_list_remove(&surface_link->pending.link);
> - wl_list_init(&surface_link->pending.link);
> - }
> -}
> -
> -static void
> -clear_surface_order_list(struct ivi_layout_layer *ivilayer)
> +clear_view_pending_list(struct ivi_layout_layer *ivilayer)
> {
> - struct ivi_layout_surface *surface_link = NULL;
> - struct ivi_layout_surface *surface_next = NULL;
> + struct ivi_layout_view *view_link = NULL;
> + struct ivi_layout_view *view_next = NULL;
>
> - wl_list_for_each_safe(surface_link, surface_next,
> - &ivilayer->order.surface_list, order.link) {
> - wl_list_remove(&surface_link->order.link);
> - wl_list_init(&surface_link->order.link);
> - surface_link->on_layer = NULL;
> + wl_list_for_each_safe(view_link, view_next,
> + &ivilayer->pending.view_list, pending_link) {
> + wl_list_remove(&view_link->pending_link);
> + wl_list_init(&view_link->pending_link);
> }
> }
>
> @@ -1171,6 +1202,7 @@ ivi_layout_get_layers_under_surface(struct ivi_layout_surface *ivisurf,
> int32_t *pLength,
> struct ivi_layout_layer ***ppArray)
> {
> + struct ivi_layout_view *ivi_view;
> int32_t length = 0;
> int32_t n = 0;
>
> @@ -1179,20 +1211,30 @@ ivi_layout_get_layers_under_surface(struct ivi_layout_surface *ivisurf,
> return IVI_FAILED;
> }
>
> - if (ivisurf->on_layer != NULL) {
> - /* the Array must be freed by module which called this function */
> - length = 1;
> - *ppArray = calloc(length, sizeof(struct ivi_layout_screen *));
> + if (!wl_list_empty(&ivisurf->view_list)) {
> + /* the Array must be free by module which called this function */
> + length = wl_list_length(&ivisurf->view_list);
> + *ppArray = calloc(length, sizeof(struct ivi_layout_layer *));
> if (*ppArray == NULL) {
> weston_log("fails to allocate memory\n");
> return IVI_FAILED;
> }
>
> - (*ppArray)[n++] = ivisurf->on_layer;
> + wl_list_for_each_reverse(ivi_view, &ivisurf->view_list, surf_link) {
> + if (ivi_view->active)
> + (*ppArray)[n++] = ivi_view->on_layer;
> + else
> + length--;
> + }
Btw. is it documented what happens if a surface is on the same layer
multiple times? Is it possible or forbidden?
That affects not just here, but also
ivi_layout_get_surfaces_on_layer(), ivi_layout_layer_set_render_order()
and ivi_layout_layer_add_surface()?
Looking at set_render_order() and add_surface, a surface cannot be on
the same layer multiple times, but it also not an error to try to do
that. The end result just happens to be the last element winning.
> }
>
> *pLength = length;
>
> + if (!length) {
> + free(*ppArray);
> + *ppArray = NULL;
> + }
> +
> return IVI_SUCCEEDED;
> }
>
> @@ -1235,7 +1277,7 @@ ivi_layout_get_surfaces_on_layer(struct ivi_layout_layer *ivilayer,
> int32_t *pLength,
> struct ivi_layout_surface ***ppArray)
> {
> - struct ivi_layout_surface *ivisurf = NULL;
> + struct ivi_layout_view *ivi_view = NULL;
> int32_t length = 0;
> int32_t n = 0;
>
> @@ -1244,7 +1286,7 @@ ivi_layout_get_surfaces_on_layer(struct ivi_layout_layer *ivilayer,
> return IVI_FAILED;
> }
>
> - length = wl_list_length(&ivilayer->order.surface_list);
> + length = wl_list_length(&ivilayer->order.view_list);
>
> if (length != 0) {
> /* the Array must be freed by module which called this function */
> @@ -1254,8 +1296,8 @@ ivi_layout_get_surfaces_on_layer(struct ivi_layout_layer *ivilayer,
> return IVI_FAILED;
> }
>
> - wl_list_for_each(ivisurf, &ivilayer->order.surface_list, order.link) {
> - (*ppArray)[n++] = ivisurf;
> + wl_list_for_each(ivi_view, &ivilayer->order.view_list, order_link) {
> + (*ppArray)[n++] = ivi_view->ivisurf;
> }
> }
>
> @@ -1291,11 +1333,11 @@ ivi_layout_layer_create_with_dimension(uint32_t id_layer,
>
> init_layer_properties(&ivilayer->prop, width, height);
>
> - wl_list_init(&ivilayer->pending.surface_list);
> + wl_list_init(&ivilayer->pending.view_list);
> wl_list_init(&ivilayer->pending.link);
> ivilayer->pending.prop = ivilayer->prop;
>
> - wl_list_init(&ivilayer->order.surface_list);
> + wl_list_init(&ivilayer->order.view_list);
> wl_list_init(&ivilayer->order.link);
>
> wl_list_insert(&layout->layer_list, &ivilayer->link);
> @@ -1309,6 +1351,7 @@ static void
> ivi_layout_layer_destroy(struct ivi_layout_layer *ivilayer)
> {
> struct ivi_layout *layout = get_instance();
> + struct ivi_layout_view *ivi_view, *next;
>
> if (ivilayer == NULL) {
> weston_log("ivi_layout_layer_remove: invalid argument\n");
> @@ -1318,10 +1361,13 @@ ivi_layout_layer_destroy(struct ivi_layout_layer *ivilayer)
> if (--ivilayer->ref_count > 0)
> return;
>
> - wl_signal_emit(&layout->layer_notification.removed, ivilayer);
> + /*Destroy all ivi_views*/
> + wl_list_for_each_safe(ivi_view, next, &layout->view_list, link) {
> + if (ivi_view->on_layer == ivilayer)
Is there any reason why this condition would legitimately be false?
If not, I would recommend making this an assert instead.
> + ivi_view_destroy(ivi_view);
> + }
>
> - clear_surface_pending_list(ivilayer);
> - clear_surface_order_list(ivilayer);
> + wl_signal_emit(&layout->layer_notification.removed, ivilayer);
>
> wl_list_remove(&ivilayer->pending.link);
> wl_list_remove(&ivilayer->order.link);
> @@ -1460,18 +1506,24 @@ ivi_layout_layer_set_render_order(struct ivi_layout_layer *ivilayer,
> int32_t number)
> {
> int32_t i = 0;
> + struct ivi_layout_view * ivi_view;
>
> if (ivilayer == NULL) {
> weston_log("ivi_layout_layer_set_render_order: invalid argument\n");
> return IVI_FAILED;
> }
>
> - clear_surface_pending_list(ivilayer);
> + clear_view_pending_list(ivilayer);
>
> for (i = 0; i < number; i++) {
> - wl_list_remove(&pSurface[i]->pending.link);
> - wl_list_insert(&ivilayer->pending.surface_list,
> - &pSurface[i]->pending.link);
> + ivi_view = get_ivi_view(ivilayer, pSurface[i]);
> + if (!ivi_view)
> + ivi_view = ivi_view_create(ivilayer, pSurface[i]);
> +
> + assert(ivi_view != NULL);
> +
> + wl_list_remove(&ivi_view->pending_link);
> + wl_list_insert(&ivilayer->pending.view_list, &ivi_view->pending_link);
> }
>
> ivilayer->order.dirty = 1;
> @@ -1709,16 +1761,21 @@ static int32_t
> ivi_layout_layer_add_surface(struct ivi_layout_layer *ivilayer,
> struct ivi_layout_surface *addsurf)
> {
> + struct ivi_layout_view *ivi_view;
> +
> if (ivilayer == NULL || addsurf == NULL) {
> weston_log("ivi_layout_layer_add_surface: invalid argument\n");
> return IVI_FAILED;
> }
>
> - if (addsurf->on_layer == ivilayer)
> + ivi_view = get_ivi_view(ivilayer, addsurf);
> + if (!ivi_view)
> + ivi_view = ivi_view_create(ivilayer, addsurf);
> + else if (ivi_view->active)
> return IVI_SUCCEEDED;
If an existing view is found, tying the surface to the layer, is it
still possible that 'active' can be false?
When you add a surface to a layer it is already on, this function does
not change the z-order, but _set_render_order() does?
>
> - wl_list_remove(&addsurf->pending.link);
> - wl_list_insert(&ivilayer->pending.surface_list, &addsurf->pending.link);
> + wl_list_remove(&ivi_view->pending_link);
> + wl_list_insert(&ivilayer->pending.view_list, &ivi_view->pending_link);
>
> ivilayer->order.dirty = 1;
>
> @@ -1729,15 +1786,20 @@ static void
> ivi_layout_layer_remove_surface(struct ivi_layout_layer *ivilayer,
> struct ivi_layout_surface *remsurf)
> {
> + struct ivi_layout_view *ivi_view;
> +
> if (ivilayer == NULL || remsurf == NULL) {
> weston_log("ivi_layout_layer_remove_surface: invalid argument\n");
> return;
> }
>
> - wl_list_remove(&remsurf->pending.link);
> - wl_list_init(&remsurf->pending.link);
> + ivi_view = get_ivi_view(ivilayer, remsurf);
> + if (ivi_view && ivi_view->active) {
Do you care about 'active' really?
Hmm, so if the view is already on the pending list, but not in the
order list or active, calling remove_surface will not remove it from
the pending list? Which means that on the next commit_changes() the
removed surface becomes visible.
> + wl_list_remove(&ivi_view->pending_link);
> + wl_list_init(&ivi_view->pending_link);
>
> - ivilayer->order.dirty = 1;
> + ivilayer->order.dirty = 1;
> + }
> }
>
> static int32_t
> @@ -1893,7 +1955,6 @@ ivi_layout_surface_create(struct weston_surface *wl_surface,
> {
> struct ivi_layout *layout = get_instance();
> struct ivi_layout_surface *ivisurf = NULL;
> - struct weston_view *tmpview = NULL;
>
> if (wl_surface == NULL) {
> weston_log("ivi_layout_surface_create: invalid argument\n");
> @@ -1920,17 +1981,9 @@ ivi_layout_surface_create(struct weston_surface *wl_surface,
>
> ivisurf->surface = wl_surface;
>
> - tmpview = weston_view_create(wl_surface);
> - if (tmpview == NULL) {
> - weston_log("fails to allocate memory\n");
> - }
> -
> ivisurf->surface->width_from_buffer = 0;
> ivisurf->surface->height_from_buffer = 0;
>
> - weston_matrix_init(&ivisurf->transform.matrix);
> - wl_list_init(&ivisurf->transform.link);
> -
> init_surface_properties(&ivisurf->prop);
>
> ivisurf->pending.prop = ivisurf->prop;
> @@ -1939,6 +1992,8 @@ ivi_layout_surface_create(struct weston_surface *wl_surface,
> wl_list_init(&ivisurf->order.link);
> wl_list_init(&ivisurf->order.layer_list);
>
> + wl_list_init(&ivisurf->view_list);
> +
> wl_list_insert(&layout->surface_list, &ivisurf->link);
>
> wl_signal_emit(&layout->surface_notification.created, ivisurf);
> @@ -1956,6 +2011,7 @@ ivi_layout_init_with_compositor(struct weston_compositor *ec)
> wl_list_init(&layout->surface_list);
> wl_list_init(&layout->layer_list);
> wl_list_init(&layout->screen_list);
> + wl_list_init(&layout->view_list);
>
> wl_signal_init(&layout->layer_notification.created);
> wl_signal_init(&layout->layer_notification.removed);
As such this gets only
Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
because I think many of the details could be cleared.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160613/30f88a17/attachment.sig>
More information about the wayland-devel
mailing list