From e9aef7caad55b99935965c7b6e0c51b2f14f6b2a Mon Sep 17 00:00:00 2001
From: cmeissl <meissl.christian@gmail.com>
Date: Tue, 15 Jun 2021 23:32:02 +0200
Subject: [PATCH] rework xdg_shell (#286)

* rework xdg_shell
use distinct surface roles for xdg_toplevel and
xdg_popup using a xdg_role! macro

* fix clippy warnings in shell

* added a generic DeadResource error and...
...added a result to xdg with_pending_state
Renamed the ToplevelState to ToplevelStateSet
---
 anvil/src/shell.rs                       |  222 +++--
 anvil/src/window_map.rs                  |   67 +-
 src/utils/mod.rs                         |   12 +
 src/utils/rectangle.rs                   |    2 +-
 src/wayland/compositor/roles.rs          |    9 +-
 src/wayland/shell/legacy/mod.rs          |   33 +-
 src/wayland/shell/legacy/wl_handlers.rs  |    6 +-
 src/wayland/shell/mod.rs                 |   15 +
 src/wayland/shell/xdg/mod.rs             | 1104 ++++++++++++++++++----
 src/wayland/shell/xdg/xdg_handlers.rs    |  374 +++++---
 src/wayland/shell/xdg/zxdgv6_handlers.rs |  377 +++++---
 11 files changed, 1690 insertions(+), 531 deletions(-)

diff --git a/anvil/src/shell.rs b/anvil/src/shell.rs
index ad72e92..d94488d 100644
--- a/anvil/src/shell.rs
+++ b/anvil/src/shell.rs
@@ -28,8 +28,8 @@ use smithay::{
                 wl_shell_init, ShellRequest, ShellState as WlShellState, ShellSurfaceKind, ShellSurfaceRole,
             },
             xdg::{
-                xdg_shell_init, PopupConfigure, ShellState as XdgShellState, ToplevelConfigure, XdgRequest,
-                XdgSurfacePendingState, XdgSurfaceRole,
+                xdg_shell_init, Configure, ShellState as XdgShellState, XdgPopupSurfaceRole, XdgRequest,
+                XdgToplevelSurfaceRole,
             },
         },
         Serial,
@@ -38,24 +38,17 @@ use smithay::{
 
 use crate::{
     state::AnvilState,
-    window_map::{Kind as SurfaceKind, WindowMap},
+    window_map::{Kind as SurfaceKind, PopupKind, WindowMap},
 };
 
 #[cfg(feature = "xwayland")]
 use crate::xwayland::X11SurfaceRole;
 
-// The xwayland feature only adds a X11Surface role, but the macro does not support #[cfg]
-#[cfg(not(feature = "xwayland"))]
 define_roles!(Roles =>
-    [ XdgSurface, XdgSurfaceRole ]
-    [ ShellSurface, ShellSurfaceRole]
-    [ DnDIcon, DnDIconRole ]
-    [ CursorImage, CursorImageRole ]
-);
-#[cfg(feature = "xwayland")]
-define_roles!(Roles =>
-    [ XdgSurface, XdgSurfaceRole ]
+    [ XdgToplevelSurface, XdgToplevelSurfaceRole ]
+    [ XdgPopupSurface, XdgPopupSurfaceRole ]
     [ ShellSurface, ShellSurfaceRole]
+    #[cfg(feature = "xwayland")]
     [ X11Surface, X11SurfaceRole ]
     [ DnDIcon, DnDIconRole ]
     [ CursorImage, CursorImageRole ]
@@ -172,7 +165,7 @@ impl PointerGrab for ResizeSurfaceGrab {
         _handle: &mut PointerInnerHandle<'_>,
         location: (f64, f64),
         _focus: Option<(wl_surface::WlSurface, (f64, f64))>,
-        serial: Serial,
+        _serial: Serial,
         _time: u32,
     ) {
         let mut dx = location.0 - self.start_data.location.0;
@@ -226,11 +219,17 @@ impl PointerGrab for ResizeSurfaceGrab {
         self.last_window_size = (new_window_width, new_window_height);
 
         match &self.toplevel {
-            SurfaceKind::Xdg(xdg) => xdg.send_configure(ToplevelConfigure {
-                size: Some(self.last_window_size),
-                states: vec![xdg_toplevel::State::Resizing],
-                serial,
-            }),
+            SurfaceKind::Xdg(xdg) => {
+                if xdg
+                    .with_pending_state(|state| {
+                        state.states.set(xdg_toplevel::State::Resizing);
+                        state.size = Some(self.last_window_size);
+                    })
+                    .is_ok()
+                {
+                    xdg.send_configure();
+                }
+            }
             SurfaceKind::Wl(wl) => wl.send_configure(
                 (self.last_window_size.0 as u32, self.last_window_size.1 as u32),
                 self.edges.into(),
@@ -256,12 +255,15 @@ impl PointerGrab for ResizeSurfaceGrab {
             handle.unset_grab(serial, time);
 
             if let SurfaceKind::Xdg(xdg) = &self.toplevel {
-                // Send the final configure without the resizing state.
-                xdg.send_configure(ToplevelConfigure {
-                    size: Some(self.last_window_size),
-                    states: vec![],
-                    serial,
-                });
+                if xdg
+                    .with_pending_state(|state| {
+                        state.states.unset(xdg_toplevel::State::Resizing);
+                        state.size = Some(self.last_window_size);
+                    })
+                    .is_ok()
+                {
+                    xdg.send_configure();
+                }
 
                 self.ctoken
                     .with_surface_data(self.toplevel.get_surface().unwrap(), |attrs| {
@@ -348,20 +350,19 @@ pub fn init_shell<Backend: 'static>(display: &mut Display, log: ::slog::Logger)
                 let mut rng = rand::thread_rng();
                 let x = range.sample(&mut rng);
                 let y = range.sample(&mut rng);
-                surface.send_configure(ToplevelConfigure {
-                    size: None,
-                    states: vec![],
-                    serial: Serial::from(42),
-                });
+                // Do not send a configure here, the initial configure
+                // of a xdg_surface has to be sent during the commit if
+                // the surface is not already configured
                 xdg_window_map
                     .borrow_mut()
                     .insert(SurfaceKind::Xdg(surface), (x, y));
             }
-            XdgRequest::NewPopup { surface } => surface.send_configure(PopupConfigure {
-                size: (10, 10),
-                position: (10, 10),
-                serial: Serial::from(42),
-            }),
+            XdgRequest::NewPopup { surface } => {
+                // Do not send a configure here, the initial configure
+                // of a xdg_surface has to be sent during the commit if
+                // the surface is not already configured
+                xdg_window_map.borrow_mut().insert_popup(PopupKind::Xdg(surface));
+            }
             XdgRequest::Move {
                 surface,
                 seat,
@@ -462,38 +463,95 @@ pub fn init_shell<Backend: 'static>(display: &mut Display, log: ::slog::Logger)
 
                 pointer.set_grab(grab, serial);
             }
-            XdgRequest::AckConfigure { surface, .. } => {
-                let waiting_for_serial = compositor_token.with_surface_data(&surface, |attrs| {
-                    if let Some(data) = attrs.user_data.get::<RefCell<SurfaceData>>() {
-                        if let ResizeState::WaitingForFinalAck(_, serial) = data.borrow().resize_state {
-                            return Some(serial);
+            XdgRequest::AckConfigure {
+                surface, configure, ..
+            } => {
+                if let Configure::Toplevel(configure) = configure {
+                    let waiting_for_serial = compositor_token.with_surface_data(&surface, |attrs| {
+                        if let Some(data) = attrs.user_data.get::<RefCell<SurfaceData>>() {
+                            if let ResizeState::WaitingForFinalAck(_, serial) = data.borrow().resize_state {
+                                return Some(serial);
+                            }
+                        }
+
+                        None
+                    });
+
+                    if let Some(serial) = waiting_for_serial {
+                        if configure.serial > serial {
+                            // TODO: huh, we have missed the serial somehow.
+                            // this should not happen, but it may be better to handle
+                            // this case anyway
+                        }
+
+                        if serial == configure.serial {
+                            if configure.state.states.contains(xdg_toplevel::State::Resizing) {
+                                compositor_token.with_surface_data(&surface, |attrs| {
+                                    let mut data = attrs
+                                        .user_data
+                                        .get::<RefCell<SurfaceData>>()
+                                        .unwrap()
+                                        .borrow_mut();
+                                    if let ResizeState::WaitingForFinalAck(resize_data, _) = data.resize_state
+                                    {
+                                        data.resize_state = ResizeState::WaitingForCommit(resize_data);
+                                    } else {
+                                        unreachable!()
+                                    }
+                                })
+                            }
                         }
                     }
-
-                    None
-                });
-
-                if let Some(serial) = waiting_for_serial {
-                    let acked = compositor_token
-                        .with_role_data(&surface, |role: &mut XdgSurfaceRole| {
-                            !role.pending_configures.contains(&serial.into())
-                        })
-                        .unwrap();
-
-                    if acked {
-                        compositor_token.with_surface_data(&surface, |attrs| {
-                            let mut data = attrs
-                                .user_data
-                                .get::<RefCell<SurfaceData>>()
-                                .unwrap()
-                                .borrow_mut();
-                            if let ResizeState::WaitingForFinalAck(resize_data, _) = data.resize_state {
-                                data.resize_state = ResizeState::WaitingForCommit(resize_data);
-                            } else {
-                                unreachable!()
-                            }
-                        })
-                    }
+                }
+            }
+            XdgRequest::Fullscreen { surface, output, .. } => {
+                if surface
+                    .with_pending_state(|state| {
+                        // TODO: Use size of current output the window is on and set position to (0,0)
+                        state.states.set(xdg_toplevel::State::Fullscreen);
+                        state.size = Some((800, 600));
+                        // TODO: If the provided output is None, use the output where
+                        // the toplevel is currently shown
+                        state.fullscreen_output = output;
+                    })
+                    .is_ok()
+                {
+                    surface.send_configure();
+                }
+            }
+            XdgRequest::UnFullscreen { surface } => {
+                if surface
+                    .with_pending_state(|state| {
+                        state.states.unset(xdg_toplevel::State::Fullscreen);
+                        state.size = None;
+                        state.fullscreen_output = None;
+                    })
+                    .is_ok()
+                {
+                    surface.send_configure();
+                }
+            }
+            XdgRequest::Maximize { surface } => {
+                if surface
+                    .with_pending_state(|state| {
+                        // TODO: Use size of current output the window is on and set position to (0,0)
+                        state.states.set(xdg_toplevel::State::Maximized);
+                        state.size = Some((800, 600));
+                    })
+                    .is_ok()
+                {
+                    surface.send_configure();
+                }
+            }
+            XdgRequest::UnMaximize { surface } => {
+                if surface
+                    .with_pending_state(|state| {
+                        state.states.unset(xdg_toplevel::State::Maximized);
+                        state.size = None;
+                    })
+                    .is_ok()
+                {
+                    surface.send_configure();
                 }
             }
             _ => (),
@@ -790,14 +848,36 @@ fn surface_commit(
     let mut geometry = None;
     let mut min_size = (0, 0);
     let mut max_size = (0, 0);
-    let _ = token.with_role_data(surface, |role: &mut XdgSurfaceRole| {
-        if let XdgSurfacePendingState::Toplevel(ref state) = role.pending_state {
-            min_size = state.min_size;
-            max_size = state.max_size;
+
+    if token.has_role::<XdgToplevelSurfaceRole>(surface) {
+        if let Some(SurfaceKind::Xdg(xdg)) = window_map.borrow().find(surface) {
+            xdg.commit();
         }
 
-        geometry = role.window_geometry;
-    });
+        token
+            .with_role_data(surface, |role: &mut XdgToplevelSurfaceRole| {
+                if let XdgToplevelSurfaceRole::Some(attributes) = role {
+                    geometry = attributes.window_geometry;
+                    min_size = attributes.min_size;
+                    max_size = attributes.max_size;
+                }
+            })
+            .unwrap();
+    }
+
+    if token.has_role::<XdgPopupSurfaceRole>(surface) {
+        if let Some(PopupKind::Xdg(xdg)) = window_map.borrow().find_popup(&surface) {
+            xdg.commit();
+        }
+
+        token
+            .with_role_data(surface, |role: &mut XdgPopupSurfaceRole| {
+                if let XdgPopupSurfaceRole::Some(attributes) = role {
+                    geometry = attributes.window_geometry;
+                }
+            })
+            .unwrap();
+    }
 
     let sub_data = token
         .with_role_data(surface, |&mut role: &mut SubsurfaceRole| role)
diff --git a/anvil/src/window_map.rs b/anvil/src/window_map.rs
index 2eec66f..1874ba4 100644
--- a/anvil/src/window_map.rs
+++ b/anvil/src/window_map.rs
@@ -7,7 +7,7 @@ use smithay::{
         compositor::{roles::Role, CompositorToken, SubsurfaceRole, TraversalAction},
         shell::{
             legacy::{ShellSurface, ShellSurfaceRole},
-            xdg::{ToplevelSurface, XdgSurfaceRole},
+            xdg::{PopupSurface, ToplevelSurface, XdgPopupSurfaceRole, XdgToplevelSurfaceRole},
         },
     },
 };
@@ -37,7 +37,7 @@ impl<R> Clone for Kind<R> {
 
 impl<R> Kind<R>
 where
-    R: Role<SubsurfaceRole> + Role<XdgSurfaceRole> + Role<ShellSurfaceRole> + 'static,
+    R: Role<SubsurfaceRole> + Role<XdgToplevelSurfaceRole> + Role<ShellSurfaceRole> + 'static,
 {
     pub fn alive(&self) -> bool {
         match *self {
@@ -68,6 +68,35 @@ where
     }
 }
 
+pub enum PopupKind<R> {
+    Xdg(PopupSurface<R>),
+}
+
+// We implement Clone manually because #[derive(..)] would require R: Clone.
+impl<R> Clone for PopupKind<R> {
+    fn clone(&self) -> Self {
+        match self {
+            PopupKind::Xdg(xdg) => PopupKind::Xdg(xdg.clone()),
+        }
+    }
+}
+
+impl<R> PopupKind<R>
+where
+    R: Role<XdgPopupSurfaceRole> + 'static,
+{
+    pub fn alive(&self) -> bool {
+        match *self {
+            PopupKind::Xdg(ref t) => t.alive(),
+        }
+    }
+    pub fn get_surface(&self) -> Option<&wl_surface::WlSurface> {
+        match *self {
+            PopupKind::Xdg(ref t) => t.get_surface(),
+        }
+    }
+}
+
 struct Window<R> {
     location: (i32, i32),
     /// A bounding box over this window and its children.
@@ -80,7 +109,7 @@ struct Window<R> {
 
 impl<R> Window<R>
 where
-    R: Role<SubsurfaceRole> + Role<XdgSurfaceRole> + Role<ShellSurfaceRole> + 'static,
+    R: Role<SubsurfaceRole> + Role<XdgToplevelSurfaceRole> + Role<ShellSurfaceRole> + 'static,
 {
     /// Finds the topmost surface under this point if any and returns it together with the location of this
     /// surface.
@@ -203,19 +232,29 @@ where
     }
 }
 
+pub struct Popup<R> {
+    popup: PopupKind<R>,
+}
+
 pub struct WindowMap<R> {
     ctoken: CompositorToken<R>,
     windows: Vec<Window<R>>,
+    popups: Vec<Popup<R>>,
 }
 
 impl<R> WindowMap<R>
 where
-    R: Role<SubsurfaceRole> + Role<XdgSurfaceRole> + Role<ShellSurfaceRole> + 'static,
+    R: Role<SubsurfaceRole>
+        + Role<XdgToplevelSurfaceRole>
+        + Role<XdgPopupSurfaceRole>
+        + Role<ShellSurfaceRole>
+        + 'static,
 {
     pub fn new(ctoken: CompositorToken<R>) -> Self {
         WindowMap {
             ctoken,
             windows: Vec::new(),
+            popups: Vec::new(),
         }
     }
 
@@ -229,6 +268,11 @@ where
         self.windows.insert(0, window);
     }
 
+    pub fn insert_popup(&mut self, popup: PopupKind<R>) {
+        let popup = Popup { popup };
+        self.popups.push(popup);
+    }
+
     pub fn get_surface_under(&self, point: (f64, f64)) -> Option<(wl_surface::WlSurface, (f64, f64))> {
         for w in &self.windows {
             if let Some(surface) = w.matching(point, self.ctoken) {
@@ -269,6 +313,7 @@ where
 
     pub fn refresh(&mut self) {
         self.windows.retain(|w| w.toplevel.alive());
+        self.popups.retain(|p| p.popup.alive());
         for w in &mut self.windows {
             w.self_update(self.ctoken);
         }
@@ -300,6 +345,20 @@ where
         })
     }
 
+    pub fn find_popup(&self, surface: &wl_surface::WlSurface) -> Option<PopupKind<R>> {
+        self.popups.iter().find_map(|p| {
+            if p.popup
+                .get_surface()
+                .map(|s| s.as_ref().equals(surface.as_ref()))
+                .unwrap_or(false)
+            {
+                Some(p.popup.clone())
+            } else {
+                None
+            }
+        })
+    }
+
     /// Returns the location of the toplevel, if it exists.
     pub fn location(&self, toplevel: &Kind<R>) -> Option<(i32, i32)> {
         self.windows
diff --git a/src/utils/mod.rs b/src/utils/mod.rs
index ae96d5f..0704b51 100644
--- a/src/utils/mod.rs
+++ b/src/utils/mod.rs
@@ -15,3 +15,15 @@ impl std::fmt::Display for UnmanagedResource {
 }
 
 impl std::error::Error for UnmanagedResource {}
+
+/// This resource has been destroyed and can no longer be used.
+#[derive(Debug)]
+pub struct DeadResource;
+
+impl std::fmt::Display for DeadResource {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str("This resource has been destroyed and can no longer be used.")
+    }
+}
+
+impl std::error::Error for DeadResource {}
diff --git a/src/utils/rectangle.rs b/src/utils/rectangle.rs
index febfb10..6ae2c7e 100644
--- a/src/utils/rectangle.rs
+++ b/src/utils/rectangle.rs
@@ -1,5 +1,5 @@
 /// A rectangle defined by its top-left corner and dimensions
-#[derive(Copy, Clone, Debug, Default)]
+#[derive(Copy, Clone, Debug, Default, PartialEq)]
 pub struct Rectangle {
     /// horizontal position of the top-left corner of the rectangle, in surface coordinates
     pub x: i32,
diff --git a/src/wayland/compositor/roles.rs b/src/wayland/compositor/roles.rs
index 990be47..038aedf 100644
--- a/src/wayland/compositor/roles.rs
+++ b/src/wayland/compositor/roles.rs
@@ -186,17 +186,17 @@ macro_rules! define_roles(
     ($enum_name: ident) => {
         define_roles!($enum_name =>);
     };
-    ($enum_name:ident => $([ $role_name: ident, $role_data: ty])*) => {
+    ($enum_name:ident => $($(#[$role_attr:meta])* [$role_name: ident, $role_data: ty])*) => {
         define_roles!(__impl $enum_name =>
             // add in subsurface role
             [Subsurface, $crate::wayland::compositor::SubsurfaceRole]
-            $([$role_name, $role_data])*
+            $($(#[$role_attr])* [$role_name, $role_data])*
         );
     };
-    (__impl $enum_name:ident => $([ $role_name: ident, $role_data: ty])*) => {
+    (__impl $enum_name:ident => $($(#[$role_attr:meta])* [$role_name: ident, $role_data: ty])*) => {
         pub enum $enum_name {
             NoRole,
-            $($role_name($role_data)),*
+            $($(#[$role_attr])* $role_name($role_data)),*
         }
 
         impl Default for $enum_name {
@@ -216,6 +216,7 @@ macro_rules! define_roles(
         }
 
         $(
+            $(#[$role_attr])*
             impl $crate::wayland::compositor::roles::Role<$role_data> for $enum_name {
                 fn set_with(&mut self, data: $role_data) -> ::std::result::Result<(), $role_data> {
                     if let $enum_name::NoRole = *self {
diff --git a/src/wayland/shell/legacy/mod.rs b/src/wayland/shell/legacy/mod.rs
index 20c89ae..1b5d71e 100644
--- a/src/wayland/shell/legacy/mod.rs
+++ b/src/wayland/shell/legacy/mod.rs
@@ -74,6 +74,8 @@ use wayland_server::{
     Display, Filter, Global,
 };
 
+use super::PingError;
+
 mod wl_handlers;
 
 /// Metadata associated with the `wl_surface` role
@@ -83,7 +85,7 @@ pub struct ShellSurfaceRole {
     pub title: String,
     /// Class of the surface
     pub class: String,
-    pending_ping: Serial,
+    pending_ping: Option<Serial>,
 }
 
 /// A handle to a shell surface
@@ -139,24 +141,21 @@ where
     /// down to 0 before a pong is received, mark the client as unresponsive.
     ///
     /// Fails if this shell client already has a pending ping or is already dead.
-    pub fn send_ping(&self, serial: Serial) -> Result<(), ()> {
+    pub fn send_ping(&self, serial: Serial) -> Result<(), PingError> {
         if !self.alive() {
-            return Err(());
-        }
-        let ret = self.token.with_role_data(&self.wl_surface, |data| {
-            if data.pending_ping == Serial::from(0) {
-                data.pending_ping = serial;
-                true
-            } else {
-                false
-            }
-        });
-        if let Ok(true) = ret {
-            self.shell_surface.ping(serial.into());
-            Ok(())
-        } else {
-            Err(())
+            return Err(PingError::DeadSurface);
         }
+        self.token
+            .with_role_data(&self.wl_surface, |data| {
+                if let Some(pending_ping) = data.pending_ping {
+                    return Err(PingError::PingAlreadyPending(pending_ping));
+                }
+                data.pending_ping = Some(serial);
+                Ok(())
+            })
+            .unwrap()?;
+        self.shell_surface.ping(serial.into());
+        Ok(())
     }
 
     /// Send a configure event to this toplevel surface to suggest it a new configuration
diff --git a/src/wayland/shell/legacy/wl_handlers.rs b/src/wayland/shell/legacy/wl_handlers.rs
index 02751b6..29919bf 100644
--- a/src/wayland/shell/legacy/wl_handlers.rs
+++ b/src/wayland/shell/legacy/wl_handlers.rs
@@ -32,7 +32,7 @@ pub(crate) fn implement_shell<R, Impl>(
         let role_data = ShellSurfaceRole {
             title: "".into(),
             class: "".into(),
-            pending_ping: Serial::from(0),
+            pending_ping: None,
         };
         if ctoken.give_role_with(&surface, role_data).is_err() {
             shell
@@ -102,8 +102,8 @@ where
                 let serial = Serial::from(serial);
                 let valid = ctoken
                     .with_role_data(&data.surface, |data| {
-                        if data.pending_ping == serial {
-                            data.pending_ping = Serial::from(0);
+                        if data.pending_ping == Some(serial) {
+                            data.pending_ping = None;
                             true
                         } else {
                             false
diff --git a/src/wayland/shell/mod.rs b/src/wayland/shell/mod.rs
index 114614e..4db509d 100644
--- a/src/wayland/shell/mod.rs
+++ b/src/wayland/shell/mod.rs
@@ -14,5 +14,20 @@
 //! - The [`legacy`](legacy/index.html) module provides handlers for the `wl_shell` protocol, which
 //!   is now deprecated. You only need it if you want to support apps predating `xdg_shell`.
 
+use super::Serial;
+use thiserror::Error;
+
 pub mod legacy;
 pub mod xdg;
+
+/// Represents the possible errors returned from
+/// a surface ping
+#[derive(Debug, Error)]
+pub enum PingError {
+    /// The operation failed because the underlying surface has been destroyed
+    #[error("the ping failed cause the underlying surface has been destroyed")]
+    DeadSurface,
+    /// There is already a pending ping
+    #[error("there is already a ping pending `{0:?}`")]
+    PingAlreadyPending(Serial),
+}
diff --git a/src/wayland/shell/xdg/mod.rs b/src/wayland/shell/xdg/mod.rs
index f66232b..f540f06 100644
--- a/src/wayland/shell/xdg/mod.rs
+++ b/src/wayland/shell/xdg/mod.rs
@@ -32,13 +32,14 @@
 //! #
 //! use smithay::wayland::compositor::roles::*;
 //! use smithay::wayland::compositor::CompositorToken;
-//! use smithay::wayland::shell::xdg::{xdg_shell_init, XdgSurfaceRole, XdgRequest};
+//! use smithay::wayland::shell::xdg::{xdg_shell_init, XdgToplevelSurfaceRole, XdgPopupSurfaceRole, XdgRequest};
 //! use wayland_protocols::unstable::xdg_shell::v6::server::zxdg_shell_v6::ZxdgShellV6;
 //! # use wayland_server::protocol::{wl_seat, wl_output};
 //!
 //! // define the roles type. You need to integrate the XdgSurface role:
 //! define_roles!(MyRoles =>
-//!     [XdgSurface, XdgSurfaceRole]
+//!     [XdgToplevelSurface, XdgToplevelSurfaceRole]
+//!     [XdgPopupSurface, XdgPopupSurfaceRole]
 //! );
 //!
 //! // define the metadata you want associated with the shell clients
@@ -87,56 +88,329 @@
 //! that you are given (in an `Arc<Mutex<_>>`) as return value of the `init` function.
 
 use crate::utils::Rectangle;
+use crate::wayland::compositor::roles::WrongRole;
 use crate::wayland::compositor::{roles::Role, CompositorToken};
-use crate::wayland::Serial;
+use crate::wayland::{Serial, SERIAL_COUNTER};
+use std::fmt::Debug;
 use std::{
     cell::RefCell,
     rc::Rc,
     sync::{Arc, Mutex},
 };
+use thiserror::Error;
+use wayland_protocols::unstable::xdg_shell::v6::server::zxdg_surface_v6;
+use wayland_protocols::xdg_shell::server::xdg_surface;
 use wayland_protocols::{
-    unstable::xdg_shell::v6::server::{zxdg_popup_v6, zxdg_shell_v6, zxdg_surface_v6, zxdg_toplevel_v6},
-    xdg_shell::server::{xdg_popup, xdg_positioner, xdg_surface, xdg_toplevel, xdg_wm_base},
+    unstable::xdg_shell::v6::server::{zxdg_popup_v6, zxdg_shell_v6, zxdg_toplevel_v6},
+    xdg_shell::server::{xdg_popup, xdg_positioner, xdg_toplevel, xdg_wm_base},
 };
 use wayland_server::{
     protocol::{wl_output, wl_seat, wl_surface},
     Display, Filter, Global, UserDataMap,
 };
 
+use super::PingError;
+
 // handlers for the xdg_shell protocol
 mod xdg_handlers;
 // compatibility handlers for the zxdg_shell_v6 protocol, its earlier version
 mod zxdgv6_handlers;
 
-/// Metadata associated with the `xdg_surface` role
-#[derive(Debug)]
-pub struct XdgSurfaceRole {
-    /// Pending state as requested by the client
+macro_rules! xdg_role {
+    ($(#[$role_attr:meta])* $role:ident,
+     $configure:ty,
+     $(#[$attr:meta])* $element:ident {$($(#[$field_attr:meta])* $vis:vis$field:ident:$type:ty),*},
+     $role_ack_configure:expr) => {
+        $(#[$role_attr])*
+        pub type $role = Option<$element>;
+
+        $(#[$attr])*
+        pub struct $element {
+                /// Defines the current window geometry of the surface if any.
+                pub window_geometry: Option<Rectangle>,
+
+                /// Holds the double-buffered geometry that may be specified
+                /// by xdg_surface.set_window_geometry. This should be moved to the
+                /// current configuration on a commit of the underlying
+                /// wl_surface.
+                pub client_pending_window_geometry: Option<Rectangle>,
+
+                /// Defines if the surface has received at least one
+                /// xdg_surface.ack_configure from the client
+                pub configured: bool,
+
+                /// The serial of the last acked configure
+                pub configure_serial: Option<Serial>,
+
+                /// Holds the state if the surface has sent the initial
+                /// configure event to the client. It is expected that
+                /// during the first commit a initial
+                /// configure event is sent to the client
+                pub initial_configure_sent: bool,
+
+                /// Holds the configures the server has sent out
+                /// to the client waiting to be acknowledged by
+                /// the client. All pending configures that are older
+                /// than the acknowledged one will be discarded during
+                /// processing xdg_surface.ack_configure.
+                pending_configures: Vec<$configure>,
+
+                $(
+                    $(#[$field_attr])*
+                    $vis $field: $type,
+                )*
+        }
+
+        impl XdgSurface for $element {
+            fn set_window_geometry(&mut self, geometry: Rectangle) {
+                self.client_pending_window_geometry = Some(geometry);
+            }
+
+            fn ack_configure(&mut self, serial: Serial) -> Result<Configure, ConfigureError> {
+                let configure = match self
+                    .pending_configures
+                    .iter()
+                    .find(|configure| configure.serial == serial)
+                {
+                    Some(configure) => (*configure).clone(),
+                    None => {
+                        return Err(ConfigureError::SerialNotFound(serial));
+                    }
+                };
+
+                // Role specific ack_configure
+                let role_ack_configure: &dyn Fn(&mut Self, $configure) = &$role_ack_configure;
+                role_ack_configure(self, configure.clone());
+
+                // Set the xdg_surface to configured
+                self.configured = true;
+
+                // Save the last configure serial as a reference
+                self.configure_serial = Some(Serial::from(serial));
+
+                // Clean old configures
+                self.pending_configures.retain(|c| c.serial > serial);
+
+                Ok(configure.into())
+            }
+        }
+
+        impl Default for $element {
+            fn default() -> Self {
+                Self {
+                    window_geometry: None,
+                    client_pending_window_geometry: None,
+                    configured: false,
+                    configure_serial: None,
+                    pending_configures: Vec::new(),
+                    initial_configure_sent: false,
+
+                    $(
+                        $field: Default::default(),
+                    )*
+                }
+            }
+        }
+    };
+}
+
+impl<R> CompositorToken<R>
+where
+    R: Role<XdgToplevelSurfaceRole> + Role<XdgPopupSurfaceRole> + 'static,
+{
+    fn with_xdg_role<F, T>(&self, surface: &wl_surface::WlSurface, f: F) -> Result<T, WrongRole>
+    where
+        F: FnOnce(&mut dyn XdgSurface) -> T,
+    {
+        if self.has_role::<XdgToplevelSurfaceRole>(surface) {
+            self.with_role_data(surface, |role: &mut XdgToplevelSurfaceRole| f(role))
+        } else {
+            self.with_role_data(surface, |role: &mut XdgPopupSurfaceRole| f(role))
+        }
+    }
+}
+
+trait XdgSurface {
+    fn set_window_geometry(&mut self, geometry: Rectangle);
+
+    fn ack_configure(&mut self, serial: Serial) -> Result<Configure, ConfigureError>;
+}
+
+#[derive(Debug, Error)]
+enum ConfigureError {
+    #[error("the serial `{0:?}` was not found in the pending configure list")]
+    SerialNotFound(Serial),
+}
+
+impl<T> XdgSurface for Option<T>
+where
+    T: XdgSurface,
+{
+    fn set_window_geometry(&mut self, geometry: Rectangle) {
+        self.as_mut()
+            .expect("xdg_surface exists but role has been destroyed?!")
+            .set_window_geometry(geometry)
+    }
+
+    fn ack_configure(&mut self, serial: Serial) -> Result<Configure, ConfigureError> {
+        self.as_mut()
+            .expect("xdg_surface exists but role has been destroyed?!")
+            .ack_configure(serial)
+    }
+}
+
+xdg_role!(
+    /// This interface defines an xdg_surface role which allows a surface to,
+    /// among other things, set window-like properties such as maximize,
+    /// fullscreen, and minimize, set application-specific metadata like title and
+    /// id, and well as trigger user interactive operations such as interactive
+    /// resize and move.
     ///
-    /// The data in this field are double-buffered, you should
-    /// apply them on a surface commit.
-    pub pending_state: XdgSurfacePendingState,
-    /// Geometry of the surface
+    /// Unmapping an xdg_toplevel means that the surface cannot be shown
+    /// by the compositor until it is explicitly mapped again.
+    /// All active operations (e.g., move, resize) are canceled and all
+    /// attributes (e.g. title, state, stacking, ...) are discarded for
+    /// an xdg_toplevel surface when it is unmapped. The xdg_toplevel returns to
+    /// the state it had right after xdg_surface.get_toplevel. The client
+    /// can re-map the toplevel by perfoming a commit without any buffer
+    /// attached, waiting for a configure event and handling it as usual (see
+    /// xdg_surface description).
     ///
-    /// Defines, in surface relative coordinates, what should
-    /// be considered as "the surface itself", regarding focus,
-    /// window alignment, etc...
+    /// Attaching a null buffer to a toplevel unmaps the surface.
+    XdgToplevelSurfaceRole,
+    ToplevelConfigure,
+    /// Role specific attributes for xdg_toplevel
+    XdgToplevelSurfaceRoleAttributes {
+        /// The parent field of a toplevel should be used
+        /// by the compositor to determine which toplevel
+        /// should be brought to front. If the parent is focused
+        /// all of it's child should be brought to front.
+        pub parent: Option<wl_surface::WlSurface>,
+        /// Holds the optional title the client has set for
+        /// this toplevel. For example a web-browser will most likely
+        /// set this to include the current uri.
+        ///
+        /// This string may be used to identify the surface in a task bar,
+        /// window list, or other user interface elements provided by the
+        /// compositor.
+        pub title: Option<String>,
+        /// Holds the optional app ID the client has set for
+        /// this toplevel.
+        ///
+        /// The app ID identifies the general class of applications to which
+        /// the surface belongs. The compositor can use this to group multiple
+        /// surfaces together, or to determine how to launch a new application.
+        ///
+        /// For D-Bus activatable applications, the app ID is used as the D-Bus
+        /// service name.
+        pub app_id: Option<String>,
+        /// Minimum size requested for this surface
+        ///
+        /// A value of 0 on an axis means this axis is not constrained
+        pub min_size: (i32, i32),
+        /// Maximum size requested for this surface
+        ///
+        /// A value of 0 on an axis means this axis is not constrained
+        pub max_size: (i32, i32),
+        /// Holds the pending state as set by the client.
+        pub client_pending: Option<ToplevelClientPending>,
+        /// Holds the pending state as set by the server.
+        pub server_pending: Option<ToplevelState>,
+        /// Holds the last server_pending state that has been acknowledged
+        /// by the client. This state should be cloned to the current
+        /// during a commit.
+        pub last_acked: Option<ToplevelState>,
+        /// Holds the current state of the toplevel after a successful
+        /// commit.
+        pub current: ToplevelState
+    },
+    |attributes, configure| {
+        attributes.last_acked = Some(configure.state);
+    }
+);
+
+xdg_role!(
+    /// A popup surface is a short-lived, temporary surface. It can be used to
+    /// implement for example menus, popovers, tooltips and other similar user
+    /// interface concepts.
     ///
-    /// By default, you should consider the full contents of the
-    /// buffers of this surface and its subsurfaces.
-    pub window_geometry: Option<Rectangle>,
-    /// List of non-acked configures pending
+    /// A popup can be made to take an explicit grab. See xdg_popup.grab for
+    /// details.
     ///
-    /// Whenever a configure is acked by the client, all configure
-    /// older than it are discarded as well. As such, this `Vec` contains
-    /// the serials of all the configure send to this surface that are
-    /// newer than the last ack received.
-    pub pending_configures: Vec<u32>,
-    /// Has this surface acked at least one configure?
+    /// When the popup is dismissed, a popup_done event will be sent out, and at
+    /// the same time the surface will be unmapped. See the xdg_popup.popup_done
+    /// event for details.
     ///
-    /// `xdg_shell` defines it as illegal to commit on a surface that has
-    /// not yet acked a configure.
-    pub configured: bool,
+    /// Explicitly destroying the xdg_popup object will also dismiss the popup and
+    /// unmap the surface. Clients that want to dismiss the popup when another
+    /// surface of their own is clicked should dismiss the popup using the destroy
+    /// request.
+    ///
+    /// A newly created xdg_popup will be stacked on top of all previously created
+    /// xdg_popup surfaces associated with the same xdg_toplevel.
+    ///
+    /// The parent of an xdg_popup must be mapped (see the xdg_surface
+    /// description) before the xdg_popup itself.
+    ///
+    /// The client must call wl_surface.commit on the corresponding wl_surface
+    /// for the xdg_popup state to take effect.
+    XdgPopupSurfaceRole,
+    PopupConfigure,
+    /// Popup Surface Role
+    XdgPopupSurfaceRoleAttributes {
+        /// Holds the parent for the xdg_popup.
+        ///
+        /// The parent is allowed to remain unset as long
+        /// as no commit has been requested for the underlying
+        /// wl_surface. The parent can either be directly
+        /// specified during xdg_surface.get_popup or using
+        /// another protocol extension, for example xdg_layer_shell.
+        ///
+        /// It is a protocol error to call commit on a wl_surface with
+        /// the xdg_popup role when no parent is set.
+        pub parent: Option<wl_surface::WlSurface>,
+        /// The positioner state can be used by the compositor
+        /// to calculate the best placement for the popup.
+        ///
+        /// For example the compositor should prevent that a popup
+        /// is placed outside the visible rectangle of a output.
+        pub positioner: PositionerState,
+        /// Holds the last server_pending state that has been acknowledged
+        /// by the client. This state should be cloned to the current
+        /// during a commit.
+        pub last_acked: Option<PopupState>,
+        /// Holds the current state of the popup after a successful
+        /// commit.
+        pub current: PopupState,
+        /// Holds the pending state as set by the server.
+        pub server_pending: Option<PopupState>
+    },
+    |attributes,configure| {
+        attributes.last_acked = Some(configure.state);
+    }
+);
+
+/// Represents the state of the popup
+#[derive(Debug, Clone, Copy, PartialEq)]
+pub struct PopupState {
+    /// Holds the geometry of the popup as defined by the positioner.
+    ///
+    /// `Rectangle::width` and `Rectangle::height` holds the size of the
+    /// of the popup in surface-local coordinates and corresponds to the
+    /// window geometry
+    ///
+    /// `Rectangle::x` and `Rectangle::y` holds the position of the popup
+    /// The position is relative to the window geometry as defined by
+    /// xdg_surface.set_window_geometry of the parent surface.
+    pub geometry: Rectangle,
+}
+
+impl Default for PopupState {
+    fn default() -> Self {
+        Self {
+            geometry: Default::default(),
+        }
+    }
 }
 
 #[derive(Clone, Debug)]
@@ -159,6 +433,19 @@ pub struct PositionerState {
     pub offset: (i32, i32),
 }
 
+impl Default for PositionerState {
+    fn default() -> Self {
+        PositionerState {
+            anchor_edges: xdg_positioner::Anchor::None,
+            anchor_rect: Default::default(),
+            constraint_adjustment: xdg_positioner::ConstraintAdjustment::empty(),
+            gravity: xdg_positioner::Gravity::None,
+            offset: (0, 0),
+            rect_size: (0, 0),
+        }
+    }
+}
+
 impl PositionerState {
     pub(crate) fn new() -> PositionerState {
         PositionerState {
@@ -175,91 +462,270 @@ impl PositionerState {
             offset: (0, 0),
         }
     }
-}
 
-/// Contents of the pending state of a shell surface, depending on its role
-#[derive(Debug)]
-pub enum XdgSurfacePendingState {
-    /// This a regular, toplevel surface
+    pub(crate) fn anchor_has_edge(&self, edge: xdg_positioner::Anchor) -> bool {
+        match edge {
+            xdg_positioner::Anchor::Top => {
+                self.anchor_edges == xdg_positioner::Anchor::Top
+                    || self.anchor_edges == xdg_positioner::Anchor::TopLeft
+                    || self.anchor_edges == xdg_positioner::Anchor::TopRight
+            }
+            xdg_positioner::Anchor::Bottom => {
+                self.anchor_edges == xdg_positioner::Anchor::Bottom
+                    || self.anchor_edges == xdg_positioner::Anchor::BottomLeft
+                    || self.anchor_edges == xdg_positioner::Anchor::BottomRight
+            }
+            xdg_positioner::Anchor::Left => {
+                self.anchor_edges == xdg_positioner::Anchor::Left
+                    || self.anchor_edges == xdg_positioner::Anchor::TopLeft
+                    || self.anchor_edges == xdg_positioner::Anchor::BottomLeft
+            }
+            xdg_positioner::Anchor::Right => {
+                self.anchor_edges == xdg_positioner::Anchor::Right
+                    || self.anchor_edges == xdg_positioner::Anchor::TopRight
+                    || self.anchor_edges == xdg_positioner::Anchor::BottomRight
+            }
+            _ => unreachable!(),
+        }
+    }
+
+    pub(crate) fn gravity_has_edge(&self, edge: xdg_positioner::Gravity) -> bool {
+        match edge {
+            xdg_positioner::Gravity::Top => {
+                self.gravity == xdg_positioner::Gravity::Top
+                    || self.gravity == xdg_positioner::Gravity::TopLeft
+                    || self.gravity == xdg_positioner::Gravity::TopRight
+            }
+            xdg_positioner::Gravity::Bottom => {
+                self.gravity == xdg_positioner::Gravity::Bottom
+                    || self.gravity == xdg_positioner::Gravity::BottomLeft
+                    || self.gravity == xdg_positioner::Gravity::BottomRight
+            }
+            xdg_positioner::Gravity::Left => {
+                self.gravity == xdg_positioner::Gravity::Left
+                    || self.gravity == xdg_positioner::Gravity::TopLeft
+                    || self.gravity == xdg_positioner::Gravity::BottomLeft
+            }
+            xdg_positioner::Gravity::Right => {
+                self.gravity == xdg_positioner::Gravity::Right
+                    || self.gravity == xdg_positioner::Gravity::TopRight
+                    || self.gravity == xdg_positioner::Gravity::BottomRight
+            }
+            _ => unreachable!(),
+        }
+    }
+
+    /// Get the geometry for a popup as defined by this positioner.
     ///
-    /// This corresponds to the `xdg_toplevel` role
+    /// `Rectangle::width` and `Rectangle::height` corresponds to the
+    /// size set by `xdg_positioner.set_size`.
     ///
-    /// This is what you'll generally interpret as "a window".
-    Toplevel(ToplevelState),
-    /// This is a popup surface
-    ///
-    /// This corresponds to the `xdg_popup` role
-    ///
-    /// This are mostly for small tooltips and similar short-lived
-    /// surfaces.
-    Popup(PopupState),
-    /// This surface was not yet assigned a kind
-    None,
+    /// `Rectangle::x` and `Rectangle::y` define the position of the
+    /// popup relative to it's parent surface `window_geometry`.
+    /// The position is calculated according to the rules defined
+    /// in the `xdg_shell` protocol.
+    /// The `constraint_adjustment` will not be considered by this
+    /// implementation and the position and size should be re-calculated
+    /// in the compositor if the compositor implements `constraint_adjustment`
+    pub(crate) fn get_geometry(&self) -> Rectangle {
+        // From the `xdg_shell` prococol specification:
+        //
+        // set_offset:
+        //
+        //  Specify the surface position offset relative to the position of the
+        //  anchor on the anchor rectangle and the anchor on the surface. For
+        //  example if the anchor of the anchor rectangle is at (x, y), the surface
+        //  has the gravity bottom|right, and the offset is (ox, oy), the calculated
+        //  surface position will be (x + ox, y + oy)
+        let mut geometry = Rectangle {
+            x: self.offset.0,
+            y: self.offset.1,
+            width: self.rect_size.0,
+            height: self.rect_size.1,
+        };
+
+        // Defines the anchor point for the anchor rectangle. The specified anchor
+        // is used derive an anchor point that the child surface will be
+        // positioned relative to. If a corner anchor is set (e.g. 'top_left' or
+        // 'bottom_right'), the anchor point will be at the specified corner;
+        // otherwise, the derived anchor point will be centered on the specified
+        // edge, or in the center of the anchor rectangle if no edge is specified.
+        if self.anchor_has_edge(xdg_positioner::Anchor::Top) {
+            geometry.y += self.anchor_rect.y;
+        } else if self.anchor_has_edge(xdg_positioner::Anchor::Bottom) {
+            geometry.y += self.anchor_rect.y + self.anchor_rect.height;
+        } else {
+            geometry.y += self.anchor_rect.y + self.anchor_rect.height / 2;
+        }
+
+        if self.anchor_has_edge(xdg_positioner::Anchor::Left) {
+            geometry.x += self.anchor_rect.x;
+        } else if self.anchor_has_edge(xdg_positioner::Anchor::Right) {
+            geometry.x += self.anchor_rect.x + self.anchor_rect.width;
+        } else {
+            geometry.x += self.anchor_rect.x + self.anchor_rect.width / 2;
+        }
+
+        // Defines in what direction a surface should be positioned, relative to
+        // the anchor point of the parent surface. If a corner gravity is
+        // specified (e.g. 'bottom_right' or 'top_left'), then the child surface
+        // will be placed towards the specified gravity; otherwise, the child
+        // surface will be centered over the anchor point on any axis that had no
+        // gravity specified.
+        if self.gravity_has_edge(xdg_positioner::Gravity::Top) {
+            geometry.y -= geometry.height;
+        } else if self.gravity_has_edge(xdg_positioner::Gravity::Bottom) {
+            geometry.y -= geometry.height / 2;
+        }
+
+        if self.gravity_has_edge(xdg_positioner::Gravity::Left) {
+            geometry.x -= geometry.width;
+        } else if self.gravity_has_edge(xdg_positioner::Gravity::Right) {
+            geometry.x -= geometry.width / 2;
+        }
+
+        geometry
+    }
 }
 
 /// State of a regular toplevel surface
-#[derive(Debug)]
+#[derive(Debug, PartialEq)]
 pub struct ToplevelState {
-    /// Parent of this surface
-    ///
-    /// If this surface has a parent, it should be hidden
-    /// or displayed, brought up at the same time as it.
-    pub parent: Option<wl_surface::WlSurface>,
-    /// Title of this shell surface
-    pub title: String,
-    /// App id for this shell surface
-    ///
-    /// This identifier can be used to group surface together
-    /// as being several instance of the same app. This can
-    /// also be used as the D-Bus name for the app.
-    pub app_id: String,
-    /// Minimum size requested for this surface
-    ///
-    /// A value of 0 on an axis means this axis is not constrained
-    pub min_size: (i32, i32),
-    /// Maximum size requested for this surface
-    ///
-    /// A value of 0 on an axis means this axis is not constrained
-    pub max_size: (i32, i32),
+    /// The suggested size of the surface
+    pub size: Option<(i32, i32)>,
+
+    /// The states for this surface
+    pub states: ToplevelStateSet,
+
+    /// The output for a fullscreen display
+    pub fullscreen_output: Option<wl_output::WlOutput>,
+}
+
+impl Default for ToplevelState {
+    fn default() -> Self {
+        ToplevelState {
+            fullscreen_output: None,
+            states: Default::default(),
+            size: None,
+        }
+    }
 }
 
 impl Clone for ToplevelState {
     fn clone(&self) -> ToplevelState {
         ToplevelState {
-            parent: self.parent.as_ref().cloned(),
-            title: self.title.clone(),
-            app_id: self.app_id.clone(),
-            min_size: self.min_size,
-            max_size: self.max_size,
+            fullscreen_output: self.fullscreen_output.clone(),
+            states: self.states.clone(),
+            size: self.size,
         }
     }
 }
 
-/// The pending state of a popup surface
-#[derive(Debug)]
-pub struct PopupState {
-    /// Parent of this popup surface
-    pub parent: Option<wl_surface::WlSurface>,
-    /// The positioner specifying how this tooltip should
-    /// be placed relative to its parent.
-    pub positioner: PositionerState,
+/// Container holding the states for a `XdgToplevel`
+///
+/// This container will prevent the `XdgToplevel` from
+/// having the same `xdg_toplevel::State` multiple times
+/// and simplifies setting and un-setting a particularly
+/// `xdg_toplevel::State`
+#[derive(Debug, Clone, PartialEq)]
+pub struct ToplevelStateSet {
+    states: Vec<xdg_toplevel::State>,
 }
 
-impl Clone for PopupState {
-    fn clone(&self) -> PopupState {
-        PopupState {
-            parent: self.parent.as_ref().cloned(),
-            positioner: self.positioner.clone(),
+impl ToplevelStateSet {
+    /// Returns `true` if the states contains a state.
+    pub fn contains(&self, state: xdg_toplevel::State) -> bool {
+        self.states.iter().any(|s| *s == state)
+    }
+
+    /// Adds a state to the states.
+    ///
+    /// If the states did not have this state present, `true` is returned.
+    ///
+    /// If the states did have this state present, `false` is returned.
+    pub fn set(&mut self, state: xdg_toplevel::State) -> bool {
+        if self.contains(state) {
+            false
+        } else {
+            self.states.push(state);
+            true
+        }
+    }
+
+    /// Removes a state from the states. Returns whether the state was
+    /// present in the states.
+    pub fn unset(&mut self, state: xdg_toplevel::State) -> bool {
+        if !self.contains(state) {
+            false
+        } else {
+            self.states.retain(|s| *s != state);
+            true
         }
     }
 }
 
-impl Default for XdgSurfacePendingState {
-    fn default() -> XdgSurfacePendingState {
-        XdgSurfacePendingState::None
+impl Default for ToplevelStateSet {
+    fn default() -> Self {
+        Self { states: Vec::new() }
     }
 }
 
+impl IntoIterator for ToplevelStateSet {
+    type Item = xdg_toplevel::State;
+    type IntoIter = std::vec::IntoIter<Self::Item>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        self.states.into_iter()
+    }
+}
+
+impl From<ToplevelStateSet> for Vec<xdg_toplevel::State> {
+    fn from(states: ToplevelStateSet) -> Self {
+        states.states
+    }
+}
+
+/// Represents the client pending state
+#[derive(Debug, Clone, Copy)]
+pub struct ToplevelClientPending {
+    /// Minimum size requested for this surface
+    ///
+    /// A value of 0 on an axis means this axis is not constrained
+    ///
+    /// A value of `None` represents the client has not defined
+    /// a minimum size
+    pub min_size: Option<(i32, i32)>,
+    /// Maximum size requested for this surface
+    ///
+    /// A value of 0 on an axis means this axis is not constrained
+    ///
+    /// A value of `None` represents the client has not defined
+    /// a maximum size
+    pub max_size: Option<(i32, i32)>,
+}
+
+impl Default for ToplevelClientPending {
+    fn default() -> Self {
+        Self {
+            min_size: None,
+            max_size: None,
+        }
+    }
+}
+
+/// Represents the possible errors
+/// returned from `ToplevelSurface::with_pending_state`
+/// and `PopupSurface::with_pending_state`
+#[derive(Debug, Error)]
+pub enum PendingStateError {
+    /// The operation failed because the underlying surface has been destroyed
+    #[error("could not retrieve the pending state because the underlying surface has been destroyed")]
+    DeadSurface,
+    /// The role of the xdg_surface has been destroyed
+    #[error("the role of the xdg_surface has been destroyed")]
+    RoleDestroyed,
+}
+
 pub(crate) struct ShellData<R> {
     log: ::slog::Logger,
     compositor_token: CompositorToken<R>,
@@ -290,7 +756,7 @@ pub fn xdg_shell_init<R, L, Impl>(
     Global<zxdg_shell_v6::ZxdgShellV6>,
 )
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + Role<XdgPopupSurfaceRole> + 'static,
     L: Into<Option<::slog::Logger>>,
     Impl: FnMut(XdgRequest<R>) + 'static,
 {
@@ -338,7 +804,7 @@ pub struct ShellState<R> {
 
 impl<R> ShellState<R>
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + Role<XdgPopupSurfaceRole> + 'static,
 {
     /// Access all the shell surfaces known by this handler
     pub fn toplevel_surfaces(&self) -> &[ToplevelSurface<R>] {
@@ -362,13 +828,13 @@ enum ShellClientKind {
 }
 
 pub(crate) struct ShellClientData {
-    pending_ping: Serial,
+    pending_ping: Option<Serial>,
     data: UserDataMap,
 }
 
 fn make_shell_client_data() -> ShellClientData {
     ShellClientData {
-        pending_ping: Serial::from(0),
+        pending_ping: None,
         data: UserDataMap::new(),
     }
 }
@@ -391,7 +857,7 @@ pub struct ShellClient<R> {
 
 impl<R> ShellClient<R>
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + Role<XdgPopupSurfaceRole> + 'static,
 {
     /// Is the shell client represented by this handle still connected?
     pub fn alive(&self) -> bool {
@@ -422,9 +888,9 @@ where
     /// down to 0 before a pong is received, mark the client as unresponsive.
     ///
     /// Fails if this shell client already has a pending ping or is already dead.
-    pub fn send_ping(&self, serial: Serial) -> Result<(), ()> {
+    pub fn send_ping(&self, serial: Serial) -> Result<(), PingError> {
         if !self.alive() {
-            return Err(());
+            return Err(PingError::DeadSurface);
         }
         match self.kind {
             ShellClientKind::Xdg(ref shell) => {
@@ -434,10 +900,10 @@ where
                     .get::<self::xdg_handlers::ShellUserData<R>>()
                     .unwrap();
                 let mut guard = user_data.client_data.lock().unwrap();
-                if guard.pending_ping == Serial::from(0) {
-                    return Err(());
+                if let Some(pending_ping) = guard.pending_ping {
+                    return Err(PingError::PingAlreadyPending(pending_ping));
                 }
-                guard.pending_ping = serial;
+                guard.pending_ping = Some(serial);
                 shell.ping(serial.into());
             }
             ShellClientKind::ZxdgV6(ref shell) => {
@@ -447,10 +913,10 @@ where
                     .get::<self::zxdgv6_handlers::ShellUserData<R>>()
                     .unwrap();
                 let mut guard = user_data.client_data.lock().unwrap();
-                if guard.pending_ping == Serial::from(0) {
-                    return Err(());
+                if let Some(pending_ping) = guard.pending_ping {
+                    return Err(PingError::PingAlreadyPending(pending_ping));
                 }
-                guard.pending_ping = serial;
+                guard.pending_ping = Some(serial);
                 shell.ping(serial.into());
             }
         }
@@ -458,12 +924,12 @@ where
     }
 
     /// Access the user data associated with this shell client
-    pub fn with_data<F, T>(&self, f: F) -> Result<T, ()>
+    pub fn with_data<F, T>(&self, f: F) -> Result<T, crate::utils::DeadResource>
     where
         F: FnOnce(&mut UserDataMap) -> T,
     {
         if !self.alive() {
-            return Err(());
+            return Err(crate::utils::DeadResource);
         }
         match self.kind {
             ShellClientKind::Xdg(ref shell) => {
@@ -515,7 +981,7 @@ impl<R> Clone for ToplevelSurface<R> {
 
 impl<R> ToplevelSurface<R>
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + 'static,
 {
     /// Is the toplevel surface referred by this handle still alive?
     pub fn alive(&self) -> bool {
@@ -564,16 +1030,135 @@ where
         })
     }
 
+    /// Gets the current pending state for a configure
+    ///
+    /// Returns `Some` if either no initial configure has been sent or
+    /// the `server_pending` is `Some` and different from the last pending
+    /// configure or `last_acked` if there is no pending
+    ///
+    /// Returns `None` if either no `server_pending` or the pending
+    /// has already been sent to the client or the pending is equal
+    /// to the `last_acked`
+    fn get_pending_state(&self, attributes: &mut XdgToplevelSurfaceRoleAttributes) -> Option<ToplevelState> {
+        if !attributes.initial_configure_sent {
+            return Some(attributes.server_pending.take().unwrap_or_default());
+        }
+
+        let server_pending = match attributes.server_pending.take() {
+            Some(state) => state,
+            None => {
+                return None;
+            }
+        };
+
+        let last_state = attributes
+            .pending_configures
+            .last()
+            .map(|c| &c.state)
+            .or_else(|| attributes.last_acked.as_ref());
+
+        if let Some(state) = last_state {
+            if state == &server_pending {
+                return None;
+            }
+        }
+
+        Some(server_pending)
+    }
+
     /// Send a configure event to this toplevel surface to suggest it a new configuration
     ///
     /// The serial of this configure will be tracked waiting for the client to ACK it.
-    pub fn send_configure(&self, cfg: ToplevelConfigure) {
-        if !self.alive() {
-            return;
+    pub fn send_configure(&self) {
+        if let Some(surface) = self.get_surface() {
+            let configure = self
+                .token
+                .with_role_data(surface, |role| {
+                    if let Some(attributes) = role {
+                        if let Some(pending) = self.get_pending_state(attributes) {
+                            let configure = ToplevelConfigure {
+                                serial: SERIAL_COUNTER.next_serial(),
+                                state: pending,
+                            };
+
+                            attributes.pending_configures.push(configure.clone());
+
+                            Some((configure, !attributes.initial_configure_sent))
+                        } else {
+                            None
+                        }
+                    } else {
+                        None
+                    }
+                })
+                .unwrap_or(None);
+
+            if let Some((configure, initial)) = configure {
+                match self.shell_surface {
+                    ToplevelKind::Xdg(ref s) => {
+                        self::xdg_handlers::send_toplevel_configure::<R>(s, configure)
+                    }
+                    ToplevelKind::ZxdgV6(ref s) => {
+                        self::zxdgv6_handlers::send_toplevel_configure::<R>(s, configure)
+                    }
+                }
+
+                if initial {
+                    self.token
+                        .with_role_data(surface, |role| {
+                            if let XdgToplevelSurfaceRole::Some(attributes) = role {
+                                attributes.initial_configure_sent = true;
+                            }
+                        })
+                        .unwrap();
+                }
+            }
         }
-        match self.shell_surface {
-            ToplevelKind::Xdg(ref s) => self::xdg_handlers::send_toplevel_configure::<R>(s, cfg),
-            ToplevelKind::ZxdgV6(ref s) => self::zxdgv6_handlers::send_toplevel_configure::<R>(s, cfg),
+    }
+
+    /// Handles the role specific commit logic
+    ///
+    /// This should be called when the underlying WlSurface
+    /// handles a wl_surface.commit request.
+    pub fn commit(&self) {
+        if let Some(surface) = self.get_surface() {
+            let send_initial_configure = self
+                .token
+                .with_role_data(surface, |role: &mut XdgToplevelSurfaceRole| {
+                    if let XdgToplevelSurfaceRole::Some(attributes) = role {
+                        if let Some(window_geometry) = attributes.client_pending_window_geometry.take() {
+                            attributes.window_geometry = Some(window_geometry);
+                        }
+
+                        if attributes.initial_configure_sent {
+                            if let Some(state) = attributes.last_acked.clone() {
+                                if state != attributes.current {
+                                    attributes.current = state;
+                                }
+                            }
+
+                            if let Some(mut client_pending) = attributes.client_pending.take() {
+                                // update state from the client that doesn't need compositor approval
+                                if let Some(size) = client_pending.max_size.take() {
+                                    attributes.max_size = size;
+                                }
+
+                                if let Some(size) = client_pending.min_size.take() {
+                                    attributes.min_size = size;
+                                }
+                            }
+                        }
+
+                        !attributes.initial_configure_sent
+                    } else {
+                        false
+                    }
+                })
+                .unwrap_or(false);
+
+            if send_initial_configure {
+                self.send_configure();
+            }
         }
     }
 
@@ -591,7 +1176,11 @@ where
         }
         let configured = self
             .token
-            .with_role_data::<XdgSurfaceRole, _, _>(&self.wl_surface, |data| data.configured)
+            .with_role_data::<XdgToplevelSurfaceRole, _, _>(&self.wl_surface, |data| {
+                data.as_ref()
+                    .expect("xdg_toplevel exists but role has been destroyed?!")
+                    .configured
+            })
             .expect("A shell surface object exists but the surface does not have the shell_surface role ?!");
         if !configured {
             match self.shell_surface {
@@ -641,24 +1230,42 @@ where
         }
     }
 
-    /// Retrieve a copy of the pending state of this toplevel surface
+    /// Allows the pending state of this toplevel to
+    /// be manipulated.
     ///
-    /// Returns `None` of the toplevel surface actually no longer exists.
-    pub fn get_pending_state(&self) -> Option<ToplevelState> {
+    /// This should be used to inform the client about
+    /// size and state changes.
+    ///
+    /// For example after a resize request from the client.
+    ///
+    /// The state will be sent to the client when calling
+    /// send_configure.
+    pub fn with_pending_state<F, T>(&self, f: F) -> Result<T, PendingStateError>
+    where
+        F: FnOnce(&mut ToplevelState) -> T,
+    {
         if !self.alive() {
-            return None;
+            return Err(PendingStateError::DeadSurface);
         }
+
         self.token
-            .with_role_data::<XdgSurfaceRole, _, _>(&self.wl_surface, |data| match data.pending_state {
-                XdgSurfacePendingState::Toplevel(ref state) => Some(state.clone()),
-                _ => None,
+            .with_role_data(&self.wl_surface, |role| {
+                if let Some(attributes) = role {
+                    if attributes.server_pending.is_none() {
+                        attributes.server_pending = Some(attributes.current.clone());
+                    }
+
+                    let server_pending = attributes.server_pending.as_mut().unwrap();
+                    Ok(f(server_pending))
+                } else {
+                    Err(PendingStateError::RoleDestroyed)
+                }
             })
-            .ok()
-            .and_then(|x| x)
+            .unwrap()
     }
 }
 
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub(crate) enum PopupKind {
     Xdg(xdg_popup::XdgPopup),
     ZxdgV6(zxdg_popup_v6::ZxdgPopupV6),
@@ -675,9 +1282,20 @@ pub struct PopupSurface<R> {
     token: CompositorToken<R>,
 }
 
+// We implement Clone manually because #[derive(..)] would require R: Clone.
+impl<R> Clone for PopupSurface<R> {
+    fn clone(&self) -> Self {
+        Self {
+            wl_surface: self.wl_surface.clone(),
+            shell_surface: self.shell_surface.clone(),
+            token: self.token,
+        }
+    }
+}
+
 impl<R> PopupSurface<R>
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgPopupSurfaceRole> + 'static,
 {
     /// Is the popup surface referred by this handle still alive?
     pub fn alive(&self) -> bool {
@@ -688,6 +1306,20 @@ where
         shell_alive && self.wl_surface.as_ref().is_alive()
     }
 
+    /// Gets a reference of the parent WlSurface of
+    /// this popup.
+    pub fn get_parent_surface(&self) -> Option<wl_surface::WlSurface> {
+        if !self.alive() {
+            None
+        } else {
+            self.token
+                .with_role_data(&self.wl_surface, |role: &mut XdgPopupSurfaceRole| {
+                    role.as_ref().and_then(|role| role.parent.clone())
+                })
+                .unwrap()
+        }
+    }
+
     /// Do this handle and the other one actually refer to the same popup surface?
     pub fn equals(&self, other: &Self) -> bool {
         self.alive() && other.alive() && self.wl_surface.as_ref().equals(&other.wl_surface.as_ref())
@@ -726,19 +1358,135 @@ where
         })
     }
 
-    /// Send a configure event to this toplevel surface to suggest it a new configuration
+    /// Send a configure event to this popup surface to suggest it a new configuration
     ///
     /// The serial of this configure will be tracked waiting for the client to ACK it.
-    pub fn send_configure(&self, cfg: PopupConfigure) {
-        if !self.alive() {
-            return;
-        }
-        match self.shell_surface {
-            PopupKind::Xdg(ref p) => {
-                self::xdg_handlers::send_popup_configure::<R>(p, cfg);
+    pub fn send_configure(&self) {
+        if let Some(surface) = self.get_surface() {
+            if let Some((configure, initial)) = self
+                .token
+                .with_role_data(surface, |role| {
+                    if let XdgPopupSurfaceRole::Some(attributes) = role {
+                        if !attributes.initial_configure_sent || attributes.server_pending.is_some() {
+                            let pending = attributes.server_pending.take().unwrap_or(attributes.current);
+
+                            let configure = PopupConfigure {
+                                state: pending,
+                                serial: SERIAL_COUNTER.next_serial(),
+                            };
+
+                            attributes.pending_configures.push(configure);
+
+                            Some((configure, !attributes.initial_configure_sent))
+                        } else {
+                            None
+                        }
+                    } else {
+                        None
+                    }
+                })
+                .unwrap_or(None)
+            {
+                match self.shell_surface {
+                    PopupKind::Xdg(ref p) => {
+                        self::xdg_handlers::send_popup_configure::<R>(p, configure);
+                    }
+                    PopupKind::ZxdgV6(ref p) => {
+                        self::zxdgv6_handlers::send_popup_configure::<R>(p, configure);
+                    }
+                }
+
+                if initial {
+                    self.token
+                        .with_role_data(surface, |role| {
+                            if let XdgPopupSurfaceRole::Some(attributes) = role {
+                                attributes.initial_configure_sent = true;
+                            }
+                        })
+                        .unwrap();
+                }
             }
-            PopupKind::ZxdgV6(ref p) => {
-                self::zxdgv6_handlers::send_popup_configure::<R>(p, cfg);
+        }
+    }
+
+    /// Handles the role specific commit logic
+    ///
+    /// This should be called when the underlying WlSurface
+    /// handles a wl_surface.commit request.
+    pub fn commit(&self) {
+        if let Some(surface) = self.get_surface() {
+            let has_parent = self
+                .token
+                .with_role_data(surface, |role| {
+                    if let Some(attributes) = role {
+                        attributes.parent.is_some()
+                    } else {
+                        false
+                    }
+                })
+                .unwrap_or(false);
+
+            if !has_parent {
+                match self.shell_surface {
+                    PopupKind::Xdg(ref s) => {
+                        let data = s
+                            .as_ref()
+                            .user_data()
+                            .get::<self::xdg_handlers::ShellSurfaceUserData<R>>()
+                            .unwrap();
+                        data.xdg_surface.as_ref().post_error(
+                            xdg_surface::Error::NotConstructed as u32,
+                            "Surface has not been configured yet.".into(),
+                        );
+                    }
+                    PopupKind::ZxdgV6(ref s) => {
+                        let data = s
+                            .as_ref()
+                            .user_data()
+                            .get::<self::zxdgv6_handlers::ShellSurfaceUserData<R>>()
+                            .unwrap();
+                        data.xdg_surface.as_ref().post_error(
+                            zxdg_surface_v6::Error::NotConstructed as u32,
+                            "Surface has not been configured yet.".into(),
+                        );
+                    }
+                }
+                return;
+            }
+
+            let send_initial_configure = self
+                .token
+                .with_role_data(surface, |role: &mut XdgPopupSurfaceRole| {
+                    if let XdgPopupSurfaceRole::Some(attributes) = role {
+                        if let Some(window_geometry) = attributes.client_pending_window_geometry.take() {
+                            attributes.window_geometry = Some(window_geometry);
+                        }
+
+                        if attributes.initial_configure_sent {
+                            if let Some(state) = attributes.last_acked {
+                                if state != attributes.current {
+                                    attributes.current = state;
+                                }
+                            }
+
+                            if attributes.window_geometry.is_none() {
+                                attributes.window_geometry = Some(Rectangle {
+                                    width: attributes.current.geometry.width,
+                                    height: attributes.current.geometry.height,
+                                    ..Default::default()
+                                })
+                            }
+                        }
+
+                        !attributes.initial_configure_sent
+                    } else {
+                        false
+                    }
+                })
+                .unwrap_or(false);
+
+            if send_initial_configure {
+                self.send_configure();
             }
         }
     }
@@ -757,7 +1505,11 @@ where
         }
         let configured = self
             .token
-            .with_role_data::<XdgSurfaceRole, _, _>(&self.wl_surface, |data| data.configured)
+            .with_role_data::<XdgPopupSurfaceRole, _, _>(&self.wl_surface, |data| {
+                data.as_ref()
+                    .expect("xdg_popup exists but role has been destroyed?!")
+                    .configured
+            })
             .expect("A shell surface object exists but the surface does not have the shell_surface role ?!");
         if !configured {
             match self.shell_surface {
@@ -810,33 +1562,47 @@ where
         }
     }
 
-    /// Retrieve a copy of the pending state of this popup surface
+    /// Allows the pending state of this popup to
+    /// be manipulated.
     ///
-    /// Returns `None` of the popup surface actually no longer exists.
-    pub fn get_pending_state(&self) -> Option<PopupState> {
+    /// This should be used to inform the client about
+    /// size and position changes.
+    ///
+    /// For example after a move of the parent toplevel.
+    ///
+    /// The state will be sent to the client when calling
+    /// send_configure.
+    pub fn with_pending_state<F, T>(&self, f: F) -> Result<T, PendingStateError>
+    where
+        F: FnOnce(&mut PopupState) -> T,
+    {
         if !self.alive() {
-            return None;
+            return Err(PendingStateError::DeadSurface);
         }
+
         self.token
-            .with_role_data::<XdgSurfaceRole, _, _>(&self.wl_surface, |data| match data.pending_state {
-                XdgSurfacePendingState::Popup(ref state) => Some(state.clone()),
-                _ => None,
+            .with_role_data(&self.wl_surface, |role| {
+                if let Some(attributes) = role {
+                    if attributes.server_pending.is_none() {
+                        attributes.server_pending = Some(attributes.current);
+                    }
+
+                    let server_pending = attributes.server_pending.as_mut().unwrap();
+                    Ok(f(server_pending))
+                } else {
+                    Err(PendingStateError::RoleDestroyed)
+                }
             })
-            .ok()
-            .and_then(|x| x)
+            .unwrap()
     }
 }
 
 /// A configure message for toplevel surfaces
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct ToplevelConfigure {
-    /// A suggestion for a new size for the surface
-    pub size: Option<(i32, i32)>,
-    /// A notification of what are the current states of this surface
-    ///
-    /// A surface can be any combination of these possible states
-    /// at the same time.
-    pub states: Vec<xdg_toplevel::State>,
+    /// The state associated with this configure
+    pub state: ToplevelState,
+
     /// A serial number to track ACK from the client
     ///
     /// This should be an ever increasing number, as the ACK-ing
@@ -846,13 +1612,10 @@ pub struct ToplevelConfigure {
 }
 
 /// A configure message for popup surface
-#[derive(Debug)]
+#[derive(Debug, Clone, Copy)]
 pub struct PopupConfigure {
-    /// The position chosen for this popup relative to
-    /// its parent
-    pub position: (i32, i32),
-    /// A suggested size for the popup
-    pub size: (i32, i32),
+    /// The state associated with this configure,
+    pub state: PopupState,
     /// A serial number to track ACK from the client
     ///
     /// This should be an ever increasing number, as the ACK-ing
@@ -861,6 +1624,31 @@ pub struct PopupConfigure {
     pub serial: Serial,
 }
 
+/// Defines the possible configure variants
+/// for a XdgSurface that will be issued in
+/// the user_impl for notifying about a ack_configure
+#[derive(Debug)]
+pub enum Configure {
+    /// A xdg_surface with a role of xdg_toplevel
+    /// has processed an ack_configure request
+    Toplevel(ToplevelConfigure),
+    /// A xdg_surface with a role of xdg_popup
+    /// has processed an ack_configure request
+    Popup(PopupConfigure),
+}
+
+impl From<ToplevelConfigure> for Configure {
+    fn from(configure: ToplevelConfigure) -> Self {
+        Configure::Toplevel(configure)
+    }
+}
+
+impl From<PopupConfigure> for Configure {
+    fn from(configure: PopupConfigure) -> Self {
+        Configure::Popup(configure)
+    }
+}
+
 /// Events generated by xdg shell surfaces
 ///
 /// These are events that the provided implementation cannot process
@@ -976,6 +1764,6 @@ pub enum XdgRequest<R> {
         /// The surface.
         surface: wl_surface::WlSurface,
         /// The configure serial.
-        serial: Serial,
+        configure: Configure,
     },
 }
diff --git a/src/wayland/shell/xdg/xdg_handlers.rs b/src/wayland/shell/xdg/xdg_handlers.rs
index 6f1b4dd..5bb03d0 100644
--- a/src/wayland/shell/xdg/xdg_handlers.rs
+++ b/src/wayland/shell/xdg/xdg_handlers.rs
@@ -1,6 +1,7 @@
 use std::{cell::RefCell, ops::Deref as _, sync::Mutex};
 
 use crate::wayland::compositor::{roles::*, CompositorToken};
+use crate::wayland::shell::xdg::{ConfigureError, PopupState};
 use crate::wayland::Serial;
 use wayland_protocols::xdg_shell::server::{
     xdg_popup, xdg_positioner, xdg_surface, xdg_toplevel, xdg_wm_base,
@@ -10,9 +11,9 @@ use wayland_server::{protocol::wl_surface, Filter, Main};
 use crate::utils::Rectangle;
 
 use super::{
-    make_shell_client_data, PopupConfigure, PopupKind, PopupState, PositionerState, ShellClient,
-    ShellClientData, ShellData, ToplevelConfigure, ToplevelKind, ToplevelState, XdgRequest,
-    XdgSurfacePendingState, XdgSurfaceRole,
+    make_shell_client_data, PopupConfigure, PopupKind, PositionerState, ShellClient, ShellClientData,
+    ShellData, ToplevelClientPending, ToplevelConfigure, ToplevelKind, XdgPopupSurfaceRole,
+    XdgPopupSurfaceRoleAttributes, XdgRequest, XdgToplevelSurfaceRole, XdgToplevelSurfaceRoleAttributes,
 };
 
 pub(crate) fn implement_wm_base<R>(
@@ -20,7 +21,7 @@ pub(crate) fn implement_wm_base<R>(
     shell_data: &ShellData<R>,
 ) -> xdg_wm_base::XdgWmBase
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + Role<XdgPopupSurfaceRole> + 'static,
 {
     shell.quick_assign(|shell, req, _data| wm_implementation::<R>(req, shell.deref().clone()));
     shell.as_ref().user_data().set(|| ShellUserData {
@@ -55,7 +56,7 @@ pub(crate) fn make_shell_client<R>(
 
 fn wm_implementation<R>(request: xdg_wm_base::Request, shell: xdg_wm_base::XdgWmBase)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + Role<XdgPopupSurfaceRole> + 'static,
 {
     let data = shell.as_ref().user_data().get::<ShellUserData<R>>().unwrap();
     match request {
@@ -66,24 +67,9 @@ where
             implement_positioner(id);
         }
         xdg_wm_base::Request::GetXdgSurface { id, surface } => {
-            let role_data = XdgSurfaceRole {
-                pending_state: XdgSurfacePendingState::None,
-                window_geometry: None,
-                pending_configures: Vec::new(),
-                configured: false,
-            };
-            if data
-                .shell_data
-                .compositor_token
-                .give_role_with(&surface, role_data)
-                .is_err()
-            {
-                shell.as_ref().post_error(
-                    xdg_wm_base::Error::Role as u32,
-                    "Surface already has a role.".into(),
-                );
-                return;
-            }
+            // Do not assign a role to the surface here
+            // xdg_surface is not role, only xdg_toplevel and
+            // xdg_popup are defined as roles
             id.quick_assign(|surface, req, _data| {
                 xdg_surface_implementation::<R>(req, surface.deref().clone())
             });
@@ -98,8 +84,8 @@ where
             let serial = Serial::from(serial);
             let valid = {
                 let mut guard = data.client_data.lock().unwrap();
-                if guard.pending_ping == serial {
-                    guard.pending_ping = Serial::from(0);
+                if guard.pending_ping == Some(serial) {
+                    guard.pending_ping = None;
                     true
                 } else {
                     false
@@ -191,7 +177,7 @@ struct XdgSurfaceUserData<R> {
 
 fn destroy_surface<R>(surface: xdg_surface::XdgSurface)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + Role<XdgPopupSurfaceRole> + 'static,
 {
     let data = surface
         .as_ref()
@@ -204,24 +190,36 @@ where
         // disconnecting client), ignore the protocol check.
         return;
     }
-    data.shell_data
+
+    if !data.shell_data.compositor_token.has_a_role(&data.wl_surface) {
+        // No role assigned to the surface, we can exit early.
+        return;
+    }
+
+    let has_active_xdg_role = data
+        .shell_data
         .compositor_token
-        .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |rdata| {
-            if let XdgSurfacePendingState::None = rdata.pending_state {
-                // all is good
-            } else {
-                data.wm_base.as_ref().post_error(
-                    xdg_wm_base::Error::Role as u32,
-                    "xdg_surface was destroyed before its role object".into(),
-                );
-            }
+        .with_role_data(&data.wl_surface, |role: &mut XdgToplevelSurfaceRole| {
+            role.is_some()
         })
-        .expect("xdg_surface exists but surface has not shell_surface role?!");
+        .unwrap_or(false)
+        || data
+            .shell_data
+            .compositor_token
+            .with_role_data(&data.wl_surface, |role: &mut XdgPopupSurfaceRole| role.is_some())
+            .unwrap_or(false);
+
+    if has_active_xdg_role {
+        data.wm_base.as_ref().post_error(
+            xdg_wm_base::Error::Role as u32,
+            "xdg_surface was destroyed before its role object".into(),
+        );
+    }
 }
 
 fn xdg_surface_implementation<R>(request: xdg_surface::Request, xdg_surface: xdg_surface::XdgSurface)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + Role<XdgPopupSurfaceRole> + 'static,
 {
     let data = xdg_surface
         .as_ref()
@@ -233,18 +231,25 @@ where
             // all is handled by our destructor
         }
         xdg_surface::Request::GetToplevel { id } => {
-            data.shell_data
+            // We now can assign a role to the surface
+            let surface = &data.wl_surface;
+            let shell = &data.wm_base;
+
+            let role_data = XdgToplevelSurfaceRole::Some(Default::default());
+
+            if data
+                .shell_data
                 .compositor_token
-                .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| {
-                    data.pending_state = XdgSurfacePendingState::Toplevel(ToplevelState {
-                        parent: None,
-                        title: String::new(),
-                        app_id: String::new(),
-                        min_size: (0, 0),
-                        max_size: (0, 0),
-                    });
-                })
-                .expect("xdg_surface exists but surface has not shell_surface role?!");
+                .give_role_with(&surface, role_data)
+                .is_err()
+            {
+                shell.as_ref().post_error(
+                    xdg_wm_base::Error::Role as u32,
+                    "Surface already has a role.".into(),
+                );
+                return;
+            }
+
             id.quick_assign(|toplevel, req, _data| {
                 toplevel_implementation::<R>(req, toplevel.deref().clone())
             });
@@ -276,7 +281,9 @@ where
                 .as_ref()
                 .user_data()
                 .get::<RefCell<PositionerState>>()
-                .unwrap();
+                .unwrap()
+                .borrow()
+                .clone();
 
             let parent_surface = parent.map(|parent| {
                 let parent_data = parent
@@ -286,16 +293,34 @@ where
                     .unwrap();
                 parent_data.wl_surface.clone()
             });
-            data.shell_data
+
+            // We now can assign a role to the surface
+            let surface = &data.wl_surface;
+            let shell = &data.wm_base;
+
+            let role_data = XdgPopupSurfaceRole::Some(XdgPopupSurfaceRoleAttributes {
+                parent: parent_surface,
+                server_pending: Some(PopupState {
+                    // Set the positioner data as the popup geometry
+                    geometry: positioner_data.get_geometry(),
+                }),
+                ..Default::default()
+            });
+
+            if data
+                .shell_data
                 .compositor_token
-                .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| {
-                    data.pending_state = XdgSurfacePendingState::Popup(PopupState {
-                        parent: parent_surface,
-                        positioner: positioner_data.borrow().clone(),
-                    });
-                })
-                .expect("xdg_surface exists but surface has not shell_surface role?!");
-            id.quick_assign(|popup, req, _data| xg_popup_implementation::<R>(req, popup.deref().clone()));
+                .give_role_with(&surface, role_data)
+                .is_err()
+            {
+                shell.as_ref().post_error(
+                    xdg_wm_base::Error::Role as u32,
+                    "Surface already has a role.".into(),
+                );
+                return;
+            }
+
+            id.quick_assign(|popup, req, _data| xdg_popup_implementation::<R>(req, popup.deref().clone()));
             id.assign_destructor(Filter::new(|popup, _, _data| destroy_popup::<R>(popup)));
             id.as_ref().user_data().set(|| ShellSurfaceUserData {
                 shell_data: data.shell_data.clone(),
@@ -316,40 +341,93 @@ where
             (&mut *user_impl)(XdgRequest::NewPopup { surface: handle });
         }
         xdg_surface::Request::SetWindowGeometry { x, y, width, height } => {
-            data.shell_data
+            // Check the role of the surface, this can be either xdg_toplevel
+            // or xdg_popup. If none of the role matches the xdg_surface has no role set
+            // which is a protocol error.
+            let surface = &data.wl_surface;
+
+            if !data.shell_data.compositor_token.has_a_role(surface) {
+                data.wm_base.as_ref().post_error(
+                    xdg_surface::Error::NotConstructed as u32,
+                    "xdg_surface must have a role.".into(),
+                );
+                return;
+            }
+
+            // Set the next window geometry here, the geometry will be moved from
+            // next to the current geometry on a commit. This has to be done currently
+            // in anvil as the whole commit logic is implemented there until a proper
+            // abstraction has been found to handle commits within roles. This also
+            // ensures that a commit for a xdg_surface follows the rules for subsurfaces.
+            let has_wrong_role = data
+                .shell_data
                 .compositor_token
-                .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| {
-                    data.window_geometry = Some(Rectangle { x, y, width, height });
+                .with_xdg_role(surface, |role| {
+                    role.set_window_geometry(Rectangle { x, y, width, height })
                 })
-                .expect("xdg_surface exists but surface has not shell_surface role?!");
+                .is_err();
+
+            if has_wrong_role {
+                data.wm_base.as_ref().post_error(
+                    xdg_wm_base::Error::Role as u32,
+                    "xdg_surface must have a role of xdg_toplevel or xdg_popup.".into(),
+                );
+            }
         }
         xdg_surface::Request::AckConfigure { serial } => {
-            data.shell_data
+            let serial = Serial::from(serial);
+            let surface = &data.wl_surface;
+
+            // Check the role of the surface, this can be either xdg_toplevel
+            // or xdg_popup. If none of the role matches the xdg_surface has no role set
+            // which is a protocol error.
+            if !data.shell_data.compositor_token.has_a_role(surface) {
+                data.wm_base.as_ref().post_error(
+                    xdg_surface::Error::NotConstructed as u32,
+                    "xdg_surface must have a role.".into(),
+                );
+                return;
+            }
+
+            // Find the correct configure state for the provided serial
+            // discard all configure states that are older than the provided
+            // serial.
+            // If no matching serial can be found raise a protocol error
+            //
+            // Invoke the user impl with the found configuration
+            // This has to include the serial and the role specific data.
+            // - For xdg_popup there is no data.
+            // - For xdg_toplevel send the state data including
+            //   width, height, min/max size, maximized, fullscreen, resizing, activated
+            //
+            // This can be used to integrate custom protocol extensions
+            //
+            let configure = match data
+                .shell_data
                 .compositor_token
-                .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |role_data| {
-                    let mut found = false;
-                    role_data.pending_configures.retain(|&s| {
-                        if s == serial {
-                            found = true;
-                        }
-                        s > serial
-                    });
-                    if !found {
-                        // client responded to a non-existing configure
-                        data.wm_base.as_ref().post_error(
-                            xdg_wm_base::Error::InvalidSurfaceState as u32,
-                            format!("Wrong configure serial: {}", serial),
-                        );
-                    }
-                    role_data.configured = true;
-                })
-                .expect("xdg_surface exists but surface has not shell_surface role?!");
+                .with_xdg_role(surface, |role| role.ack_configure(serial))
+            {
+                Ok(Ok(configure)) => configure,
+                Ok(Err(ConfigureError::SerialNotFound(serial))) => {
+                    data.wm_base.as_ref().post_error(
+                        xdg_wm_base::Error::InvalidSurfaceState as u32,
+                        format!("wrong configure serial: {}", <u32>::from(serial)),
+                    );
+                    return;
+                }
+                Err(_) => {
+                    data.wm_base.as_ref().post_error(
+                        xdg_wm_base::Error::Role as u32,
+                        "xdg_surface must have a role of xdg_toplevel or xdg_popup.".into(),
+                    );
+                    return;
+                }
+            };
 
             let mut user_impl = data.shell_data.user_impl.borrow_mut();
-            let serial = Serial::from(serial);
             (&mut *user_impl)(XdgRequest::AckConfigure {
-                surface: data.wl_surface.clone(),
-                serial,
+                surface: surface.clone(),
+                configure,
             });
         }
         _ => unreachable!(),
@@ -368,38 +446,62 @@ pub(crate) struct ShellSurfaceUserData<R> {
 }
 
 // Utility functions allowing to factor out a lot of the upcoming logic
-fn with_surface_toplevel_data<R, F>(shell_data: &ShellData<R>, toplevel: &xdg_toplevel::XdgToplevel, f: F)
+fn with_surface_toplevel_role_data<R, F, T>(
+    shell_data: &ShellData<R>,
+    toplevel: &xdg_toplevel::XdgToplevel,
+    f: F,
+) -> T
 where
-    R: Role<XdgSurfaceRole> + 'static,
-    F: FnOnce(&mut ToplevelState),
+    R: Role<XdgToplevelSurfaceRole> + 'static,
+    F: FnOnce(&mut XdgToplevelSurfaceRoleAttributes) -> T,
 {
-    let toplevel_data = toplevel
+    let data = toplevel
         .as_ref()
         .user_data()
         .get::<ShellSurfaceUserData<R>>()
         .unwrap();
     shell_data
         .compositor_token
-        .with_role_data::<XdgSurfaceRole, _, _>(&toplevel_data.wl_surface, |data| match data.pending_state {
-            XdgSurfacePendingState::Toplevel(ref mut toplevel_data) => f(toplevel_data),
-            _ => unreachable!(),
+        .with_role_data::<XdgToplevelSurfaceRole, _, _>(&data.wl_surface, |role| {
+            let attributes = role
+                .as_mut()
+                .expect("xdg_toplevel exists but role has been destroyed?!");
+            f(attributes)
         })
-        .expect("xdg_toplevel exists but surface has not shell_surface role?!");
+        .expect("xdg_toplevel exists but surface has not shell_surface role?!")
+}
+
+fn with_surface_toplevel_client_pending<R, F, T>(
+    shell_data: &ShellData<R>,
+    toplevel: &xdg_toplevel::XdgToplevel,
+    f: F,
+) -> T
+where
+    R: Role<XdgToplevelSurfaceRole> + 'static,
+    F: FnOnce(&mut ToplevelClientPending) -> T,
+{
+    with_surface_toplevel_role_data(shell_data, toplevel, |data| {
+        if data.client_pending.is_none() {
+            data.client_pending = Some(Default::default());
+        }
+        f(&mut data.client_pending.as_mut().unwrap())
+    })
 }
 
 pub fn send_toplevel_configure<R>(resource: &xdg_toplevel::XdgToplevel, configure: ToplevelConfigure)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + 'static,
 {
     let data = resource
         .as_ref()
         .user_data()
         .get::<ShellSurfaceUserData<R>>()
         .unwrap();
-    let (width, height) = configure.size.unwrap_or((0, 0));
+
+    let (width, height) = configure.state.size.unwrap_or((0, 0));
     // convert the Vec<State> (which is really a Vec<u32>) into Vec<u8>
     let states = {
-        let mut states = configure.states;
+        let mut states: Vec<xdg_toplevel::State> = configure.state.states.into();
         let ptr = states.as_mut_ptr();
         let len = states.len();
         let cap = states.capacity();
@@ -407,15 +509,13 @@ where
         unsafe { Vec::from_raw_parts(ptr as *mut u8, len * 4, cap * 4) }
     };
     let serial = configure.serial;
+
+    // Send the toplevel configure
     resource.configure(width, height, states);
+
+    // Send the base xdg_surface configure event to mark
+    // The configure as finished
     data.xdg_surface.configure(serial.into());
-    // Add the configure as pending
-    data.shell_data
-        .compositor_token
-        .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| {
-            data.pending_configures.push(serial.into())
-        })
-        .expect("xdg_toplevel exists but surface has not shell_surface role?!");
 }
 
 fn make_toplevel_handle<R: 'static>(resource: &xdg_toplevel::XdgToplevel) -> super::ToplevelSurface<R> {
@@ -433,7 +533,7 @@ fn make_toplevel_handle<R: 'static>(resource: &xdg_toplevel::XdgToplevel) -> sup
 
 fn toplevel_implementation<R>(request: xdg_toplevel::Request, toplevel: xdg_toplevel::XdgToplevel)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + 'static,
 {
     let data = toplevel
         .as_ref()
@@ -445,8 +545,9 @@ where
             // all it done by the destructor
         }
         xdg_toplevel::Request::SetParent { parent } => {
-            with_surface_toplevel_data(&data.shell_data, &toplevel, |toplevel_data| {
-                toplevel_data.parent = parent.map(|toplevel_surface_parent| {
+            // Parent is not double buffered, we can set it directly
+            with_surface_toplevel_role_data(&data.shell_data, &toplevel, |data| {
+                data.parent = parent.map(|toplevel_surface_parent| {
                     toplevel_surface_parent
                         .as_ref()
                         .user_data()
@@ -458,16 +559,19 @@ where
             });
         }
         xdg_toplevel::Request::SetTitle { title } => {
-            with_surface_toplevel_data(&data.shell_data, &toplevel, |toplevel_data| {
-                toplevel_data.title = title;
+            // Title is not double buffered, we can set it directly
+            with_surface_toplevel_role_data(&data.shell_data, &toplevel, |data| {
+                data.title = Some(title);
             });
         }
         xdg_toplevel::Request::SetAppId { app_id } => {
-            with_surface_toplevel_data(&data.shell_data, &toplevel, |toplevel_data| {
-                toplevel_data.app_id = app_id;
+            // AppId is not double buffered, we can set it directly
+            with_surface_toplevel_role_data(&data.shell_data, &toplevel, |role| {
+                role.app_id = Some(app_id);
             });
         }
         xdg_toplevel::Request::ShowWindowMenu { seat, serial, x, y } => {
+            // This has to be handled by the compositor
             let handle = make_toplevel_handle(&toplevel);
             let serial = Serial::from(serial);
             let mut user_impl = data.shell_data.user_impl.borrow_mut();
@@ -479,6 +583,7 @@ where
             });
         }
         xdg_toplevel::Request::Move { seat, serial } => {
+            // This has to be handled by the compositor
             let handle = make_toplevel_handle(&toplevel);
             let serial = Serial::from(serial);
             let mut user_impl = data.shell_data.user_impl.borrow_mut();
@@ -489,6 +594,7 @@ where
             });
         }
         xdg_toplevel::Request::Resize { seat, serial, edges } => {
+            // This has to be handled by the compositor
             let handle = make_toplevel_handle(&toplevel);
             let mut user_impl = data.shell_data.user_impl.borrow_mut();
             let serial = Serial::from(serial);
@@ -500,13 +606,13 @@ where
             });
         }
         xdg_toplevel::Request::SetMaxSize { width, height } => {
-            with_surface_toplevel_data(&data.shell_data, &toplevel, |toplevel_data| {
-                toplevel_data.max_size = (width, height);
+            with_surface_toplevel_client_pending(&data.shell_data, &toplevel, |toplevel_data| {
+                toplevel_data.max_size = Some((width, height));
             });
         }
         xdg_toplevel::Request::SetMinSize { width, height } => {
-            with_surface_toplevel_data(&data.shell_data, &toplevel, |toplevel_data| {
-                toplevel_data.min_size = (width, height);
+            with_surface_toplevel_client_pending(&data.shell_data, &toplevel, |toplevel_data| {
+                toplevel_data.min_size = Some((width, height));
             });
         }
         xdg_toplevel::Request::SetMaximized => {
@@ -533,6 +639,8 @@ where
             (&mut *user_impl)(XdgRequest::UnFullscreen { surface: handle });
         }
         xdg_toplevel::Request::SetMinimized => {
+            // This has to be handled by the compositor, may not be
+            // supported and just ignored
             let handle = make_toplevel_handle(&toplevel);
             let mut user_impl = data.shell_data.user_impl.borrow_mut();
             (&mut *user_impl)(XdgRequest::Minimize { surface: handle });
@@ -543,7 +651,7 @@ where
 
 fn destroy_toplevel<R>(toplevel: xdg_toplevel::XdgToplevel)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + 'static,
 {
     let data = toplevel
         .as_ref()
@@ -557,9 +665,8 @@ where
     } else {
         data.shell_data
             .compositor_token
-            .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| {
-                data.pending_state = XdgSurfacePendingState::None;
-                data.configured = false;
+            .with_role_data(&data.wl_surface, |role_data| {
+                *role_data = XdgToplevelSurfaceRole::None;
             })
             .expect("xdg_toplevel exists but surface has not shell_surface role?!");
     }
@@ -578,25 +685,23 @@ where
 
 pub(crate) fn send_popup_configure<R>(resource: &xdg_popup::XdgPopup, configure: PopupConfigure)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgPopupSurfaceRole> + 'static,
 {
     let data = resource
         .as_ref()
         .user_data()
         .get::<ShellSurfaceUserData<R>>()
         .unwrap();
-    let (x, y) = configure.position;
-    let (width, height) = configure.size;
+
     let serial = configure.serial;
-    resource.configure(x, y, width, height);
+    let geometry = configure.state.geometry;
+
+    // Send the popup configure
+    resource.configure(geometry.x, geometry.y, geometry.width, geometry.height);
+
+    // Send the base xdg_surface configure event to mark
+    // the configure as finished
     data.xdg_surface.configure(serial.into());
-    // Add the configure as pending
-    data.shell_data
-        .compositor_token
-        .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| {
-            data.pending_configures.push(serial.into())
-        })
-        .expect("xdg_toplevel exists but surface has not shell_surface role?!");
 }
 
 fn make_popup_handle<R: 'static>(resource: &xdg_popup::XdgPopup) -> super::PopupSurface<R> {
@@ -612,9 +717,9 @@ fn make_popup_handle<R: 'static>(resource: &xdg_popup::XdgPopup) -> super::Popup
     }
 }
 
-fn xg_popup_implementation<R>(request: xdg_popup::Request, popup: xdg_popup::XdgPopup)
+fn xdg_popup_implementation<R>(request: xdg_popup::Request, popup: xdg_popup::XdgPopup)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgPopupSurfaceRole> + 'static,
 {
     let data = popup
         .as_ref()
@@ -641,7 +746,7 @@ where
 
 fn destroy_popup<R>(popup: xdg_popup::XdgPopup)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgPopupSurfaceRole> + 'static,
 {
     let data = popup
         .as_ref()
@@ -655,9 +760,8 @@ where
     } else {
         data.shell_data
             .compositor_token
-            .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| {
-                data.pending_state = XdgSurfacePendingState::None;
-                data.configured = false;
+            .with_role_data(&data.wl_surface, |role_data| {
+                *role_data = XdgPopupSurfaceRole::None;
             })
             .expect("xdg_popup exists but surface has not shell_surface role?!");
     }
diff --git a/src/wayland/shell/xdg/zxdgv6_handlers.rs b/src/wayland/shell/xdg/zxdgv6_handlers.rs
index a3d1f7b..d77db06 100644
--- a/src/wayland/shell/xdg/zxdgv6_handlers.rs
+++ b/src/wayland/shell/xdg/zxdgv6_handlers.rs
@@ -1,6 +1,7 @@
 use std::{cell::RefCell, ops::Deref as _, sync::Mutex};
 
 use crate::wayland::compositor::{roles::*, CompositorToken};
+use crate::wayland::shell::xdg::{ConfigureError, PopupState};
 use crate::wayland::Serial;
 use wayland_protocols::{
     unstable::xdg_shell::v6::server::{
@@ -13,9 +14,9 @@ use wayland_server::{protocol::wl_surface, Filter, Main};
 use crate::utils::Rectangle;
 
 use super::{
-    make_shell_client_data, PopupConfigure, PopupKind, PopupState, PositionerState, ShellClient,
-    ShellClientData, ShellData, ToplevelConfigure, ToplevelKind, ToplevelState, XdgRequest,
-    XdgSurfacePendingState, XdgSurfaceRole,
+    make_shell_client_data, PopupConfigure, PopupKind, PositionerState, ShellClient, ShellClientData,
+    ShellData, ToplevelClientPending, ToplevelConfigure, ToplevelKind, XdgPopupSurfaceRole,
+    XdgPopupSurfaceRoleAttributes, XdgRequest, XdgToplevelSurfaceRole, XdgToplevelSurfaceRoleAttributes,
 };
 
 pub(crate) fn implement_shell<R>(
@@ -23,7 +24,7 @@ pub(crate) fn implement_shell<R>(
     shell_data: &ShellData<R>,
 ) -> zxdg_shell_v6::ZxdgShellV6
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + Role<XdgPopupSurfaceRole> + 'static,
 {
     shell.quick_assign(|shell, req, _data| shell_implementation::<R>(req, shell.deref().clone()));
     shell.as_ref().user_data().set(|| ShellUserData {
@@ -58,7 +59,7 @@ pub(crate) fn make_shell_client<R>(
 
 fn shell_implementation<R>(request: zxdg_shell_v6::Request, shell: zxdg_shell_v6::ZxdgShellV6)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + Role<XdgPopupSurfaceRole> + 'static,
 {
     let data = shell.as_ref().user_data().get::<ShellUserData<R>>().unwrap();
     match request {
@@ -69,24 +70,6 @@ where
             implement_positioner(id);
         }
         zxdg_shell_v6::Request::GetXdgSurface { id, surface } => {
-            let role_data = XdgSurfaceRole {
-                pending_state: XdgSurfacePendingState::None,
-                window_geometry: None,
-                pending_configures: Vec::new(),
-                configured: false,
-            };
-            if data
-                .shell_data
-                .compositor_token
-                .give_role_with(&surface, role_data)
-                .is_err()
-            {
-                shell.as_ref().post_error(
-                    zxdg_shell_v6::Error::Role as u32,
-                    "Surface already has a role.".into(),
-                );
-                return;
-            }
             id.quick_assign(|surface, req, _data| {
                 xdg_surface_implementation::<R>(req, surface.deref().clone())
             });
@@ -98,10 +81,11 @@ where
             });
         }
         zxdg_shell_v6::Request::Pong { serial } => {
+            let serial = Serial::from(serial);
             let valid = {
                 let mut guard = data.client_data.lock().unwrap();
-                if guard.pending_ping == Serial::from(serial) {
-                    guard.pending_ping = Serial::from(0);
+                if guard.pending_ping == Some(serial) {
+                    guard.pending_ping = None;
                     true
                 } else {
                     false
@@ -209,7 +193,7 @@ struct XdgSurfaceUserData<R> {
 
 fn destroy_surface<R>(surface: zxdg_surface_v6::ZxdgSurfaceV6)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + Role<XdgPopupSurfaceRole> + 'static,
 {
     let data = surface
         .as_ref()
@@ -222,26 +206,38 @@ where
         // disconnecting client), ignore the protocol check.
         return;
     }
-    data.shell_data
+
+    if !data.shell_data.compositor_token.has_a_role(&data.wl_surface) {
+        // No role assigned to the surface, we can exit early.
+        return;
+    }
+
+    let has_active_xdg_role = data
+        .shell_data
         .compositor_token
-        .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |rdata| {
-            if let XdgSurfacePendingState::None = rdata.pending_state {
-                // all is good
-            } else {
-                data.shell.as_ref().post_error(
-                    zxdg_shell_v6::Error::Role as u32,
-                    "xdg_surface was destroyed before its role object".into(),
-                );
-            }
+        .with_role_data(&data.wl_surface, |role: &mut XdgToplevelSurfaceRole| {
+            role.is_some()
         })
-        .expect("xdg_surface exists but surface has not shell_surface role?!");
+        .unwrap_or(false)
+        || data
+            .shell_data
+            .compositor_token
+            .with_role_data(&data.wl_surface, |role: &mut XdgPopupSurfaceRole| role.is_some())
+            .unwrap_or(false);
+
+    if has_active_xdg_role {
+        data.shell.as_ref().post_error(
+            zxdg_shell_v6::Error::Role as u32,
+            "xdg_surface was destroyed before its role object".into(),
+        );
+    }
 }
 
 fn xdg_surface_implementation<R>(
     request: zxdg_surface_v6::Request,
     xdg_surface: zxdg_surface_v6::ZxdgSurfaceV6,
 ) where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + Role<XdgPopupSurfaceRole> + 'static,
 {
     let data = xdg_surface
         .as_ref()
@@ -253,18 +249,25 @@ fn xdg_surface_implementation<R>(
             // all is handled by our destructor
         }
         zxdg_surface_v6::Request::GetToplevel { id } => {
-            data.shell_data
+            // We now can assign a role to the surface
+            let surface = &data.wl_surface;
+            let shell = &data.shell;
+
+            let role_data = XdgToplevelSurfaceRole::Some(Default::default());
+
+            if data
+                .shell_data
                 .compositor_token
-                .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| {
-                    data.pending_state = XdgSurfacePendingState::Toplevel(ToplevelState {
-                        parent: None,
-                        title: String::new(),
-                        app_id: String::new(),
-                        min_size: (0, 0),
-                        max_size: (0, 0),
-                    });
-                })
-                .expect("xdg_surface exists but surface has not shell_surface role?!");
+                .give_role_with(&surface, role_data)
+                .is_err()
+            {
+                shell.as_ref().post_error(
+                    zxdg_shell_v6::Error::Role as u32,
+                    "Surface already has a role.".into(),
+                );
+                return;
+            }
+
             id.quick_assign(|toplevel, req, _data| {
                 toplevel_implementation::<R>(req, toplevel.deref().clone())
             });
@@ -296,22 +299,45 @@ fn xdg_surface_implementation<R>(
                 .as_ref()
                 .user_data()
                 .get::<RefCell<PositionerState>>()
-                .unwrap();
+                .unwrap()
+                .borrow()
+                .clone();
 
-            let parent_data = parent
-                .as_ref()
-                .user_data()
-                .get::<XdgSurfaceUserData<R>>()
-                .unwrap();
-            data.shell_data
+            let parent_surface = {
+                let parent_data = parent
+                    .as_ref()
+                    .user_data()
+                    .get::<XdgSurfaceUserData<R>>()
+                    .unwrap();
+                parent_data.wl_surface.clone()
+            };
+
+            // We now can assign a role to the surface
+            let surface = &data.wl_surface;
+            let shell = &data.shell;
+
+            let role_data = XdgPopupSurfaceRole::Some(XdgPopupSurfaceRoleAttributes {
+                parent: Some(parent_surface),
+                server_pending: Some(PopupState {
+                    // Set the positioner data as the popup geometry
+                    geometry: positioner_data.get_geometry(),
+                }),
+                ..Default::default()
+            });
+
+            if data
+                .shell_data
                 .compositor_token
-                .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| {
-                    data.pending_state = XdgSurfacePendingState::Popup(PopupState {
-                        parent: Some(parent_data.wl_surface.clone()),
-                        positioner: positioner_data.borrow().clone(),
-                    });
-                })
-                .expect("xdg_surface exists but surface has not shell_surface role?!");
+                .give_role_with(&surface, role_data)
+                .is_err()
+            {
+                shell.as_ref().post_error(
+                    zxdg_shell_v6::Error::Role as u32,
+                    "Surface already has a role.".into(),
+                );
+                return;
+            }
+
             id.quick_assign(|popup, req, _data| popup_implementation::<R>(req, popup.deref().clone()));
             id.assign_destructor(Filter::new(|popup, _, _data| destroy_popup::<R>(popup)));
             id.as_ref().user_data().set(|| ShellSurfaceUserData {
@@ -333,40 +359,93 @@ fn xdg_surface_implementation<R>(
             (&mut *user_impl)(XdgRequest::NewPopup { surface: handle });
         }
         zxdg_surface_v6::Request::SetWindowGeometry { x, y, width, height } => {
-            data.shell_data
+            // Check the role of the surface, this can be either xdg_toplevel
+            // or xdg_popup. If none of the role matches the xdg_surface has no role set
+            // which is a protocol error.
+            let surface = &data.wl_surface;
+
+            if !data.shell_data.compositor_token.has_a_role(surface) {
+                data.shell.as_ref().post_error(
+                    zxdg_surface_v6::Error::NotConstructed as u32,
+                    "xdg_surface must have a role.".into(),
+                );
+                return;
+            }
+
+            // Set the next window geometry here, the geometry will be moved from
+            // next to the current geometry on a commit. This has to be done currently
+            // in anvil as the whole commit logic is implemented there until a proper
+            // abstraction has been found to handle commits within roles. This also
+            // ensures that a commit for a xdg_surface follows the rules for subsurfaces.
+            let has_wrong_role = data
+                .shell_data
                 .compositor_token
-                .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| {
-                    data.window_geometry = Some(Rectangle { x, y, width, height });
+                .with_xdg_role(surface, |role| {
+                    role.set_window_geometry(Rectangle { x, y, width, height })
                 })
-                .expect("xdg_surface exists but surface has not shell_surface role?!");
+                .is_err();
+
+            if has_wrong_role {
+                data.shell.as_ref().post_error(
+                    zxdg_shell_v6::Error::Role as u32,
+                    "xdg_surface must have a role of xdg_toplevel or xdg_popup.".into(),
+                );
+            }
         }
         zxdg_surface_v6::Request::AckConfigure { serial } => {
-            data.shell_data
+            let serial = Serial::from(serial);
+            let surface = &data.wl_surface;
+
+            // Check the role of the surface, this can be either xdg_toplevel
+            // or xdg_popup. If none of the role matches the xdg_surface has no role set
+            // which is a protocol error.
+            if !data.shell_data.compositor_token.has_a_role(surface) {
+                data.shell.as_ref().post_error(
+                    zxdg_surface_v6::Error::NotConstructed as u32,
+                    "xdg_surface must have a role.".into(),
+                );
+                return;
+            }
+
+            // Find the correct configure state for the provided serial
+            // discard all configure states that are older than the provided
+            // serial.
+            // If no matching serial can be found raise a protocol error
+            //
+            // Invoke the user impl with the found configuration
+            // This has to include the serial and the role specific data.
+            // - For xdg_popup there is no data.
+            // - For xdg_toplevel send the state data including
+            //   width, height, min/max size, maximized, fullscreen, resizing, activated
+            //
+            // This can be used to integrate custom protocol extensions
+            //
+            let configure = match data
+                .shell_data
                 .compositor_token
-                .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |role_data| {
-                    let mut found = false;
-                    role_data.pending_configures.retain(|&s| {
-                        if s == serial {
-                            found = true;
-                        }
-                        s > serial
-                    });
-                    if !found {
-                        // client responded to a non-existing configure
-                        data.shell.as_ref().post_error(
-                            zxdg_shell_v6::Error::InvalidSurfaceState as u32,
-                            format!("Wrong configure serial: {}", serial),
-                        );
-                    }
-                    role_data.configured = true;
-                })
-                .expect("xdg_surface exists but surface has not shell_surface role?!");
+                .with_xdg_role(surface, |role| role.ack_configure(serial))
+            {
+                Ok(Ok(configure)) => configure,
+                Ok(Err(ConfigureError::SerialNotFound(serial))) => {
+                    data.shell.as_ref().post_error(
+                        zxdg_shell_v6::Error::InvalidSurfaceState as u32,
+                        format!("wrong configure serial: {}", <u32>::from(serial)),
+                    );
+                    return;
+                }
+                Err(_) => {
+                    data.shell.as_ref().post_error(
+                        zxdg_shell_v6::Error::Role as u32,
+                        "xdg_surface must have a role of xdg_toplevel or xdg_popup.".into(),
+                    );
+                    return;
+                }
+            };
 
             let mut user_impl = data.shell_data.user_impl.borrow_mut();
-            let serial = Serial::from(serial);
             (&mut *user_impl)(XdgRequest::AckConfigure {
-                surface: data.wl_surface.clone(),
-                serial,
+                surface: surface.clone(),
+                configure,
             });
         }
         _ => unreachable!(),
@@ -385,38 +464,62 @@ pub struct ShellSurfaceUserData<R> {
 }
 
 // Utility functions allowing to factor out a lot of the upcoming logic
-fn with_surface_toplevel_data<R, F>(toplevel: &zxdg_toplevel_v6::ZxdgToplevelV6, f: F)
+fn with_surface_toplevel_role_data<R, F, T>(
+    shell_data: &ShellData<R>,
+    toplevel: &zxdg_toplevel_v6::ZxdgToplevelV6,
+    f: F,
+) -> T
 where
-    R: Role<XdgSurfaceRole> + 'static,
-    F: FnOnce(&mut ToplevelState),
+    R: Role<XdgToplevelSurfaceRole> + 'static,
+    F: FnOnce(&mut XdgToplevelSurfaceRoleAttributes) -> T,
 {
     let data = toplevel
         .as_ref()
         .user_data()
         .get::<ShellSurfaceUserData<R>>()
         .unwrap();
-    data.shell_data
+    shell_data
         .compositor_token
-        .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| match data.pending_state {
-            XdgSurfacePendingState::Toplevel(ref mut toplevel_data) => f(toplevel_data),
-            _ => unreachable!(),
+        .with_role_data::<XdgToplevelSurfaceRole, _, _>(&data.wl_surface, |role| {
+            let attributes = role
+                .as_mut()
+                .expect("xdg_toplevel exists but role has been destroyed?!");
+            f(attributes)
         })
-        .expect("xdg_toplevel exists but surface has not shell_surface role?!");
+        .expect("xdg_toplevel exists but surface has not shell_surface role?!")
+}
+
+fn with_surface_toplevel_client_pending<R, F, T>(
+    shell_data: &ShellData<R>,
+    toplevel: &zxdg_toplevel_v6::ZxdgToplevelV6,
+    f: F,
+) -> T
+where
+    R: Role<XdgToplevelSurfaceRole> + 'static,
+    F: FnOnce(&mut ToplevelClientPending) -> T,
+{
+    with_surface_toplevel_role_data(shell_data, toplevel, |data| {
+        if data.client_pending.is_none() {
+            data.client_pending = Some(Default::default());
+        }
+        f(&mut data.client_pending.as_mut().unwrap())
+    })
 }
 
 pub fn send_toplevel_configure<R>(resource: &zxdg_toplevel_v6::ZxdgToplevelV6, configure: ToplevelConfigure)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + 'static,
 {
     let data = resource
         .as_ref()
         .user_data()
         .get::<ShellSurfaceUserData<R>>()
         .unwrap();
-    let (width, height) = configure.size.unwrap_or((0, 0));
+
+    let (width, height) = configure.state.size.unwrap_or((0, 0));
     // convert the Vec<State> (which is really a Vec<u32>) into Vec<u8>
     let states = {
-        let mut states = configure.states;
+        let mut states: Vec<xdg_toplevel::State> = configure.state.states.into();
         let ptr = states.as_mut_ptr();
         let len = states.len();
         let cap = states.capacity();
@@ -424,15 +527,13 @@ where
         unsafe { Vec::from_raw_parts(ptr as *mut u8, len * 4, cap * 4) }
     };
     let serial = configure.serial;
+
+    // Send the toplevel configure
     resource.configure(width, height, states);
+
+    // Send the base xdg_surface configure event to mark
+    // The configure as finished
     data.xdg_surface.configure(serial.into());
-    // Add the configure as pending
-    data.shell_data
-        .compositor_token
-        .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| {
-            data.pending_configures.push(serial.into())
-        })
-        .expect("xdg_toplevel exists but surface has not shell_surface role?!");
 }
 
 fn make_toplevel_handle<R: 'static>(
@@ -452,7 +553,7 @@ fn make_toplevel_handle<R: 'static>(
 
 fn toplevel_implementation<R>(request: zxdg_toplevel_v6::Request, toplevel: zxdg_toplevel_v6::ZxdgToplevelV6)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + 'static,
 {
     let data = toplevel
         .as_ref()
@@ -464,8 +565,8 @@ where
             // all it done by the destructor
         }
         zxdg_toplevel_v6::Request::SetParent { parent } => {
-            with_surface_toplevel_data::<R, _>(&toplevel, |toplevel_data| {
-                toplevel_data.parent = parent.map(|toplevel_surface_parent| {
+            with_surface_toplevel_role_data(&data.shell_data, &toplevel, |data| {
+                data.parent = parent.map(|toplevel_surface_parent| {
                     let parent_data = toplevel_surface_parent
                         .as_ref()
                         .user_data()
@@ -476,13 +577,15 @@ where
             });
         }
         zxdg_toplevel_v6::Request::SetTitle { title } => {
-            with_surface_toplevel_data::<R, _>(&toplevel, |toplevel_data| {
-                toplevel_data.title = title;
+            // Title is not double buffered, we can set it directly
+            with_surface_toplevel_role_data(&data.shell_data, &toplevel, |data| {
+                data.title = Some(title);
             });
         }
         zxdg_toplevel_v6::Request::SetAppId { app_id } => {
-            with_surface_toplevel_data::<R, _>(&toplevel, |toplevel_data| {
-                toplevel_data.app_id = app_id;
+            // AppId is not double buffered, we can set it directly
+            with_surface_toplevel_role_data(&data.shell_data, &toplevel, |role| {
+                role.app_id = Some(app_id);
             });
         }
         zxdg_toplevel_v6::Request::ShowWindowMenu { seat, serial, x, y } => {
@@ -520,13 +623,13 @@ where
             });
         }
         zxdg_toplevel_v6::Request::SetMaxSize { width, height } => {
-            with_surface_toplevel_data::<R, _>(&toplevel, |toplevel_data| {
-                toplevel_data.max_size = (width, height);
+            with_surface_toplevel_client_pending(&data.shell_data, &toplevel, |toplevel_data| {
+                toplevel_data.max_size = Some((width, height));
             });
         }
         zxdg_toplevel_v6::Request::SetMinSize { width, height } => {
-            with_surface_toplevel_data::<R, _>(&toplevel, |toplevel_data| {
-                toplevel_data.min_size = (width, height);
+            with_surface_toplevel_client_pending(&data.shell_data, &toplevel, |toplevel_data| {
+                toplevel_data.min_size = Some((width, height));
             });
         }
         zxdg_toplevel_v6::Request::SetMaximized => {
@@ -553,6 +656,8 @@ where
             (&mut *user_impl)(XdgRequest::UnFullscreen { surface: handle });
         }
         zxdg_toplevel_v6::Request::SetMinimized => {
+            // This has to be handled by the compositor, may not be
+            // supported and just ignored
             let handle = make_toplevel_handle(&toplevel);
             let mut user_impl = data.shell_data.user_impl.borrow_mut();
             (&mut *user_impl)(XdgRequest::Minimize { surface: handle });
@@ -563,7 +668,7 @@ where
 
 fn destroy_toplevel<R>(toplevel: zxdg_toplevel_v6::ZxdgToplevelV6)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgToplevelSurfaceRole> + 'static,
 {
     let data = toplevel
         .as_ref()
@@ -577,9 +682,8 @@ where
     } else {
         data.shell_data
             .compositor_token
-            .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| {
-                data.pending_state = XdgSurfacePendingState::None;
-                data.configured = false;
+            .with_role_data(&data.wl_surface, |role_data| {
+                *role_data = XdgToplevelSurfaceRole::None;
             })
             .expect("xdg_toplevel exists but surface has not shell_surface role?!");
     }
@@ -598,25 +702,23 @@ where
 
 pub(crate) fn send_popup_configure<R>(resource: &zxdg_popup_v6::ZxdgPopupV6, configure: PopupConfigure)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgPopupSurfaceRole> + 'static,
 {
     let data = resource
         .as_ref()
         .user_data()
         .get::<ShellSurfaceUserData<R>>()
         .unwrap();
-    let (x, y) = configure.position;
-    let (width, height) = configure.size;
+
     let serial = configure.serial;
-    resource.configure(x, y, width, height);
+    let geometry = configure.state.geometry;
+
+    // Send the popup configure
+    resource.configure(geometry.x, geometry.y, geometry.width, geometry.height);
+
+    // Send the base xdg_surface configure event to mark
+    // the configure as finished
     data.xdg_surface.configure(serial.into());
-    // Add the configure as pending
-    data.shell_data
-        .compositor_token
-        .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| {
-            data.pending_configures.push(serial.into())
-        })
-        .expect("xdg_toplevel exists but surface has not shell_surface role?!");
 }
 
 fn make_popup_handle<R: 'static>(resource: &zxdg_popup_v6::ZxdgPopupV6) -> super::PopupSurface<R> {
@@ -634,7 +736,7 @@ fn make_popup_handle<R: 'static>(resource: &zxdg_popup_v6::ZxdgPopupV6) -> super
 
 fn popup_implementation<R>(request: zxdg_popup_v6::Request, popup: zxdg_popup_v6::ZxdgPopupV6)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgPopupSurfaceRole> + 'static,
 {
     let data = popup
         .as_ref()
@@ -661,7 +763,7 @@ where
 
 fn destroy_popup<R>(popup: zxdg_popup_v6::ZxdgPopupV6)
 where
-    R: Role<XdgSurfaceRole> + 'static,
+    R: Role<XdgPopupSurfaceRole> + 'static,
 {
     let data = popup
         .as_ref()
@@ -675,9 +777,8 @@ where
     } else {
         data.shell_data
             .compositor_token
-            .with_role_data::<XdgSurfaceRole, _, _>(&data.wl_surface, |data| {
-                data.pending_state = XdgSurfacePendingState::None;
-                data.configured = false;
+            .with_role_data(&data.wl_surface, |role_data| {
+                *role_data = XdgPopupSurfaceRole::None;
             })
             .expect("xdg_popup exists but surface has not shell_surface role?!");
     }