From c2d4662f493921081146b0163f7837d632f92742 Mon Sep 17 00:00:00 2001 From: Justin Scroggins Date: Sun, 18 Oct 2026 16:27:29 -0400 Subject: [PATCH] fix: record my own Discord profile at READY so a sending message keeps my avatar A message being sent names no picture, so its row shows the account's. On Discord that was missing: the account's picture is read from my own row in People, and READY and USER_UPDATE only set the account's name. READY now registers my picture and records my row before naming the account, and a USER_UPDATE of mine does the same, so the picture is there from connect and follows a change. The rail and chatd's own-picture fetch get it too. For a message of mine the window now shows its own picture, then the account's, and only then the one my newest message here was stored with, for a network that keeps no row of me. A stored picture is never re-read, so it no longer beats a newer account picture. The fake Discord counts fetches of my picture apart from what is asked of the API, since chatd fetches it on its own clock. Fixes #46 Co-Authored-By: Claude Opus 5.5 --- crates/chatd/src/adapters/discord/events.rs | 5 +- crates/chatd/src/adapters/discord/ready.rs | 11 +- crates/chatd/tests/discord.rs | 42 +++++++- .../tinyclient/src/gui/conversation/draw.rs | 100 ++++++++++++++++-- 4 files changed, 146 insertions(+), 12 deletions(-) diff --git a/crates/chatd/src/adapters/discord/events.rs b/crates/chatd/src/adapters/discord/events.rs index 53e45c3..58438fd 100644 --- a/crates/chatd/src/adapters/discord/events.rs +++ b/crates/chatd/src/adapters/discord/events.rs @@ -169,7 +169,10 @@ impl Discord { "CHANNEL_UNREAD_UPDATE" | "PASSIVE_UPDATE_V2" => self.passive(from_str(data)?)?, "USER_UPDATE" => { let user: User = from_str(data)?; - self.ctx.describe(user.name(), &user.id)?; + // Ours: a new name or picture. + if user.id == self.me() { + self.myself(&user)?; + } } "USER_SETTINGS_PROTO_UPDATE" => self.settings_proto(from_str(data)?)?, // Discord wants a phone number, an email check or a captcha: nothing more is asked of diff --git a/crates/chatd/src/adapters/discord/ready.rs b/crates/chatd/src/adapters/discord/ready.rs index d4de1e7..f701cdf 100644 --- a/crates/chatd/src/adapters/discord/ready.rs +++ b/crates/chatd/src/adapters/discord/ready.rs @@ -625,6 +625,15 @@ impl Discord { self.ctx.conversation(&row) } + /// Who we are, from READY or a change to it: the account's name, and our own row in People, + /// which is where the account's picture is read from. The row goes first, so the status + /// the name announces already has the picture. + pub(super) fn myself(&self, user: &User) -> rusqlite::Result<()> { + self.ctx.media(&[user.avatar()])?; + self.ctx.people(&[user.person(None)])?; + self.ctx.describe(user.name(), &user.id) + } + /// The user's server folders as JSON settings or protobuf ones carry them. Only a folder with /// an id is one; a server alone in a nameless one is simply not in a folder. fn folders(ready: &Ready) -> Option> { @@ -704,7 +713,7 @@ impl Discord { /// Takes READY apart into the store. Only [`super::State`] outlives this call. pub(super) fn ready(&self, ready: Ready) -> Result<(), Failure> { let me = ready.user.id.clone(); - self.ctx.describe(ready.user.name(), &me)?; + self.myself(&ready.user)?; self.capabilities(ready.user.premium_type); let order = Self::order(&ready); let folders = Self::folders(&ready); diff --git a/crates/chatd/tests/discord.rs b/crates/chatd/tests/discord.rs index 8ee83b3..684c40a 100644 --- a/crates/chatd/tests/discord.rs +++ b/crates/chatd/tests/discord.rs @@ -88,6 +88,9 @@ enum Control { struct Shared { requests: Mutex>, + /// Fetches of our own picture, kept out of `requests`: chatd gets the rail's picture on its + /// own clock, which no test can time. + own_pictures: AtomicUsize, /// Every frame the adapter sent on the gateway, in order. frames: Mutex>, control: Mutex>, @@ -393,7 +396,11 @@ fn serve_http(shared: Arc, stream: TcpStream) { bytes: body.len(), at: Instant::now(), }; - shared.requests.lock().unwrap().push(seen.clone()); + if seen.path.starts_with("/media/avatars/1000/") { + shared.own_pictures.fetch_add(1, Ordering::Relaxed); + } else { + shared.requests.lock().unwrap().push(seen.clone()); + } let bare = seen.path.split('?').next().unwrap().to_owned(); // What is not Discord's JSON: a file from the media proxy, the page that names the // build, and whatever a test wrote out by hand. @@ -578,6 +585,7 @@ impl World { let (control, controlled) = channel(); let shared = Arc::new(Shared { requests: Mutex::new(Vec::new()), + own_pictures: AtomicUsize::new(0), frames: Mutex::new(Vec::new()), control: Mutex::new(control), scripted: Mutex::new(HashMap::new()), @@ -968,6 +976,35 @@ fn ready_fills_spaces_trees_people_and_unread_state() { assert_eq!(&sent, properties); } +#[test] +fn my_picture_is_the_accounts_from_ready_and_follows_a_change_to_it() { + let world = World::start(); + let mine = || match world.ask(Request::Status) { + Response::Status { accounts, .. } => { + let ours = accounts.into_iter().find(|account| account.key == world.account()).unwrap(); + (ours.name, ours.avatar.map(|avatar| avatar.id)) + } + other => panic!("{other:?}"), + }; + // READY names our picture, so the account has it before any message of ours is seen: a + // message being sent, which names none, shows it. It is registered, and fetched for the + // rail with nothing asked by the shell. + assert_eq!(mine(), ("Justin".to_owned(), Some("avatar:1000:me0hash".to_owned()))); + wait("our picture to be fetched", || (world.discord.own_pictures.load(Ordering::Relaxed) > 0).then_some(())); + // Our own row is on none of the lists we own. + let owned = [world.people(None, None), world.people(Some(Relation::Friend), None), world.people(Some(Relation::Blocked), None)]; + assert!(owned.iter().flatten().all(|person| person.key.id != "1000"), "we are listed among our own people"); + + // A new picture is the account's at once. + world.discord.push("USER_UPDATE", json!({"id": "1000", "username": "justin", "global_name": "Justin", "discriminator": "0", "avatar": "me1hash"})); + wait("the new picture", || (mine().1.as_deref() == Some("avatar:1000:me1hash")).then_some(())); + // Someone else's is not ours. + world.discord.push("USER_UPDATE", json!({"id": "2001", "username": "mara", "global_name": "Mara Okafor", "discriminator": "0", "avatar": "a1b2c3"})); + world.incoming("3002", None, "after the update", json!({})); + wait("the update to be handled", || world.messages("3002").pop().filter(|message| message.text == "after the update")); + assert_eq!(mine(), ("Justin".to_owned(), Some("avatar:1000:me1hash".to_owned()))); +} + #[test] fn messages_map_to_the_one_model_and_follow_live_events() { let world = World::start(); @@ -2407,8 +2444,9 @@ fn a_ready_of_real_size_connects_quickly_within_the_memory_budget_and_is_not_kep assert!(released || kept_by_allocator, "settled at {settled} KB after a peak of {peak} KB"); // The first thirty seconds: five unread conversations fetched ahead, at a person's pace, - // and nothing else asked of Discord at all. + // our own picture once, and nothing else asked of Discord at all. sleep(Duration::from_secs(30).saturating_sub(started.elapsed())); + assert_eq!(world.discord.own_pictures.load(Ordering::Relaxed), 1); let requests = world.discord.requests.lock().unwrap().clone(); let paths: Vec<&str> = requests.iter().map(|seen| seen.path.as_str()).collect(); assert_eq!(requests.len(), 5, "{paths:?}"); diff --git a/crates/tinyclient/src/gui/conversation/draw.rs b/crates/tinyclient/src/gui/conversation/draw.rs index 5b95ec5..2c518e4 100644 --- a/crates/tinyclient/src/gui/conversation/draw.rs +++ b/crates/tinyclient/src/gui/conversation/draw.rs @@ -500,18 +500,18 @@ impl Pane { /// Someone in this conversation, from what its messages say of them, else from the people /// hub: their name and their picture. fn who(&self, person: &Key, cx: &mut Context) -> (String, Option) { - let root = self.open.as_ref().and_then(|open| open.root.as_ref()).map(|(root, _)| root); - let theirs = || root.into_iter().chain(&self.window.messages).rev().filter(|message| message.sender.id == person.id); - // A tombstone keeps the name and loses the picture, so one that has it is looked for first. - if let Some(message) = theirs().find(|message| message.sender.avatar.is_some()).or_else(|| theirs().next()) { + if let Some(message) = said_by(self.root(), &self.window.messages, &person.id) { return (message.sender.name.clone(), message.sender.avatar.clone()); } let known = self.shell.upgrade().and_then(|shell| shell.read(cx).people_state.hub().person(person)); known.map(|person| (person.name, person.avatar)).unwrap_or_default() } - /// My own picture on this account. A message I am sending is not from the network yet, - /// so it names no picture itself. + fn root(&self) -> Option<&Rc> { + self.open.as_ref().and_then(|open| open.root.as_ref()).map(|(root, _)| root) + } + + /// My own picture on this account. fn own_picture(&self, cx: &mut Context) -> Option { let key = self.key()?; self.shell.upgrade().and_then(|shell| shell.read(cx).own_picture(key)) @@ -527,8 +527,8 @@ impl Pane { None => Avatar::named(&sender.name, size), }; let file = match (&sender.avatar, message.outgoing) { - (Some(file), _) => Some(file.clone()), - (None, true) => self.own_picture(cx), + (_, true) => own_face(message, self.root(), &self.window.messages, &me, || self.own_picture(cx)), + (Some(file), false) => Some(file.clone()), // A tombstone names its sender and nothing else; their other messages have the picture. (None, false) => self.person(id).and_then(|key| self.who(&key, cx).1), }; @@ -1705,3 +1705,87 @@ fn natural(attachment: &Attachment) -> (u32, u32) { (still.width, still.height) } } + +/// The message that says most of who someone is here: their newest with a picture, else their +/// newest. A tombstone keeps the name and loses the picture, so one that has it is looked for first. +fn said_by<'a>(root: Option<&'a Rc>, messages: &'a [Rc], id: &str) -> Option<&'a Message> { + let theirs = || root.into_iter().chain(messages).rev().filter(|message| message.sender.id == id); + let found = theirs().find(|message| message.sender.avatar.is_some()).or_else(|| theirs().next()); + found.map(|message| &**message) +} + +/// The picture on a message of mine: the one it names, else the account's, which follows a +/// change of picture. One I am sending is not from the network yet and names none. Without the +/// account's (a network that keeps no row of me), my newest message here lends the picture it +/// was stored with, which may since have changed. +fn own_face( + message: &Message, + root: Option<&Rc>, + messages: &[Rc], + me: &str, + account: impl FnOnce() -> Option, +) -> Option { + let shown = || match me { + // Without my id nothing here is known to be mine. + "" => None, + me => said_by(root, messages, me)?.sender.avatar.clone(), + }; + message.sender.avatar.clone().or_else(account).or_else(shown) +} + +#[cfg(test)] +mod tests { + use super::*; + use protocol::MediaKind; + + fn picture(id: &str) -> MediaFile { + MediaFile::new(id, MediaKind::Avatar) + } + + /// One of mine the network has, with the picture it was stored with. + fn mine(id: &str, avatar: Option<&str>) -> Rc { + let mut message = Message::plain(id, "Jo", "said", 10, true); + message.sender.id = "me".into(); + message.sender.avatar = avatar.map(picture); + Rc::new(message) + } + + /// One of mine still going: chatd names no sender on it. + fn sending() -> Message { + let mut message = Message::plain("c1", "You", "going", 20, true); + message.state = SendState::Pending; + message + } + + fn face(message: &Message, messages: &[Rc], account: Option<&str>) -> Option { + own_face(message, None, messages, "me", || account.map(picture)).map(|file| file.id) + } + + #[test] + fn my_message_shows_its_own_picture_then_the_accounts_then_my_newest_here() { + let messages = [mine("1", Some("shown-here"))]; + assert_eq!(face(&mine("2", Some("its-own")), &messages, Some("account")).as_deref(), Some("its-own")); + // One being sent names none. + assert_eq!(face(&sending(), &messages, Some("account")).as_deref(), Some("account")); + assert_eq!(face(&sending(), &messages, None).as_deref(), Some("shown-here")); + assert_eq!(face(&sending(), &[], None), None); + } + + #[test] + fn a_picture_i_have_since_changed_does_not_come_back_from_my_earlier_messages() { + let messages = [mine("1", Some("before")), mine("2", Some("before"))]; + assert_eq!(face(&sending(), &messages, Some("after")).as_deref(), Some("after")); + } + + #[test] + fn only_my_newest_picture_here_is_lent_and_only_once_my_id_is_known() { + // A newer one of mine without a picture, a tombstone, does not lose it. + let messages = [mine("1", Some("older")), mine("2", Some("newer")), mine("3", None)]; + assert_eq!(face(&sending(), &messages, None).as_deref(), Some("newer")); + let mut theirs = Message::plain("4", "mara", "hi", 30, false); + theirs.sender.id = "mara".into(); + theirs.sender.avatar = Some(picture("mara")); + assert_eq!(face(&sending(), &[Rc::new(theirs)], None), None); + assert_eq!(own_face(&sending(), None, &messages, "", || None), None); + } +}