[PATCH weston 08/11] compositor: Untangle mapedness from outputs

Armin Krezović krezovic.armin at gmail.com
Wed Jun 22 10:16:31 UTC 2016


On 21.06.2016 14:05, Pekka Paalanen wrote:
> On Sat, 18 Jun 2016 19:15:23 +0200
> Armin Krezović <krezovic.armin at gmail.com> wrote:
> 
>> Currently, weston assumes a surface/view is mapped if
>> it has an output assigned. In a zero outputs scenario,
>> this isn't really desirable.
>>
>> This patch introduces a new flag to weston_surface and
>> weston_view, which has to be set manually to indicate
>> that a surface/view is mapped.
>>
>> Signed-off-by: Armin Krezović <krezovic.armin at gmail.com>
> 
> Hi Armin,
> 
> there is a slight problem with the split and ordering in patches 8 -
> 11. Patch 8 removes the old conditions, which breaks "everything" and
> the follow-up patches fix things up piece by piece. The problem is that
> we now have known-broken commits from the start. Those will make
> bisecting unrelated issues harder.
> 

Ugh, that's a valid concern.

> There are two possible solutions:
> 
> - split patch 8 to to only add the new fields, and make a new patch as
>   the final one in the series that switches the
>   weston_{view,surface}_is_mapped() to use the new flags
> 
> - squash all patches 8 - 11 into one
> 
> Because the changes in patches 9 - 11 are fairly trivial and unlikely
> to break anything in themselves, the patch that switches the
> *_is_mapped() functions will be the one causing the regressions if any.
> Hence it would be also ok to just squash these into one patch. It
> doesn't make too much difference for reviews, and no difference for
> bisecting.
> 

I wanted to split the shells stuff mainly because different people work(ed)
on different shells. I like the first approach.

>> ---
>>  src/compositor.c  | 29 ++++++++++-------------------
>>  src/compositor.h  |  4 ++++
>>  src/data-device.c |  2 ++
>>  src/input.c       |  2 ++
>>  4 files changed, 18 insertions(+), 19 deletions(-)
> 
> There is also a more subtle change of semantics in this patch, which
> would be good to underline in the commit message.
> 
> Previously when mappedness was determined by outputs, unmapping the
> last view unmapped also the surface, or so I believe. That no longer
> happens, which is actually a good thing. This allows a shell to unmap
> all views without losing the mappedness information of the surface.
> 
> A practical example of this is hiding a desktop-shell top-level window.
> When the window is shown again, it should appear on the position where
> it was hidden, but that will not happen if desktop-shell thinks the
> surface is not mapped. Instead, desktop-shell will randomize the window
> position again. I haven't looked if this actually happens or if there
> is code to work around that, but I think the change by this patch is
> for the better.
> 
> The mappedness states of a surface and its views are no longer
> implicitly connected.
> 

I'm not sure I'm getting all this. We can discuss about it more on
IRC or I can just copy/paste most of the text above :).

>>
>> diff --git a/src/compositor.c b/src/compositor.c
>> index d246046..b25c671 100644
>> --- a/src/compositor.c
>> +++ b/src/compositor.c
>> @@ -1549,19 +1549,13 @@ weston_view_set_mask_infinite(struct weston_view *view)
>>  WL_EXPORT bool
>>  weston_view_is_mapped(struct weston_view *view)
>>  {
>> -	if (view->output)
>> -		return true;
>> -	else
>> -		return false;
>> +	return view->is_mapped;
>>  }
>>  
>>  WL_EXPORT bool
>>  weston_surface_is_mapped(struct weston_surface *surface)
>>  {
>> -	if (surface->output)
>> -		return true;
>> -	else
>> -		return false;
>> +	return surface->is_mapped;
>>  }
>>  
>>  static void
>> @@ -1738,6 +1732,7 @@ weston_view_unmap(struct weston_view *view)
>>  	weston_view_damage_below(view);
>>  	view->output = NULL;
>>  	view->plane = NULL;
>> +	view->is_mapped = false;
>>  	weston_layer_entry_remove(&view->layer_link);
>>  	wl_list_remove(&view->link);
>>  	wl_list_init(&view->link);
> 
> Looks like weston_view_unmap() has a problem. It won't fix up the input
> device foci unless the surface is not mapped. So even before this
> patch, if a shell unmaps a view that had pointer focus, the pointer
> focus is not removed. The same for touch. But this does not apply to
> kbd focus, because kbd focus is tracked by the surface, not by the view.
> 
> Then I wonder if the sloppy handling of pointer focus is actually
> relied on somewhere, or if it's just accidental. Or both.
> 
> Giulio, do you have any recommendations on what to do here?
> Since this ties in with the issue below...
> 
> Fixing this is not a matter for this patch, though.
> 
>> @@ -1769,6 +1764,7 @@ weston_surface_unmap(struct weston_surface *surface)
>>  
>>  	wl_list_for_each(view, &surface->views, surface_link)
>>  		weston_view_unmap(view);
>> +	surface->is_mapped = false;
>>  	surface->output = NULL;
> 
> Because weston_view_unmap() is called first, and surface->is_mapped is
> set only afterwards, the input foci clean-up never runs.
> 
> I think surface->is_mapped = false should be before the
> weston_view_unmap(). That way at least weston_surface_unmap() will work
> as expected. Previously, it relied on the last call to
> weston_view_unmap() -> weston_surface_assign_output() to mark the
> surface as not mapped via output assignment, so that the input foci
> clean-up was executed once all views were unmapped.
> 

Hm, I was just trying to mimic the behavior of the previous implementation.
Setting output to NULL meant unmapping the surface. Your suggestion makes
sense though.

>>  }
>>  
>> @@ -2131,6 +2127,7 @@ view_list_add_subsurface_view(struct weston_compositor *compositor,
>>  
>>  	view->parent_view = parent;
>>  	weston_view_update_transform(view);
>> +	view->is_mapped = true;
> 
> This looks correct.
> 
>>  
>>  	if (wl_list_empty(&sub->surface->subsurface_list)) {
>>  		wl_list_insert(compositor->view_list.prev, &view->link);
>> @@ -2152,6 +2149,7 @@ view_list_add(struct weston_compositor *compositor,
>>  	struct weston_subsurface *sub;
>>  
>>  	weston_view_update_transform(view);
>> +	view->is_mapped = true;
> 
> This I am not so sure about. Isn't it the responsibility of whoever
> puts a view on a layer to set is_mapped to true?
> 
> Doing it here might paper over some overlooked places.
> 

Again, I was trying to mimic the previous behavior. weston_view_update_transform
assigns an output to the view, and when the output got assigned it was assumed
the view was mapped.

>>  
>>  	if (wl_list_empty(&view->surface->subsurface_list)) {
>>  		wl_list_insert(compositor->view_list.prev, &view->link);
>> @@ -3244,7 +3242,6 @@ subsurface_get_label(struct weston_surface *surface, char *buf, size_t len)
>>  static void
>>  subsurface_configure(struct weston_surface *surface, int32_t dx, int32_t dy)
>>  {
>> -	struct weston_compositor *compositor = surface->compositor;
>>  	struct weston_view *view;
>>  
>>  	wl_list_for_each(view, &surface->views, surface_link)
>> @@ -3256,8 +3253,9 @@ subsurface_configure(struct weston_surface *surface, int32_t dx, int32_t dy)
>>  	 * mapped, parent is not in a visible layer, so this sub-surface
>>  	 * will not be drawn either.
>>  	 */
>> -	if (!weston_surface_is_mapped(surface)) {
>> -		struct weston_output *output;
>> +
>> +	if (!weston_surface_is_mapped(surface))
>> +		surface->is_mapped = true;
>>  
>>  		/* Cannot call weston_view_update_transform(),
>>  		 * because that would call it also for the parent surface,
>> @@ -3265,18 +3263,11 @@ subsurface_configure(struct weston_surface *surface, int32_t dx, int32_t dy)
>>  		 * inconsistent state, where the window could never be
>>  		 * mapped.
>>  		 *
>> -		 * Instead just assign any output, to make
>> +		 * Instead just force the is_mapped flag on, to make
>>  		 * weston_surface_is_mapped() return true, so that when the
>>  		 * parent surface does get mapped, this one will get
>>  		 * included, too. See view_list_add().
>>  		 */
>> -		assert(!wl_list_empty(&compositor->output_list));
>> -		output = container_of(compositor->output_list.next,
>> -				      struct weston_output, link);
>> -
>> -		surface->output = output;
>> -		weston_surface_update_output_mask(surface, 1u << output->id);
>> -	}
> 
> Correct, but I'd still like to keep the braces. The comment block looks
> quite out of place otherwise.
> 

OK.

>>  }
>>  
>>  static struct weston_subsurface *
>> diff --git a/src/compositor.h b/src/compositor.h
>> index 9e5155c..8770982 100644
>> --- a/src/compositor.h
>> +++ b/src/compositor.h
>> @@ -964,6 +964,8 @@ struct weston_view {
>>  
>>  	/* Per-surface Presentation feedback flags, controlled by backend. */
>>  	uint32_t psf_flags;
>> +
>> +	bool is_mapped;
>>  };
>>  
>>  struct weston_surface_state {
>> @@ -1082,6 +1084,8 @@ struct weston_surface {
>>  	const char *role_name;
>>  
>>  	struct weston_timeline_object timeline;
>> +
>> +	bool is_mapped;
>>  };
>>  
>>  struct weston_subsurface {
>> diff --git a/src/data-device.c b/src/data-device.c
>> index f04f030..44a08f9 100644
>> --- a/src/data-device.c
>> +++ b/src/data-device.c
>> @@ -421,6 +421,8 @@ drag_surface_configure(struct weston_drag *drag,
>>  		weston_layer_entry_insert(list, &drag->icon->layer_link);
>>  		weston_view_update_transform(drag->icon);
>>  		pixman_region32_clear(&es->pending.input);
>> +		es->is_mapped = true;
>> +		drag->icon->is_mapped = true;
> 
> Looks correct.
> 
>>  	}
>>  
>>  	drag->dx += sx;
>> diff --git a/src/input.c b/src/input.c
>> index 08378d1..5ccc75f 100644
>> --- a/src/input.c
>> +++ b/src/input.c
>> @@ -1950,6 +1950,8 @@ pointer_cursor_surface_configure(struct weston_surface *es,
>>  		weston_layer_entry_insert(&es->compositor->cursor_layer.view_list,
>>  					  &pointer->sprite->layer_link);
>>  		weston_view_update_transform(pointer->sprite);
>> +		es->is_mapped = true;
>> +		pointer->sprite->is_mapped = true;
>>  	}
>>  }
>>  
> 
> Looks correct.
> 
> I didn't yet search for overlooked places, but I should do that once I
> have reviewed the last patches of this series.
> 
> 
> Thanks,
> pq
> 

Thanks for the review.

Armin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 855 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160622/f2243d78/attachment.sig>


More information about the wayland-devel mailing list