refactor: popup id with callbacks

adapters/src/rendering/femtovg/popup_window.rs

@@ -1,5 +1,5 @@
 use crate::errors::{RenderingError, Result};
-use crate::wayland::surfaces::popup_manager::PopupManager;
+use crate::wayland::surfaces::popup_manager::{OnCloseCallback, PopupId};
 use core::ops::Deref;
 use log::info;
 use slint::{
@@ -7,7 +7,7 @@ use slint::{
     platform::{Renderer, WindowAdapter, WindowEvent, femtovg_renderer::FemtoVGRenderer},
 };
 use slint_interpreter::ComponentInstance;
-use std::cell::{Cell, OnceCell, RefCell};
+use std::cell::{Cell, OnceCell};
 use std::rc::{Rc, Weak};
 
 use super::main_window::RenderState;
@@ -19,13 +19,12 @@ pub struct PopupWindow {
     render_state: Cell<RenderState>,
     size: Cell<PhysicalSize>,
     scale_factor: Cell<f32>,
-    popup_manager: RefCell<Weak<PopupManager>>,
-    popup_key: Cell<Option<usize>>,
+    popup_id: Cell<Option<PopupId>>,
+    on_close: OnceCell<OnCloseCallback>,
     configured: Cell<bool>,
     component_instance: OnceCell<ComponentInstance>,
 }
 
-#[allow(dead_code)]
 impl PopupWindow {
     #[must_use]
     pub fn new(renderer: FemtoVGRenderer) -> Rc<Self> {
@@ -37,17 +36,23 @@ impl PopupWindow {
                 render_state: Cell::new(RenderState::Clean),
                 size: Cell::new(PhysicalSize::default()),
                 scale_factor: Cell::new(1.),
-                popup_manager: RefCell::new(Weak::new()),
-                popup_key: Cell::new(None),
+                popup_id: Cell::new(None),
+                on_close: OnceCell::new(),
                 configured: Cell::new(false),
                 component_instance: OnceCell::new(),
             }
         })
     }
 
-    pub fn set_popup_manager(&self, popup_manager: Weak<PopupManager>, key: usize) {
-        *self.popup_manager.borrow_mut() = popup_manager;
-        self.popup_key.set(Some(key));
+    #[must_use]
+    pub fn new_with_callback(renderer: FemtoVGRenderer, on_close: OnCloseCallback) -> Rc<Self> {
+        let window = Self::new(renderer);
+        window.on_close.set(on_close).ok();
+        window
+    }
+
+    pub fn set_popup_id(&self, id: PopupId) {
+        self.popup_id.set(Some(id));
     }
 
     pub fn close_popup(&self) {
@@ -57,15 +62,14 @@ impl PopupWindow {
             info!("Failed to hide popup window: {e}");
         }
 
-        if let Some(popup_manager) = self.popup_manager.borrow().upgrade() {
-            if let Some(key) = self.popup_key.get() {
-                info!("Destroying popup with key {key}");
-                popup_manager.destroy_popup(key);
+        if let Some(id) = self.popup_id.get() {
+            info!("Destroying popup with id {:?}", id);
+            if let Some(on_close) = self.on_close.get() {
+                on_close(id);
             }
         }
 
-        *self.popup_manager.borrow_mut() = Weak::new();
-        self.popup_key.set(None);
+        self.popup_id.set(None);
 
         info!("Popup window cleanup complete");
     }
@@ -107,7 +111,7 @@ impl PopupWindow {
     }
 
     pub fn popup_key(&self) -> Option<usize> {
-        self.popup_key.get()
+        self.popup_id.get().map(PopupId::key)
     }
 
     pub fn mark_configured(&self) {

adapters/src/wayland/services/popup_service.rs

@@ -1,5 +1,6 @@
 use crate::errors::Result;
 use crate::rendering::femtovg::popup_window::PopupWindow;
+use crate::wayland::surfaces::popup_manager::PopupId;
 use layer_shika_domain::value_objects::popup_request::{PopupHandle, PopupRequest};
 use log::info;
 use slint::PhysicalSize;
@@ -37,7 +38,8 @@ impl PopupService {
     }
 
     pub fn close(&self, handle: PopupHandle) -> Result<()> {
-        self.manager.destroy_popup(handle.key());
+        let id = PopupId::from_key(handle.key());
+        self.manager.destroy_popup(id);
         Ok(())
     }
 

adapters/src/wayland/surfaces/popup_manager.rs

@@ -5,10 +5,10 @@ use layer_shika_domain::value_objects::popup_config::PopupConfig;
 use layer_shika_domain::value_objects::popup_positioning_mode::PopupPositioningMode;
 use layer_shika_domain::value_objects::popup_request::PopupRequest;
 use log::info;
-use slab::Slab;
 use slint::{platform::femtovg_renderer::FemtoVGRenderer, PhysicalSize, WindowSize};
 use smithay_client_toolkit::reexports::protocols_wlr::layer_shell::v1::client::zwlr_layer_surface_v1::ZwlrLayerSurfaceV1;
 use std::cell::RefCell;
+use std::collections::HashMap;
 use std::rc::Rc;
 use wayland_client::{
     backend::ObjectId,
@@ -22,6 +22,23 @@ use wayland_protocols::xdg::shell::client::xdg_wm_base::XdgWmBase;
 use super::popup_surface::PopupSurface;
 use super::surface_state::WindowState;
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+pub struct PopupId(pub(crate) usize);
+
+impl PopupId {
+    #[must_use]
+    pub const fn key(self) -> usize {
+        self.0
+    }
+
+    #[must_use]
+    pub const fn from_key(key: usize) -> Self {
+        Self(key)
+    }
+}
+
+pub type OnCloseCallback = Box<dyn Fn(PopupId)>;
+
 #[derive(Debug, Clone, Copy)]
 pub struct CreatePopupParams {
     pub last_pointer_serial: u32,
@@ -83,21 +100,23 @@ struct PendingPopup {
 }
 
 struct PopupManagerState {
-    popups: Slab<ActivePopup>,
+    popups: HashMap<PopupId, ActivePopup>,
     scale_factor: f32,
     output_size: PhysicalSize,
-    current_popup_key: Option<usize>,
+    current_popup_id: Option<PopupId>,
     pending_popup: Option<PendingPopup>,
+    id_generator: usize,
 }
 
 impl PopupManagerState {
     fn new(initial_scale_factor: f32) -> Self {
         Self {
-            popups: Slab::new(),
+            popups: HashMap::new(),
             scale_factor: initial_scale_factor,
             output_size: PhysicalSize::new(0, 0),
-            current_popup_key: None,
+            current_popup_id: None,
             pending_popup: None,
+            id_generator: 0,
         }
     }
 }
@@ -152,15 +171,15 @@ impl PopupManager {
     }
 
     pub fn close_current_popup(&self) {
-        let key = self.state.borrow_mut().current_popup_key.take();
-        if let Some(key) = key {
-            self.destroy_popup(key);
+        let id = self.state.borrow_mut().current_popup_id.take();
+        if let Some(id) = id {
+            self.destroy_popup(id);
         }
     }
 
     #[must_use]
     pub fn current_popup_key(&self) -> Option<usize> {
-        self.state.borrow().current_popup_key
+        self.state.borrow().current_popup_id.map(PopupId::key)
     }
 
     pub fn create_popup(
@@ -235,7 +254,24 @@ impl PopupManager {
         let renderer = FemtoVGRenderer::new(context)
             .map_err(|e| LayerShikaError::FemtoVGRendererCreation { source: e })?;
 
-        let popup_window = PopupWindow::new(renderer);
+        let popup_id = {
+            let mut state = self.state.borrow_mut();
+            let id = PopupId(state.id_generator);
+            state.id_generator += 1;
+            id
+        };
+
+        let on_close: OnCloseCallback = {
+            let weak_self = Rc::downgrade(self);
+            Box::new(move |id: PopupId| {
+                if let Some(manager) = weak_self.upgrade() {
+                    manager.destroy_popup(id);
+                }
+            })
+        };
+
+        let popup_window = PopupWindow::new_with_callback(renderer, on_close);
+        popup_window.set_popup_id(popup_id);
         popup_window.set_scale_factor(scale_factor);
         popup_window.set_size(WindowSize::Logical(slint::LogicalSize::new(
             params.width,
@@ -243,23 +279,25 @@ impl PopupManager {
         )));
 
         let mut state = self.state.borrow_mut();
-        let key = state.popups.insert(ActivePopup {
-            surface: popup_surface,
-            window: Rc::clone(&popup_window),
-            request,
-            last_serial: params.last_pointer_serial,
-        });
-        popup_window.set_popup_manager(Rc::downgrade(self), key);
-        state.current_popup_key = Some(key);
+        state.popups.insert(
+            popup_id,
+            ActivePopup {
+                surface: popup_surface,
+                window: Rc::clone(&popup_window),
+                request,
+                last_serial: params.last_pointer_serial,
+            },
+        );
+        state.current_popup_id = Some(popup_id);
 
-        info!("Popup window created successfully with key {key}");
+        info!("Popup window created successfully with id {:?}", popup_id);
 
         Ok(popup_window)
     }
 
     pub fn render_popups(&self) -> Result<()> {
         let state = self.state.borrow();
-        for (_key, popup) in &state.popups {
+        for popup in state.popups.values() {
             popup.window.render_frame_if_dirty()?;
         }
         Ok(())
@@ -271,7 +309,7 @@ impl PopupManager {
 
     pub fn mark_all_popups_dirty(&self) {
         let state = self.state.borrow();
-        for (_key, popup) in &state.popups {
+        for popup in state.popups.values() {
             popup.window.request_redraw();
         }
     }
@@ -281,55 +319,55 @@ impl PopupManager {
             .borrow()
             .popups
             .iter()
-            .find_map(|(key, popup)| (popup.surface.surface.id() == *surface_id).then_some(key))
+            .find_map(|(id, popup)| (popup.surface.surface.id() == *surface_id).then_some(id.key()))
     }
 
     pub fn find_popup_key_by_fractional_scale_id(
         &self,
         fractional_scale_id: &ObjectId,
     ) -> Option<usize> {
-        self.state.borrow().popups.iter().find_map(|(key, popup)| {
+        self.state.borrow().popups.iter().find_map(|(id, popup)| {
             popup
                 .surface
                 .fractional_scale
                 .as_ref()
                 .filter(|fs| fs.id() == *fractional_scale_id)
-                .map(|_| key)
+                .map(|_| id.key())
         })
     }
 
     pub fn get_popup_window(&self, key: usize) -> Option<Rc<PopupWindow>> {
+        let id = PopupId(key);
         self.state
             .borrow()
             .popups
-            .get(key)
+            .get(&id)
             .map(|popup| Rc::clone(&popup.window))
     }
 
-    pub fn destroy_popup(&self, key: usize) {
-        if let Some(popup) = self.state.borrow_mut().popups.try_remove(key) {
-            info!("Destroying popup with key {key}");
+    pub fn destroy_popup(&self, id: PopupId) {
+        if let Some(popup) = self.state.borrow_mut().popups.remove(&id) {
+            info!("Destroying popup with id {:?}", id);
 
             popup.surface.destroy();
         }
     }
 
     pub fn find_popup_key_by_xdg_popup_id(&self, xdg_popup_id: &ObjectId) -> Option<usize> {
-        self.state
-            .borrow()
-            .popups
-            .iter()
-            .find_map(|(key, popup)| (popup.surface.xdg_popup.id() == *xdg_popup_id).then_some(key))
+        self.state.borrow().popups.iter().find_map(|(id, popup)| {
+            (popup.surface.xdg_popup.id() == *xdg_popup_id).then_some(id.key())
+        })
     }
 
     pub fn find_popup_key_by_xdg_surface_id(&self, xdg_surface_id: &ObjectId) -> Option<usize> {
-        self.state.borrow().popups.iter().find_map(|(key, popup)| {
-            (popup.surface.xdg_surface.id() == *xdg_surface_id).then_some(key)
+        self.state.borrow().popups.iter().find_map(|(id, popup)| {
+            (popup.surface.xdg_surface.id() == *xdg_surface_id).then_some(id.key())
         })
     }
 
     pub fn update_popup_viewport(&self, key: usize, logical_width: i32, logical_height: i32) {
-        if let Some(popup) = self.state.borrow().popups.get(key) {
+        let id = PopupId(key);
+        if let Some(popup) = self.state.borrow().popups.get(&id) {
             popup
                 .surface
                 .update_viewport_size(logical_width, logical_height);
@@ -337,15 +375,17 @@ impl PopupManager {
     }
 
     pub fn get_popup_info(&self, key: usize) -> Option<(PopupRequest, u32)> {
+        let id = PopupId(key);
         self.state
             .borrow()
             .popups
-            .get(key)
+            .get(&id)
             .map(|popup| (popup.request.clone(), popup.last_serial))
     }
 
     pub fn mark_popup_configured(&self, key: usize) {
-        if let Some(popup) = self.state.borrow().popups.get(key) {
+        let id = PopupId(key);
+        if let Some(popup) = self.state.borrow().popups.get(&id) {
             popup.window.mark_configured();
         }
     }

composition/src/system.rs

@@ -11,7 +11,10 @@ use layer_shika_adapters::platform::slint_interpreter::{
 use layer_shika_adapters::wayland::{
     config::WaylandWindowConfig,
     shell_adapter::WaylandWindowingSystem,
-    surfaces::{popup_manager::PopupManager, surface_state::WindowState},
+    surfaces::{
+        popup_manager::{PopupId, PopupManager},
+        surface_state::WindowState,
+    },
 };
 use layer_shika_domain::config::WindowConfig;
 use layer_shika_domain::errors::DomainError;
@@ -19,6 +22,7 @@ use layer_shika_domain::value_objects::popup_positioning_mode::PopupPositioningM
 use layer_shika_domain::value_objects::popup_request::{
     PopupAt, PopupHandle, PopupRequest, PopupSize,
 };
+use std::cell::Cell;
 use std::cell::{Ref, RefCell};
 use std::os::unix::io::AsFd;
 use std::rc::{Rc, Weak};
@@ -196,7 +200,8 @@ impl RuntimeState<'_> {
 
         popup_manager.set_pending_popup(req, initial_dimensions.0, initial_dimensions.1);
 
-        let instance = Self::create_popup_instance(&definition, &popup_manager, 0, resize_sender)?;
+        let (instance, popup_key_cell) =
+            Self::create_popup_instance(&definition, &popup_manager, resize_sender)?;
 
         let popup_key = popup_manager.current_popup_key().ok_or_else(|| {
             Error::Domain(DomainError::Configuration {
@@ -204,6 +209,8 @@ impl RuntimeState<'_> {
             })
         })?;
 
+        popup_key_cell.set(popup_key);
+
         if let Some(popup_window) = popup_manager.get_popup_window(popup_key) {
             popup_window.set_component_instance(instance);
         } else {
@@ -217,7 +224,8 @@ impl RuntimeState<'_> {
 
     pub fn close_popup(&mut self, handle: PopupHandle) -> Result<()> {
         if let Some(popup_manager) = self.window_state.popup_manager() {
-            popup_manager.destroy_popup(handle.key());
+            let id = PopupId::from_key(handle.key());
+            popup_manager.destroy_popup(id);
         }
         Ok(())
     }
@@ -247,11 +255,13 @@ impl RuntimeState<'_> {
             })
             .map(Rc::clone)?;
 
-        let (request, _serial) = popup_manager.get_popup_info(key).ok_or_else(|| {
-            Error::Domain(DomainError::Configuration {
-                message: format!("Popup with key {} not found", key),
-            })
-        })?;
+        let Some((request, _serial)) = popup_manager.get_popup_info(key) else {
+            log::debug!(
+                "Ignoring resize request for non-existent popup with key {}",
+                key
+            );
+            return Ok(());
+        };
 
         let current_size = request.size.dimensions();
         let size_changed =
@@ -288,9 +298,8 @@ impl RuntimeState<'_> {
     fn create_popup_instance(
         definition: &ComponentDefinition,
         popup_manager: &Rc<PopupManager>,
-        popup_key: usize,
         resize_sender: Option<channel::Sender<PopupCommand>>,
-    ) -> Result<ComponentInstance> {
+    ) -> Result<(ComponentInstance, Rc<Cell<usize>>)> {
         let instance = definition.create().map_err(|e| {
             Error::Domain(DomainError::Configuration {
                 message: format!("Failed to create popup instance: {}", e),
@@ -311,7 +320,10 @@ impl RuntimeState<'_> {
                 })
             })?;
 
+        let popup_key_cell = Rc::new(Cell::new(0));
+
         let result = if let Some(sender) = resize_sender {
+            let key_cell = Rc::clone(&popup_key_cell);
             instance.set_callback("change_popup_size", move |args| {
                 let width: f32 = args
                     .first()
@@ -322,7 +334,13 @@ impl RuntimeState<'_> {
                     .and_then(|v| v.clone().try_into().ok())
                     .unwrap_or(150.0);
 
-                log::info!("change_popup_size callback invoked: {}x{}", width, height);
+                let popup_key = key_cell.get();
+                log::info!(
+                    "change_popup_size callback invoked: {}x{} for key {}",
+                    width,
+                    height,
+                    popup_key
+                );
 
                 if sender
                     .send(PopupCommand::Resize {
@@ -338,6 +356,7 @@ impl RuntimeState<'_> {
             })
         } else {
             let popup_manager_for_resize = Rc::downgrade(popup_manager);
+            let key_cell = Rc::clone(&popup_key_cell);
             instance.set_callback("change_popup_size", move |args| {
                 let width: f32 = args
                     .first()
@@ -348,7 +367,13 @@ impl RuntimeState<'_> {
                     .and_then(|v| v.clone().try_into().ok())
                     .unwrap_or(150.0);
 
-                log::info!("change_popup_size callback invoked: {}x{}", width, height);
+                let popup_key = key_cell.get();
+                log::info!(
+                    "change_popup_size callback invoked: {}x{} for key {}",
+                    width,
+                    height,
+                    popup_key
+                );
 
                 if let Some(popup_window) = popup_manager_for_resize
                     .upgrade()
@@ -372,7 +397,7 @@ impl RuntimeState<'_> {
             })
         })?;
 
-        Ok(instance)
+        Ok((instance, popup_key_cell))
     }
 }