Merge pull request #299 from kas-gui/work3

Restore configure_recurse; use local reconfigure for dynamic parents

crates/kas-core/src/core/widget.rs

@@ -110,26 +110,14 @@ pub trait WidgetChildren: WidgetCore {
     /// This method may be removed in the future.
     fn get_child_mut(&mut self, index: usize) -> Option<&mut dyn WidgetConfig>;
 
-    /// Get the [`WidgetId`] for this child
-    ///
-    /// Note: the result should be generated relative to `self.id`.
-    /// Most widgets may use the default implementation.
-    ///
-    /// This must return `Some(..)` when `index` is valid; in other cases the
-    /// result does not matter.
-    ///
-    /// If a custom implementation is used, then [`Self::find_child_index`]
-    /// must be implemented to do the inverse of `make_child_id`, and
-    /// probably a custom implementation of [`Layout::spatial_nav`] is needed.
-    #[inline]
-    fn make_child_id(&self, index: usize) -> Option<WidgetId> {
-        Some(self.id_ref().make_child(index))
-    }
-
     /// Find the child which is an ancestor of this `id`, if any
     ///
     /// If `Some(index)` is returned, this is *probably* but not guaranteed
     /// to be a valid child index.
+    ///
+    /// The default implementation simply uses [`WidgetId::next_key_after`].
+    /// Widgets may choose to assign children custom keys by overriding this
+    /// method and [`WidgetConfig::configure_recurse`].
     #[inline]
     fn find_child_index(&self, id: &WidgetId) -> Option<usize> {
         id.next_key_after(self.id_ref())
@@ -142,59 +130,51 @@ pub trait WidgetChildren: WidgetCore {
 /// [`derive(Widget)`] unless explicitly implemented.
 ///
 /// Widgets are *configured* on window creation or dynamically via the
-/// parent calling [`SetRectMgr::configure`]. Configuration may be repeated.
-///
-/// Configuration is required to happen before [`Layout::size_rules`] is first
-/// called, thus [`Self::configure`] may be used to load assets required by
-/// [`Layout::size_rules`].
-///
-/// Configuration happens in this order:
+/// parent calling [`SetRectMgr::configure`]. Parent widgets are responsible
+/// for ensuring that children are configured before calling
+/// [`Layout::size_rules`]. Configuration may be repeated and may be used as a
+/// mechanism to change a child's [`WidgetId`], but this may be expensive.
 ///
-/// 1.  [`Self::pre_configure`] is used to assign a [`WidgetId`] and may
-///     influence configuration of children.
-/// 2.  Configuration of children accessible through [`WidgetChildren`].
-/// 3.  [`Self::configure`] allows user-configuration of event handling and
-///     may be used for resource loading.
-///
-/// Note that if parent widgets add child widgets dynamically, that parent is
-/// responsible for ensuring that the new child widgets gets configured. This
-/// may be done (1) by adding the children during [`Self::pre_configure`], (2)
-/// by requesting [`TkAction::RECONFIGURE`] or (3) by calling
-/// [`SetRectMgr::configure`]. The latter may also be used to change a child's
-/// [`WidgetId`].
+/// Configuration invokes [`Self::configure_recurse`] which then calls
+/// [`Self::configure`]. The latter may be used to load assets before sizing.
 ///
 /// [`derive(Widget)`]: https://docs.rs/kas/latest/kas/macros/index.html#the-derivewidget-macro
-//
-// TODO(specialization): provide a blanket implementation, so that users only
-// need implement manually when they have something to configure.
 #[autoimpl(for<T: trait + ?Sized> Box<T>)]
 pub trait WidgetConfig: Layout {
-    /// Pre-configure widget
+    /// Configure widget and children
     ///
-    /// This method is part of configuration (see trait documentation).
+    /// This method:
     ///
-    /// This method assigns the widget's [`WidgetId`] and may be used to
-    /// affect the manager in ways which influence the child, for example
-    /// [`EventState::new_accel_layer`]. Custom implementations should be aware
-    /// that children may not have been configured and thus may have invalid
-    /// identifiers at the time of calling.
+    /// 1.  Assigns `id` to self
+    /// 2.  Constructs an identifier for and call `configure_recurse` on each child
+    /// 3.  Calls [`Self::configure`]
     ///
-    /// The window's scale factor (and thus any sizes available through
-    /// [`SetRectMgr::size_mgr`]) may not be correct initially (some platforms
-    /// construct all windows using scale factor 1) and/or may change in the
-    /// future. Changes to the scale factor result in recalculation of
-    /// [`Layout::size_rules`] but not repeated configuration.
-    fn pre_configure(&mut self, mgr: &mut SetRectMgr, id: WidgetId) {
-        let _ = mgr;
+    /// Normally the default implementation is used. A custom implementation
+    /// may be used to influence configuration of children, for example by
+    /// calling [`EventState::new_accel_layer`] or by constructing children's
+    /// [`WidgetId`] values in a non-standard manner (in this case ensure that
+    /// [`WidgetChildren::find_child_index`] has a correct implementation).
+    ///
+    /// To directly configure a child, call [`SetRectMgr::configure`] instead.
+    fn configure_recurse(&mut self, mgr: &mut SetRectMgr, id: WidgetId) {
         self.core_data_mut().id = id;
+
+        for index in 0..self.num_children() {
+            let id = self.id_ref().make_child(index);
+            if let Some(widget) = self.get_child_mut(index) {
+                widget.configure_recurse(mgr, id);
+            }
+        }
+
+        self.configure(mgr);
     }
 
     /// Configure widget
     ///
     /// This method is part of configuration (see trait documentation).
     ///
     /// This method may be used to configure event handling and to load
-    /// resources.
+    /// resources, including resources affecting [`Layout::size_rules`].
     ///
     /// The window's scale factor (and thus any sizes available through
     /// [`SetRectMgr::size_mgr`]) may not be correct initially (some platforms
@@ -240,9 +220,14 @@ pub trait WidgetConfig: Layout {
 /// This trait is part of the [`Widget`] family. It may be derived by
 /// the [`crate::macros::widget`] macro, but is not by default.
 ///
-/// Implementations of this trait should *either* define [`Self::layout`]
-/// (optionally with other methods as required) *or* define at least
-/// [`Self::size_rules`] and [`Self::draw`].
+/// There are two methods of implementing this trait:
+///
+/// -   Implement [`Self::layout`]. This alone suffices in many cases; other
+///     methods may be overridden if necessary.
+/// -   Ignore [`Self::layout`] and implement [`Self::size_rules`] (to give the
+///     widget size) and [`Self::draw`] (to make it show something). Other
+///     methods may be required (e.g. [`Self::set_rect`] to position child
+///     elements).
 ///
 /// Layout solving happens in two steps:
 ///
@@ -262,8 +247,9 @@ pub trait Layout: WidgetChildren {
     ///
     /// The default implementation is for an empty layout (zero size required,
     /// no child elements, no graphics).
+    #[inline]
     fn layout(&mut self) -> layout::Layout<'_> {
-        Default::default() // TODO: remove default impl
+        Default::default()
     }
 
     /// Get size rules for the given axis
@@ -446,7 +432,7 @@ pub trait WidgetExt: WidgetChildren {
     ///
     /// Note that the default-constructed [`WidgetId`] is *invalid*: any
     /// operations on this value will cause a panic. Valid identifiers are
-    /// assigned by [`WidgetConfig::pre_configure`].
+    /// assigned by [`WidgetConfig::configure_recurse`].
     #[inline]
     fn id(&self) -> WidgetId {
         self.core_data().id.clone()
@@ -456,7 +442,7 @@ pub trait WidgetExt: WidgetChildren {
     ///
     /// Note that the default-constructed [`WidgetId`] is *invalid*: any
     /// operations on this value will cause a panic. Valid identifiers are
-    /// assigned by [`WidgetConfig::pre_configure`].
+    /// assigned by [`WidgetConfig::configure_recurse`].
     #[inline]
     fn id_ref(&self) -> &WidgetId {
         &self.core_data().id

crates/kas-core/src/event/manager.rs

@@ -147,8 +147,6 @@ type AccelLayer = (bool, HashMap<VirtualKeyCode, WidgetId>);
 pub struct EventState {
     config: WindowConfig,
     disabled: Vec<WidgetId>,
-    configure_active: bool,
-    configure_count: usize,
     window_has_focus: bool,
     modifiers: ModifiersState,
     /// char focus is on same widget as sel_focus; otherwise its value is ignored

crates/kas-core/src/event/manager/mgr_shell.rs

@@ -33,8 +33,6 @@ impl EventState {
         EventState {
             config: WindowConfig::new(config, scale_factor),
             disabled: vec![],
-            configure_active: false,
-            configure_count: 0,
             window_has_focus: false,
             modifiers: ModifiersState::empty(),
             char_focus: false,
@@ -66,52 +64,6 @@ impl EventState {
         self.config.set_scale_factor(scale_factor);
     }
 
-    /// Configure a widget
-    ///
-    /// All widgets must be configured after construction (see
-    /// [`WidgetConfig::configure`]). This method may be used to configure a new
-    /// child widget without requiring the whole window to be reconfigured.
-    ///
-    /// Pass the `id` to assign to the widget: this should be constructed from
-    /// the parent's id via [`WidgetId::make_child`].
-    pub fn configure(mgr: &mut SetRectMgr, id: WidgetId, widget: &mut dyn WidgetConfig) {
-        assert!(!mgr.ev.configure_active, "configure recursion");
-        if mgr.ev.action.contains(TkAction::RECONFIGURE) {
-            return; // whole window will be reconfigured
-        }
-        mgr.ev.configure_active = true;
-
-        fn recurse(
-            mgr: &mut SetRectMgr,
-            id: WidgetId,
-            widget: &mut dyn WidgetConfig,
-            count: &mut usize,
-        ) {
-            widget.pre_configure(mgr, id);
-            *count += 1;
-            for i in 0..widget.num_children() {
-                if let Some(id) = widget.make_child_id(i) {
-                    if let Some(w) = widget.get_child_mut(i) {
-                        recurse(mgr, id, w, count);
-                    }
-                }
-            }
-            widget.configure(mgr);
-        }
-
-        let mut count = 0;
-        recurse(mgr, id, widget, &mut count);
-
-        if mgr.ev.action.contains(TkAction::RECONFIGURE) {
-            log::warn!("Detected TkAction::RECONFIGURE during configure. This may cause a reconfigure-loop.");
-            if count == mgr.ev.configure_count {
-                panic!("Reconfigure occurred with the same number of widgets — we are probably stuck in a reconfigure-loop.");
-            }
-        }
-        mgr.ev.configure_count = count;
-        mgr.ev.configure_active = false;
-    }
-
     /// Configure event manager for a widget tree.
     ///
     /// This should be called by the toolkit on the widget tree when the window
@@ -136,7 +88,7 @@ impl EventState {
 
         shell.size_and_draw_shared(&mut |size_handle, draw_shared| {
             let mut mgr = SetRectMgr::new(size_handle, draw_shared, self);
-            Self::configure(&mut mgr, WidgetId::ROOT, widget.as_widget_mut());
+            mgr.configure(WidgetId::ROOT, widget.as_widget_mut());
         });
 
         let hover = widget.find_id(self.last_mouse_coord);

crates/kas-core/src/layout/mod.rs

@@ -198,7 +198,9 @@ impl<'a> SetRectMgr<'a> {
     /// the parent's id via [`WidgetId::make_child`].
     #[inline]
     pub fn configure(&mut self, id: WidgetId, widget: &mut dyn WidgetConfig) {
-        EventState::configure(self, id, widget);
+        // Yes, this method is just a shim! We reserve the option to add other code here in the
+        // future, hence do not advise calling `configure_recurse` directly.
+        widget.configure_recurse(self, id);
     }
 }
 

crates/kas-core/src/toolkit.rs

@@ -46,8 +46,9 @@ bitflags! {
     /// assignments are supported by both `TkAction` and [`event::EventMgr`].
     ///
     /// Users receiving a value of this type from a widget update method should
-    /// generally call `*mgr |= action;` during event handling. Prior to
-    /// starting the event loop (`toolkit.run()`), these values can be ignored.
+    /// usually handle with `*mgr |= action;`. Before the event loop starts
+    /// (`toolkit.run()`) or if the widget in question is not part of a UI these
+    /// values can be ignored.
     #[must_use]
     #[derive(Default)]
     pub struct TkAction: u32 {
@@ -70,7 +71,7 @@ bitflags! {
         */
         /// Reset size of all widgets without recalculating requirements
         const SET_SIZE = 1 << 8;
-        /// Resize all widgets
+        /// Resize all widgets in the window
         const RESIZE = 1 << 9;
         /// Update theme memory
         const THEME_UPDATE = 1 << 10;

crates/kas-core/src/updatable/data_traits.rs

@@ -105,6 +105,11 @@ pub trait ListData: Debug {
     /// increase the version number (allowing change detection).
     fn version(&self) -> u64;
 
+    /// No data is available
+    fn is_empty(&self) -> bool {
+        self.len() == 0
+    }
+
     /// Number of data items available
     ///
     /// Note: users may assume this is `O(1)`.

crates/kas-widgets/src/combobox.rs

@@ -190,23 +190,26 @@ impl ComboBox<VoidMsg> {
     /// types, and the chosen `active` entry. For example:
     /// ```
     /// # use kas_widgets::ComboBox;
-    /// let combobox = ComboBox::new(&["zero", "one", "two"], 0);
+    /// let combobox = ComboBox::new_from_iter(&["zero", "one", "two"], 0);
     /// ```
     #[inline]
-    pub fn new<T: Into<AccelString>, I: IntoIterator<Item = T>>(iter: I, active: usize) -> Self {
+    pub fn new_from_iter<T: Into<AccelString>, I: IntoIterator<Item = T>>(
+        iter: I,
+        active: usize,
+    ) -> Self {
         let entries = iter
             .into_iter()
             .map(|label| MenuEntry::new(label, ()))
             .collect();
-        Self::new_entries(entries, active)
+        Self::new(entries, active)
     }
 
     /// Construct a combobox with the given menu entries
     ///
     /// A combobox presents a menu with a fixed set of choices when clicked,
     /// with the `active` choice selected (0-based index).
     #[inline]
-    pub fn new_entries(entries: Vec<MenuEntry<()>>, active: usize) -> Self {
+    pub fn new(entries: Vec<MenuEntry<()>>, active: usize) -> Self {
         let label = entries.get(active).map(|entry| entry.get_string());
         let label = Text::new_single(label.unwrap_or("".to_string()));
         ComboBox {
@@ -290,54 +293,46 @@ impl<M: 'static> ComboBox<M> {
     }
 
     /// Remove all choices
-    ///
-    /// Triggers a [reconfigure action](EventState::send_action).
-    pub fn clear(&mut self) -> TkAction {
+    pub fn clear(&mut self) {
         self.popup.inner.clear()
     }
 
     /// Add a choice to the combobox, in last position
     ///
-    /// Triggers a [reconfigure action](EventState::send_action).
-    pub fn push<T: Into<AccelString>>(&mut self, label: T) -> TkAction {
+    /// Returns the index of the new choice
+    //
+    // TODO(opt): these methods cause full-window resize. They don't need to
+    // resize at all if the menu is closed!
+    pub fn push<T: Into<AccelString>>(&mut self, mgr: &mut SetRectMgr, label: T) -> usize {
         let column = &mut self.popup.inner;
-        column.push(MenuEntry::new(label, ()))
-        // TODO: localised reconfigure
+        column.push(mgr, MenuEntry::new(label, ()))
     }
 
     /// Pops the last choice from the combobox
-    ///
-    /// Triggers a [reconfigure action](EventState::send_action).
-    pub fn pop(&mut self) -> (Option<()>, TkAction) {
-        let r = self.popup.inner.pop();
-        (r.0.map(|_| ()), r.1)
+    pub fn pop(&mut self, mgr: &mut SetRectMgr) -> Option<()> {
+        self.popup.inner.pop(mgr).map(|_| ())
     }
 
     /// Add a choice at position `index`
     ///
     /// Panics if `index > len`.
-    ///
-    /// Triggers a [reconfigure action](EventState::send_action).
-    pub fn insert<T: Into<AccelString>>(&mut self, index: usize, label: T) -> TkAction {
+    pub fn insert<T: Into<AccelString>>(&mut self, mgr: &mut SetRectMgr, index: usize, label: T) {
         let column = &mut self.popup.inner;
-        column.insert(index, MenuEntry::new(label, ()))
-        // TODO: localised reconfigure
+        column.insert(mgr, index, MenuEntry::new(label, ()));
     }
 
     /// Removes the choice at position `index`
     ///
     /// Panics if `index` is out of bounds.
-    ///
-    /// Triggers a [reconfigure action](EventState::send_action).
-    pub fn remove(&mut self, index: usize) -> TkAction {
-        self.popup.inner.remove(index).1
+    pub fn remove(&mut self, mgr: &mut SetRectMgr, index: usize) {
+        self.popup.inner.remove(mgr, index);
     }
 
     /// Replace the choice at `index`
     ///
     /// Panics if `index` is out of bounds.
-    pub fn replace<T: Into<AccelString>>(&mut self, index: usize, label: T) -> TkAction {
-        self.popup.inner[index].set_accel(label)
+    pub fn replace<T: Into<AccelString>>(&mut self, mgr: &mut SetRectMgr, index: usize, label: T) {
+        *mgr |= self.popup.inner[index].set_accel(label);
     }
 }