From 952ec805ed3dba54c82d3aa481e93ed596a955eb Mon Sep 17 00:00:00 2001 From: Dorota Czaplejewicz Date: Sat, 1 Oct 2022 12:48:52 +0000 Subject: [PATCH] layout: Put all button state into active_buttons Ths gets rid of Rc> sharing of state, which can be hard to keep track of. In addition, there's no longer any duplication of button state. --- src/data/parsing.rs | 8 +- src/drawing.rs | 16 +- src/keyboard.rs | 20 +-- src/layout.rs | 362 ++++++++++++++++++++------------------------ 4 files changed, 188 insertions(+), 218 deletions(-) diff --git a/src/data/parsing.rs b/src/data/parsing.rs index 6586e567..7a7b93ad 100644 --- a/src/data/parsing.rs +++ b/src/data/parsing.rs @@ -4,12 +4,10 @@ /*! Parsing of the data files containing layouts */ -use std::cell::RefCell; use std::collections::{ HashMap, HashSet }; use std::ffi::CString; use std::fs; use std::path::PathBuf; -use std::rc::Rc; use std::vec::Vec; use xkbcommon::xkb; @@ -18,8 +16,7 @@ use super::{ Error, LoadError }; use ::action; use crate::keyboard::{ - Key, KeyState, PressType, - generate_keymaps, generate_keycodes, KeyCode, FormattingError + Key, generate_keymaps, generate_keycodes, KeyCode, FormattingError }; use ::layout; use ::logging; @@ -517,9 +514,6 @@ fn create_button( label: label, action: data.action, keycodes: data.keycodes, - state: Rc::new( - RefCell::new(KeyState { pressed: PressType::Released }) - ), } } diff --git a/src/drawing.rs b/src/drawing.rs index a87d1f5e..8a920c74 100644 --- a/src/drawing.rs +++ b/src/drawing.rs @@ -1,11 +1,10 @@ /*! Drawing the UI */ use cairo; -use std::cell::RefCell; use ::action::{ Action, Modifier }; use ::keyboard; -use ::layout::{ Button, Label, LatchedState, Layout }; +use crate::layout::{ Button, ButtonPosition, Label, LatchedState, Layout }; use ::layout::c::{ Bounds, EekGtkKeyboard, Point }; use ::submission::c::Submission as CSubmission; @@ -84,8 +83,15 @@ mod c { let cr = unsafe { cairo::Context::from_raw_none(cr) }; let active_modifiers = submission.get_active_modifiers(); - layout.foreach_visible_button(|offset, button| { - let state = RefCell::borrow(&button.state).clone(); + layout.foreach_visible_button(|offset, button, (row, position_in_row)| { + // TODO: this iterator copies string indices way too much. + // For efficiency, it would be better to draw pressed buttons from the list first, + // and then iterate the rest without having to look up their indices. + let state = layout.state.active_buttons.get(&ButtonPosition { + view: layout.state.current_view.clone(), + row, + position_in_row, + }); let locked = LockedStyle::from_action( &button.action, @@ -116,7 +122,7 @@ mod c { let layout = unsafe { &mut *layout }; let cr = unsafe { cairo::Context::from_raw_none(cr) }; - layout.foreach_visible_button(|offset, button| { + layout.foreach_visible_button(|offset, button, _index| { render_button_at_position( renderer, &cr, offset, diff --git a/src/keyboard.rs b/src/keyboard.rs index 6380d353..8c3f5d35 100644 --- a/src/keyboard.rs +++ b/src/keyboard.rs @@ -2,14 +2,13 @@ * Regards the keyboard as if it was composed of switches. */ use crate::action::Action; +use crate::layout; use crate::util; -use std::cell::RefCell; use std::collections::HashMap; use std::fmt; use std::io; use std::mem; use std::ptr; -use std::rc::Rc; use std::string::FromUtf8Error; // Traits @@ -48,9 +47,16 @@ bitflags!{ } /// When the submitted actions of keys need to be tracked, -/// they need a stable, comparable ID +/// they need a stable, comparable ID. +/// With layout::ButtonPosition, the IDs are unique within layouts. #[derive(Clone, PartialEq)] -pub struct KeyStateId(*const KeyState); +pub struct KeyStateId(layout::ButtonPosition); + +impl From<&layout::ButtonPosition> for KeyStateId { + fn from(v: &layout::ButtonPosition) -> Self { + Self(v.clone()) + } +} #[derive(Clone)] pub struct Key { @@ -81,12 +87,6 @@ impl KeyState { ..self } } - - /// KeyStates instances are the unique identifiers of pressed keys, - /// and the actions submitted with them. - pub fn get_id(keystate: &Rc>) -> KeyStateId { - KeyStateId(keystate.as_ptr() as *const KeyState) - } } /// Sorts an iterator by converting it to a Vector and back diff --git a/src/layout.rs b/src/layout.rs index 33a98794..61cebae3 100644 --- a/src/layout.rs +++ b/src/layout.rs @@ -17,19 +17,17 @@ * and let the renderer scale and center it within the widget. */ -use std::cell::RefCell; use std::cmp; use std::collections::HashMap; use std::ffi::CString; use std::fmt; -use std::rc::Rc; use std::vec::Vec; use crate::action::Action; use crate::actors; use crate::drawing; use crate::float_ord::FloatOrd; -use crate::keyboard::{KeyState, KeyCode}; +use crate::keyboard::{KeyState, KeyCode, PressType}; use crate::logging; use crate::popover; use crate::receiver; @@ -39,7 +37,6 @@ use crate::util::find_max_double; use crate::imservice::ContentPurpose; // Traits -use std::borrow::Borrow; use crate::logging::Warn; /// Gathers stuff defined in C or called by C @@ -238,20 +235,17 @@ pub mod c { // The list must be copied, // because it will be mutated in the loop - for button in layout.state.pressed_buttons.clone() { - if let Some(key) = layout.shape.get_key(&button).map(|k| k.clone()) { - seat::handle_release_key( - layout, - &mut submission, - Some(&ui_backend), - time, - Some((&popover_state, app_state.clone())), - &key, - button, - ); - } else { - log_print!(logging::Level::Bug, "Failed to find button at position {:?}", button); - } + let pressed_buttons + = layout.state.active_buttons.clone(); + for (button, _key_state) in pressed_buttons.iter_pressed() { + seat::handle_release_key( + layout, + &mut submission, + Some(&ui_backend), + time, + Some((&popover_state, app_state.clone())), + button, + ); } drawing::queue_redraw(ui_keyboard); } @@ -269,21 +263,16 @@ pub mod c { let mut submission = submission.borrow_mut(); // The list must be copied, // because it will be mutated in the loop - for button in layout.state.pressed_buttons.clone() { - if let Some(key) = layout.shape.get_key(&button) { - let key: &Rc> = key.borrow(); - seat::handle_release_key( - layout, - &mut submission, - None, // don't update UI - Timestamp(time), - None, // don't switch layouts - &mut key.clone(), - button, - ); - } else { - log_print!(logging::Level::Bug, "Failed to find button at position {:?}", button); - } + let pressed_buttons = layout.state.active_buttons.clone(); + for (button, _key_state) in pressed_buttons.iter_pressed() { + seat::handle_release_key( + layout, + &mut submission, + None, // don't update UI + Timestamp(time), + None, // don't switch layouts + button, + ); } } @@ -304,10 +293,9 @@ pub mod c { Point { x: x_widget, y: y_widget } ); - let state = layout.find_button_by_position(point) - .map(|(button, index)| (button.state.clone(), index)); + let index = layout.find_index_by_position(point); - if let Some((state, (row, position_in_row))) = state { + if let Some((row, position_in_row)) = index { let button = ButtonPosition { view: layout.state.current_view.clone(), row, @@ -317,8 +305,7 @@ pub mod c { layout, &mut submission, Timestamp(time), - &state, - button, + &button, ); // maybe TODO: draw on the display buffer here drawing::queue_redraw(ui_keyboard); @@ -359,34 +346,29 @@ pub mod c { Point { x: x_widget, y: y_widget } ); - let pressed = layout.state.pressed_buttons.clone(); - let button_info = { - let place = layout.find_button_by_position(point); - place.map(|(button, index)| {( - button.state.clone(), - index, - )}) - }; + let pressed_buttons = layout.state.active_buttons.clone(); + let pressed_buttons = pressed_buttons.iter_pressed(); + let button_info = layout.find_index_by_position(point); - if let Some((state, (row, position_in_row))) = button_info { + if let Some((row, position_in_row)) = button_info { + let current_pos = ButtonPosition { + view: layout.state.current_view.clone(), + row, + position_in_row, + }; let mut found = false; - for button in pressed { - if let Some(key) = layout.shape.get_key(&button).map(|k| k.clone()) { - if Rc::ptr_eq(&state, &key) { - found = true; - } else { - seat::handle_release_key( - layout, - &mut submission, - Some(&ui_backend), - time, - Some((&popover_state, app_state.clone())), - &key, - button, - ); - } + for (button, _key_state) in pressed_buttons { + if button == ¤t_pos { + found = true; } else { - log_print!(logging::Level::Bug, "Failed to find button at position {:?}", button); + seat::handle_release_key( + layout, + &mut submission, + Some(&ui_backend), + time, + Some((&popover_state, app_state.clone())), + button, + ); } } if !found { @@ -399,8 +381,7 @@ pub mod c { layout, &mut submission, time, - &state, - button, + &button, ); // maybe TODO: draw on the display buffer here unsafe { @@ -408,20 +389,15 @@ pub mod c { } } } else { - for button in pressed { - if let Some(key) = layout.shape.get_key(&button).map(|k| k.clone()) { - seat::handle_release_key( - layout, - &mut submission, - Some(&ui_backend), - time, - Some((&popover_state, app_state.clone())), - &key, - button, - ); - } else { - log_print!(logging::Level::Bug, "Failed to find button at position {:?}", button); - } + for (button, _key_state) in pressed_buttons { + seat::handle_release_key( + layout, + &mut submission, + Some(&ui_backend), + time, + Some((&popover_state, app_state.clone())), + button, + ); } } drawing::queue_redraw(ui_keyboard); @@ -467,7 +443,7 @@ pub enum Label { } /// The definition of an interactive button -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq)] pub struct Button { /// ID string, e.g. for CSS pub name: CString, @@ -481,8 +457,6 @@ pub struct Button { pub keycodes: Vec, /// Static description of what the key does when pressed or released pub action: Action, - /// Current state, shared with other buttons - pub state: Rc>, } impl Button { @@ -694,16 +668,49 @@ pub struct Layout { } /// Button position for the pressed buttons list -#[derive(Clone, Debug, PartialEq)] +#[derive(Clone, Debug, PartialEq, Eq, Hash)] pub struct ButtonPosition { // There's only ever going to be a handul of pressed buttons, // so as inefficient as String is, it won't make a difference. // In the worst case, views can turn into indices in the description. - view: String, + pub view: String, /// Index to the view's row. - row: usize, + pub row: usize, /// Index to the row's button. - position_in_row: usize, + pub position_in_row: usize, +} + +#[derive(Clone)] +pub struct ActiveButtons(HashMap); + +enum Presence { + Missing, + Present, +} + +static RELEASED: KeyState = KeyState { pressed: PressType::Released }; + +impl ActiveButtons { + fn insert(&mut self, button: ButtonPosition, state: KeyState) -> Presence { + match self.0.insert(button, state) { + Some(_) => Presence::Present, + None => Presence::Missing, + } + } + + pub fn get(&self, button: &ButtonPosition) -> &KeyState { + self.0.get(button) + .unwrap_or(&RELEASED) + } + fn remove(&mut self, button: &ButtonPosition) -> Presence { + match self.0.remove(button) { + Some(_) => Presence::Present, + None => Presence::Missing, + } + } + fn iter_pressed(&self) -> impl Iterator { + self.0.iter().filter(|(_p, s)| s.pressed == PressType::Pressed) + } } /// Changeable state that can't be derived from the definition of the layout. @@ -721,7 +728,11 @@ pub struct LayoutState { // through all buttons of the current view anyway. // When the list tracks actual location, // it becomes possible to place popovers and other UI accurately. - pub pressed_buttons: Vec, + /// Buttons not in this list are in their base state: + /// not pressed. + /// Latched/locked appearance is derived from current view + /// and button metadata. + pub active_buttons: ActiveButtons, } /// A builder structure for picking up layout data from storage @@ -762,10 +773,6 @@ impl fmt::Display for NoSuchView { impl LayoutData { - fn get_key(&self, button: &ButtonPosition) -> Option<&Rc>> { - self.get_button(button).map(|button| &button.state) - } - fn get_button(&self, button: &ButtonPosition) -> Option<&Button> { let (_, view) = self.views.get(&button.view)?; let (_, row) = view.rows.get(button.row)?; @@ -835,7 +842,7 @@ impl Layout { state: LayoutState { current_view: "base".to_owned(), view_latched: LatchedState::Not, - pressed_buttons: Vec::new(), + active_buttons: ActiveButtons(HashMap::new()), }, } } @@ -863,23 +870,27 @@ impl Layout { pub fn get_view_latched(&self) -> &LatchedState { &self.state.view_latched } - - /// Returns index too - fn find_button_by_position(&self, point: c::Point) -> Option<(&Button, (usize, usize))> { - let (offset, layout) = self.get_current_view_position(); - layout.find_button_by_position(point - offset) + + /// Returns index within current view + fn find_index_by_position(&self, point: c::Point) -> Option<(usize, usize)> { + let (offset, view) = self.get_current_view_position(); + view.find_button_by_position(point - offset) + .map(|(_b, i)| i) } + /// Returns index within current view too. pub fn foreach_visible_button(&self, mut f: F) - where F: FnMut(c::Point, &Box