[PATCH weston 01/10] desktop-shell: Simplify popup_end_grab popup_done sending loop

[Pekka Paalanen]

On Fri, 13 Feb 2015 14:01:53 +0800
Jonas Ådahl <jadahl at gmail.com> wrote:

> Can just use wl_list_for_each_safe instead of dealing with pointers
> ourself.
> 
> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> ---
>  desktop-shell/shell.c | 36 +++++++++++++++---------------------
>  1 file changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index f28fc10..db0c5a9 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -3059,20 +3059,18 @@ destroy_shell_seat(struct wl_listener *listener, void *data)
>  	struct shell_seat *shseat =
>  		container_of(listener,
>  			     struct shell_seat, seat_destroy_listener);
> -	struct shell_surface *shsurf, *prev = NULL;
> +	struct shell_surface *shsurf, *next;
>  
>  	if (shseat->popup_grab.grab.interface == &popup_grab_interface) {
>  		weston_pointer_end_grab(shseat->popup_grab.grab.pointer);
>  		shseat->popup_grab.client = NULL;
>  
> -		wl_list_for_each(shsurf, &shseat->popup_grab.surfaces_list, popup.grab_link) {
> +		wl_list_for_each_safe(shsurf, next,
> +				      &shseat->popup_grab.surfaces_list,
> +				      popup.grab_link) {
>  			shsurf->popup.shseat = NULL;
> -			if (prev) {
> -				wl_list_init(&prev->popup.grab_link);
> -			}
> -			prev = shsurf;
> +			wl_list_init(&shsurf->popup.grab_link);
>  		}
> -		wl_list_init(&prev->popup.grab_link);

Hi,

while I can understand that this code very likely works right, I feel a
little awkward using wl_list_init() where one would normally use
wl_list_remove(). Even though it is somewhat redundant, I would like to
see wl_list_remove() followed by wl_list_init(), iff grab_link really
needs to be initialized.

But, I see the old code already did the awkward thing, so fixing that
can be left for another patch.

All my comments for the below hunks are just the same.

This patch pushed with my R-b.

   d2c6892..c2b1011  master -> master


Thanks,
pq

>  	}
>  
>  	wl_list_remove(&shseat->seat_destroy_listener.link);
> @@ -3328,7 +3326,7 @@ popup_grab_end(struct weston_pointer *pointer)
>  	struct shell_seat *shseat =
>  	    container_of(grab, struct shell_seat, popup_grab.grab);
>  	struct shell_surface *shsurf;
> -	struct shell_surface *prev = NULL;
> +	struct shell_surface *next;
>  
>  	if (pointer->grab->interface == &popup_grab_interface) {
>  		weston_pointer_end_grab(grab->pointer);
> @@ -3336,15 +3334,13 @@ popup_grab_end(struct weston_pointer *pointer)
>  		shseat->popup_grab.grab.interface = NULL;
>  		assert(!wl_list_empty(&shseat->popup_grab.surfaces_list));
>  		/* Send the popup_done event to all the popups open */
> -		wl_list_for_each(shsurf, &shseat->popup_grab.surfaces_list, popup.grab_link) {
> +		wl_list_for_each_safe(shsurf, next,
> +				      &shseat->popup_grab.surfaces_list,
> +				      popup.grab_link) {
>  			shell_surface_send_popup_done(shsurf);
>  			shsurf->popup.shseat = NULL;
> -			if (prev) {
> -				wl_list_init(&prev->popup.grab_link);
> -			}
> -			prev = shsurf;
> +			wl_list_init(&shsurf->popup.grab_link);
>  		}
> -		wl_list_init(&prev->popup.grab_link);
>  		wl_list_init(&shseat->popup_grab.surfaces_list);
>  	}
>  }
> @@ -3356,7 +3352,7 @@ touch_popup_grab_end(struct weston_touch *touch)
>  	struct shell_seat *shseat =
>  	    container_of(grab, struct shell_seat, popup_grab.touch_grab);
>  	struct shell_surface *shsurf;
> -	struct shell_surface *prev = NULL;
> +	struct shell_surface *next;
>  
>  	if (touch->grab->interface == &touch_popup_grab_interface) {
>  		weston_touch_end_grab(grab->touch);
> @@ -3364,15 +3360,13 @@ touch_popup_grab_end(struct weston_touch *touch)
>  		shseat->popup_grab.touch_grab.interface = NULL;
>  		assert(!wl_list_empty(&shseat->popup_grab.surfaces_list));
>  		/* Send the popup_done event to all the popups open */
> -		wl_list_for_each(shsurf, &shseat->popup_grab.surfaces_list, popup.grab_link) {
> +		wl_list_for_each_safe(shsurf, next,
> +				      &shseat->popup_grab.surfaces_list,
> +				      popup.grab_link) {
>  			shell_surface_send_popup_done(shsurf);
>  			shsurf->popup.shseat = NULL;
> -			if (prev) {
> -				wl_list_init(&prev->popup.grab_link);
> -			}
> -			prev = shsurf;
> +			wl_list_init(&shsurf->popup.grab_link);
>  		}
> -		wl_list_init(&prev->popup.grab_link);
>  		wl_list_init(&shseat->popup_grab.surfaces_list);
>  	}
>  }