Conversation
|
Thanks for the contribution! I did a high level skim and looks good so far. I'll look into it with more detail at a later time. |
src/cfg.rs
Outdated
| let layer_idxs = parse_layer_indexes(&layer_exprs, mapping_order.len())?; | ||
| let mut sorted_idxs: Vec<(String, usize)> = layer_idxs | ||
| .iter() | ||
| .map(|tuple| (tuple.0.clone(), tuple.1.clone())) |
There was a problem hiding this comment.
I believe this could be succinctly replaced with .cloned()
There was a problem hiding this comment.
I tried getting this to compile with cloned() but didn't have much luck. I tried to simplify this a little be creating a tuple of references here and then just cloning the name as required further down in the fn
src/cfg.rs
Outdated
| .iter() | ||
| .map(|tuple| (tuple.0.clone(), tuple.1.clone())) | ||
| .collect(); | ||
| sorted_idxs.sort_by(|&(_, a), &(_, b)| a.cmp(&b)); |
There was a problem hiding this comment.
I believe sort_by_key expresses the intent better here.
There was a problem hiding this comment.
Implemented this change
src/cfg.rs
Outdated
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let mut layer_info = vec![]; |
There was a problem hiding this comment.
I would prefer a direct assignment to layer_info by using map and collect rather than making it mut and pushing.
// note: not checked for correctness
let layer_info: Vec<LayerInfo> = layer_names
.iter()
.zip(layer_strings)
.map(|(name, cfg_text)| LayerInfo {name, cfg_text})
.collect();
There was a problem hiding this comment.
Implemented this change
src/kanata.rs
Outdated
|
|
||
| impl NotificationServer { | ||
| pub fn new(port: i32) -> Self { | ||
| let server = Self { |
There was a problem hiding this comment.
Can return Self directly here:
Self {
port,
connections: Arc::new(Mutex::new(HashMap::new())),
}
There was a problem hiding this comment.
Implemented this change
src/kanata.rs
Outdated
| panic!("channel disconnected") | ||
| } | ||
| Ok(event) => { | ||
| let k = kanata.lock(); |
There was a problem hiding this comment.
I don't like that we're acquiring a lock on the Kanata struct here; it means the processing loop is still dependent on and could be blocked by TCP operations.
It seems to me that the NotificationServer doesn't need to be part of the Kanata struct, so it would be better to move it out to avoid having lock contention between TCP operations and the keyboard operations.
There was a problem hiding this comment.
Removed NotificationServer from the Kanata struct
src/kanata.rs
Outdated
| } | ||
|
|
||
| pub fn start_notification_loop(kanata: Arc<Mutex<Self>>, rx: Receiver<EventNotification>) { | ||
| info!("Kanata: entering the event notification loop"); |
There was a problem hiding this comment.
Changed this to Kanata: listening for event notifications to relay to connected clients but I'm open to whatever you wanna put here
src/kanata.rs
Outdated
| log::info!("Entered layer:\n{}", self.layer_info[layer].cfg_text); | ||
| } | ||
|
|
||
| pub fn start_notification_loop(kanata: Arc<Mutex<Self>>, rx: Receiver<EventNotification>) { |
There was a problem hiding this comment.
May need to think about TCP timeout for the clients, e.g. have a heartbeat event sent every 30s (the processing loop can keep track of the timer for this one).
Also handling (and ignoring) the RX on the TCP socket so that the kernel buffers don't fill up.
LGUG2Z
force-pushed
the
feature/listeners
branch
from
ed572a6 to
57a6928
Compare
2 participants
No description provided.