feat/1-view-room-list #32
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/1-view-room-list"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Add logic to display joined and available rooms. Room addresses and mock signals conform to API specification (with at least the minimum allowed specificity). Add display with rudimentary formatting of room objects. Simple unit tests for verification of conformity to expected JSON formats. Use signals, resources and memos to encourage decoupling, re-rendering (only as necessary) and future feature scaling.
Refs #1
feel free to msg me directly about any of the points so we dont have to have a discussion here, because I'm new to Rust my review points may not be that applicable
@ -6,1 +6,4 @@[dependencies]serde = { version = "1.0", features = ["derive", "rc"] }serde_json = "1.0"either = "1.15.0"was this ever used?
Both serde, serde_json are used but either ended up being removed. I thought I removed it merging main into my branch, it is not needed.
@ -0,0 +1,379 @@use dioxus::prelude::*;use serde;It looks like all the uses of serde reference it themselves, so can this line be removed?
@ -0,0 +56,4 @@fn new(s: String) -> Result<Self, &'static str> {// Empty check.if s.is_empty() {return Err::<T, &str>("Missing room id");I noticed a lot of type annotations that can be inferred, can you remove these from the file? It causes a lot of clutter
I forget why but the compiler previously complained that they were needed. I removed all the turbofishes from the errors, let me know if you think there are other things that could be cleaned up.
Ah it was because I wasn't explicitly returning the errors before (which was also a mistake).
@ -0,0 +61,4 @@// Prefix check for SPECIFIED type.let (index, id_or_alias) = T::validate_prefix(&s);if index == None {if (|| id_or_alias == MxRoomEnum::MxRoomId as usize)() {you can just put this inline to avoid the need of ||, it will keep it a bit cleaner
@ -0,0 +63,4 @@if index == None {if (|| id_or_alias == MxRoomEnum::MxRoomId as usize)() {return Err::<T, &str>("Invalid room id, '!' missing.");} else if (|| id_or_alias == MxRoomEnum::MxRoomAlias as usize)() {same thing here, you can omit the || and put the return in line
@ -0,0 +68,4 @@}}// Check that SLD has been listed.let n_index = index.unwrap_or_else(|| 0);Wondering here as well (referring to my comment on line 77)
Technically yes, but you are right that it should be no. If the caller returns a usize that doesn't belong to the enum it would allow execution to continue, which is a flaw.
@ -0,0 +74,4 @@return Err::<T, &str>("Invalid room, ':' missing.");}// Check that TLD has been listed.let n_index = index.unwrap_or_else(|| 0);Just wondering, what does unwrap_or_else do here? Are there conditions further than the previous index == None that can still happen?
@ -0,0 +81,4 @@}// Check legal chars for SPECIFIED type.let mut t = String::with_capacity(s.len());if (|| id_or_alias == MxRoomEnum::MxRoomId as usize)() {same thing here, you can omit the || and put the replace inline
@ -0,0 +83,4 @@let mut t = String::with_capacity(s.len());if (|| id_or_alias == MxRoomEnum::MxRoomId as usize)() {t.push_str(&s.replace(&['!', ':', '.'][..], ""));} else if (|| id_or_alias == MxRoomEnum::MxRoomAlias as usize)() {same thing here, you can omit the || and put the replace inline
@ -0,0 +87,4 @@t.push_str(&s.replace(&['#', ':', '.'][..], ""));}if !(t.chars().all(char::is_alphanumeric)) {Err::<T, &str>("Invalid room, illegal char(s).")I was actually able to make a room with the following ID
#test-!a_@#$(&%($#&^%$bunchofchars:elsie.cafe
It seems like the only reserved character is the colon from what I could find (once you are already in a private/public room id from the ! or # that is)
Good catch. This was a bit trickier to sort out, but I have updated the logic to be much more lenient with room addresses. Room aliases in element disallow both '#' and '%' from the localpart which I think make sense to exclude, I've just followed that for now.
@ -0,0 +96,4 @@impl std::fmt::Display for MxRoomId {fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {write!(f, "{:?}", self.0)I noticed the {:?} format being used a few times, and saw that it is for debug
https://doc.rust-lang.org/std/fmt/index.html#fmtdisplay-vs-fmtdebug
Is this intentional?
It can be removed.
@ -0,0 +121,4 @@impl MxRoomAddressFactory for MxRoomId {fn validate_prefix(s: &str) -> (Option<usize>, usize) {let index: Option<usize> = s.find('!');if index == Some(0) && s[index.unwrap_or_else(|| 0) + 1..].find('!') == None {does the index == Some(0) followed by an unwrap_or_else make sense here?
Maybe I am misinterpreting unwrap_or_else, but I would assume it would always resolve to 0 here
I'd already removed this while updating the checks for valid room addresses. But yes, it isn't needed - I forgot that && will just short circuit if index isn't Some(0). If the conditions were flipped for some reason it might make sense as index could be None in that case.
@ -0,0 +191,4 @@impl std::fmt::Display for AvailableRoom {fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {write!(f, "{:?} @ {:?}", self.name, self.room_id.0)this one has also got the debug string formatting, is it intentional?
@ -0,0 +201,4 @@}// This is only needed for testing, we only expect to receive valid JSON objects making queries to the matrix API.impl ParseError for AvailableRoomsList {For these usages can default be used instead? it's used more as a default fallback than providing an error from what I can see.
Default could be used but I opted doing it this way since I think it's more idiomatic to have default return an empty or "base" object. ParseError is only a placeholder anyway, since once it's hooked up to the API we only expect to receive valid JSON - in which case the assertion-like unwrap() can be used instead. MxRoom_::new() already handles validity checks, so rooms can just be appended directly in the resource.
Actually it just calls default for AvailableRoomsList anyway. I was thinking of what I did for JoinedRoomsList (I wrote this code when I was quite tired). To be honest the whole thing could just be removed. There is no point since there is already a check upon serialisation of synthetic data.
Wow I completely forgot I did this but when attempting to deserialize using serde_json, all the necessary checks already take place (I overrode try_from so that serde calls the constructor method of MxRoom_) so yes this is not needed at all.
Also updating MxRoom_ defaults because I was treating those as fallback for some reason.
@ -0,0 +227,4 @@// use_server_future not preferred as manual refresh expected. Make mutable on room addition.let joined_rooms_monitor = use_resource(move || async move {serde_json::from_str::<JoinedRoomsList>(&joined_rooms_from_api.read()).unwrap_or_else(|_| JoinedRoomsList::default_parse_error()) // Can do assertion-like unwrap() if known conforming JSON.can you use .unwrap_or_default here?
@ -0,0 +301,4 @@}}macro_rules! test_block {Should this bit be out of the test block? I think it means it will get built with the application which we wouldn't want right?
Its giving me an unused macro in my ide as well when not inside of cfg(test)
Actually I don't think I need this macro at all.
5758b4240bto7c7128adc8New commits pushed, approval review dismissed automatically according to repository settings