`chathistory` Support #619

andymandias · 2024-10-18T05:04:32Z

A refactoring of the previous chathistory PR. It has all of the same features as before (plus a new configuration option), but the plumbing has been restructured to better match the paradigm set by the draft/read-marker PR.

Features (when chathistory is supported by the server):

Uses chathistory to request new history for all joined channels from the server (limited to one request if no history for the channel that exists, 500 messages or the maximum number of messages allowed by the server).
Adds button at the start of channel/query buffers to request older history via chathistory.
Uses chathistory to determine whether queries were received since last connection to the server, then request the new history for those queries from the server.
Configuration option to automatically request older history when scrolling to the top of a channel/query buffer (infinite_scroll).
Configuration option to disable chathistory for a server.

I haven't had much time to test it since refactoring, so I may make fixes in this PR as testing continues. But so far it is working as intended for me, has the entirety of my intended functionality for initial support, and is ready for review.

tarkah · 2024-10-18T12:30:02Z

data/src/history.rs

+/// Insert the incoming message into the provided vector, sorted
+/// on server time
+///
+/// Deduplication is only checked +/- 1 second around the server time
+/// of the incoming message. Either message IDs match, or server times
+/// have an exact match + target & content.
+pub fn insert_message(messages: &mut Vec<Message>, message: Message) {


Didn't we have a commit which used a binary search implementation for inserting messages?

Yes, this is the insert_message from that work we did together, completely unchanged. I was lazy about persisting the commit history while refactoring, so it's all been squashed into one commit.

The one thing this doesn't keep related to that work is the sorting of saved histories on load. In that version of the chathistory PR, I had changed the history storage scheme to no longer be obfuscated (no longer use hashes in naming the files). If histories were not found with the non-hashed storage scheme (where histories can be expected to be sorted already), then it would attempt to load & sort a history from the old, hashed storage scheme. I still like that idea, but it seemed like this PR might have too many changes already, so I omitted it this time around.

Ahh my bad, I just totally forgot what the code looked like and didn't realize that was the implementation we landed on :P

tarkah · 2024-10-21T21:37:10Z

data/src/history.rs

+    let start = message::fuzz_start_server_time(message.server_time);
+    let end = message::fuzz_end_server_time(message.server_time);
+


I don't think these helpers are helpful or necessary. We can add / sub chrono::Duration from DateTime<Utc> and construct it const. So we can add const FUZZ_SECONDS: chrono::Duration = chrono::Duration::seconds(1) to the first line of this function and then let start = message.server_time - FUZZ_SECONDS;.

Aha! Yes, I think I added the functions primarily because I was only aware of the TimeDelta version of changing the time which is considerably more of a mouthful. Secondarily because FUZZ_SECONDS was being shared with the chathistory request. I don't remember why I did that, and while they're not completely unrelated I don't see any reason to keep them in lock-step. I'll add FUZZ_SECONDS here as you suggest, and set a distinct FUZZ_SECONDS inside isupport.rs.

tarkah

This is looking really good! A lot easier to grok without all the other work that you've now separately landed.

I think there's some opportunity to remove a lot of the chat history logic from the frontend and push it behind the history API on the data layer which I think would be a little nicer. Let me know what you think! It very well be this isn't the way to go but it's just a gut feeling from briefly seeing how much "data logic" exists on the frontend vs just plumbing code / UI logic

tarkah · 2024-11-07T22:42:49Z

data/src/history/metadata.rs

+        for message_reference_type in message_reference_types {
+            match message_reference_type {
+                isupport::MessageReferenceType::MessageId => {
+                    if let Some(id) = &self.id {
+                        return isupport::MessageReference::MessageId(id.clone());
+                    }
+                }
+                isupport::MessageReferenceType::Timestamp => {
+                    return isupport::MessageReference::Timestamp(self.timestamp);
+                }
+            }
+        }
+


Do we prioritize ID over timestamp? If so, should we naively be iterating over the incoming isupport array or should we sort it so we always try id first?

Alternatively isupport::MessageReferenceType has an ALL constant we can loop over in the right order, and we check if each one of those exists in the incoming array and if so we do the normal match.

Otherwise if order doesn't matter disregard

The chathistory spec says that they are to be provided by the server ordered in (the server's) decreasing preference. I believe we preserve that order. I think the most polite thing to do is respect the server's preference, but I don't believe we are required to do so. My personal opinion is that ID is preferable over timestamp, when available.

tarkah · 2024-11-07T22:43:36Z

data/src/history/metadata.rs

+
+impl PartialEq for MessageReferences {
+    fn eq(&self, other: &Self) -> bool {
+        self.timestamp.eq(&other.timestamp)


Where do we do these equality checks and why isn't id considered? I'm asking this naively since I haven't come across it in my review yet.

Currently this is only used for some syntactic sugar, in update_partial in data/src/history.rs this (via Ord) allows .max to be used. id isn't considered because for the purpose it's currently being used we only want to know if we have a more recent set of references.

I originally implemented it this way because Ord was utilized elsewhere in the refactor, but that turned out to be unnecessary and was stripped back to just the one use rather than unraveled completely. No particular attachment to it.

tarkah · 2024-11-07T22:44:35Z

data/src/isupport.rs

-use irc::proto;
+use chrono::{format::SecondsFormat, DateTime, Utc};


Nit: Import ordering, chrono should be grouped after std.. Std -> External -> Crate

tarkah · 2024-11-07T22:46:48Z

data/src/message.rs

 use crate::config::buffer::UsernameFormat;
+use crate::history::MessageReferences;


Seems like message::Reference should live in this module, not in history, as this now creates a circular dependency between the two modules

Originally put it in history::metadata because that's where it lives, inside the Metadata struct. But I can see the rationale for moving it to message. Will do so.

tarkah · 2024-11-07T22:52:41Z

src/buffer/scroll_view.rs

+                    && matches!(self.limit, Limit::Top(_))
+                    && relative_offset == 0.0


I think this can be replaced with self.status.is_top(relative_offset)

Aha, thanks! I can't remember why, but I thought it was necessary to check limit as well. I'll switch this.

tarkah · 2024-11-07T22:57:36Z

src/main.rs

+                                                    target,
+                                                    message_reference,
+                                                );
+                                            }
+                                            ChatHistorySubcommand::Before(target, message_reference, _) => {
+                                                log::debug!(
+                                                    "[{}] received {} messages in {} before {}",
+                                                    server,
+                                                    batch_len,
+                                                    target,
+                                                    message_reference,
+                                                );
+                                            }
+                                            ChatHistorySubcommand::Between(
+                                                target,
+                                                start_message_reference,
+                                                end_message_reference,
+                                                _,
+                                            ) => {
+                                                log::debug!(
+                                                    "[{}] received {} messages in {} between {} and {}",
+                                                    server,
+                                                    batch_len,
+                                                    target,
+                                                    start_message_reference,
+                                                    end_message_reference,
+                                                );
+                                            }
+                                            ChatHistorySubcommand::Targets(
+                                                start_message_reference,
+                                                end_message_reference,
+                                                _,
+                                            ) => {
+                                                log::debug!(
+                                                    "[{}] received {} targets between {} and {}",
+                                                    server,
+                                                    batch_len,
+                                                    start_message_reference,
+                                                    end_message_reference,
+                                                );
+                                            }
+                                        }
+
+                                        self.clients.clear_chathistory_request(&server, subcommand.target());
+                                    }
+                                    data::client::Event::ChatHistoryTarget(target, server_time) => {
+                                        let command = dashboard
+                                            .load_metadata(server.clone(), target.clone())
+                                            .map_or(Task::none(), |command| command.map(Message::Dashboard));
+
+                                        let command = command.chain(Task::done(Message::Dashboard(
+                                            dashboard::Message::RequestNewerChatHistory(
+                                                server.clone(),
+                                                target,
+                                                server_time,
+                                            ),
+                                        )));
+
+                                        commands.push(command);
+                                    }
+                                    data::client::Event::ChatHistoryTargetsReceived(
+                                        subcommand,
+                                        batch_len,
+                                        server_time
+                                    ) => {
+                                        if let ChatHistorySubcommand::Targets(
+                                                start_message_reference,
+                                                end_message_reference,
+                                                _,
+                                        ) = subcommand {
+                                            log::debug!(
+                                                "[{}] received {} targets between {} and {}",
+                                                server.clone(),
+                                                batch_len,
+                                                start_message_reference,
+                                                end_message_reference,
+                                            );
+                                        }
+
+                                        let server = server.clone();
+
                                        commands.push(
-                                            dashboard
-                                                .channel_joined(server.clone(), channel)
-                                                .map(Message::Dashboard),
+                                            Task::perform(
+                                                async move {
+                                                    (
+                                                        server.clone(),
+                                                        data::client::overwrite_chathistory_targets_timestamp(
+                                                            server,
+                                                            server_time,
+                                                        )
+                                                        .await,
+                                                    )
+                                                },
+                                                move |(server, result)| {
+                                                    Message::UpdateChatHistoryTargetsTimestamp(server, server_time, result)
+                                                }
+                                            ),


Can this history::Manager API encapsulate a lot of this logic? Similar to what we did for returning Future from it's update and it has it's own internal Message type. Then we can simply hook up client Chat History events to methods on history::Manager which can be called -> Future<Output = Message> -> history_manager.update when those tasks resolve.

I don't think we should have complex backend specific logic in main

tarkah · 2024-11-07T23:01:31Z

src/buffer/scroll_view.rs

+    Channel(&'a Server, &'a str, Option<ChatHistoryState>),
+    Query(&'a Server, &'a Nick, Option<ChatHistoryState>),


Stuffing this into here seems to be causing us to require fetching this state when it's not even used. Checkout how we're querying for it when we construct Kind when calling scroll_to_backlog, this state isn't used anywhere there.

It seems we only need it during view so instead pass Option<ChatHistoryState> as a separate param there and it's just always None from server & query. This will clean up some code paths

… insertion and chathistory purposes.

…ndense `chathistory` logic into fewer locations.

andymandias · 2024-11-14T03:06:34Z

I think this is in a better place (that ~addresses the current review comments) and is ready for another look, when you have time @tarkah.

andymandias linked an issue Oct 18, 2024 that may be closed by this pull request

Feature request: IRCv3 CHATHISTORY support #206

Open

tarkah reviewed Oct 18, 2024

View reviewed changes

tarkah reviewed Oct 21, 2024

View reviewed changes

andymandias force-pushed the feat/chathistory branch from 31aeb85 to 3de0295 Compare October 30, 2024 17:08

tarkah reviewed Nov 7, 2024

View reviewed changes

andymandias force-pushed the feat/chathistory branch from 3de0295 to 6c22ed2 Compare November 9, 2024 03:42

andymandias added 8 commits November 11, 2024 14:26

Initial refactoring of previous, unmerged chathistory implementation.

a57eff3

Simplify FUZZ_SECONDS and separate into distinct values for history…

e623ccd

… insertion and chathistory purposes.

Only consider text of message content for equality comparison.

2535e69

Rebase fixes.

cf4c67f

Addressing review comments.

54f6bbe

Rebase fixes.

3da7694

Fix for duplicate messages in history under certain circumstances.

44d70ec

Restructure to move back-end related logic out of main.rs and to co…

a3bfce5

…ndense `chathistory` logic into fewer locations.

andymandias force-pushed the feat/chathistory branch from 6c22ed2 to a3bfce5 Compare November 11, 2024 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`chathistory` Support #619

`chathistory` Support #619

andymandias commented Oct 18, 2024

tarkah Oct 18, 2024

andymandias Oct 18, 2024

tarkah Oct 21, 2024

tarkah Oct 21, 2024 •

edited

Loading

andymandias Oct 22, 2024

tarkah left a comment •

edited

Loading

tarkah Nov 7, 2024

tarkah Nov 7, 2024

andymandias Nov 8, 2024 •

edited

Loading

tarkah Nov 7, 2024

andymandias Nov 8, 2024

tarkah Nov 7, 2024

tarkah Nov 7, 2024

andymandias Nov 8, 2024

tarkah Nov 7, 2024

andymandias Nov 8, 2024

tarkah Nov 7, 2024

tarkah Nov 7, 2024

andymandias commented Nov 14, 2024

		let start = message::fuzz_start_server_time(message.server_time);
		let end = message::fuzz_end_server_time(message.server_time);

		use irc::proto;
		use chrono::{format::SecondsFormat, DateTime, Utc};

		use crate::config::buffer::UsernameFormat;
		use crate::history::MessageReferences;

		&& matches!(self.limit, Limit::Top(_))
		&& relative_offset == 0.0

		Channel(&'a Server, &'a str, Option<ChatHistoryState>),
		Query(&'a Server, &'a Nick, Option<ChatHistoryState>),

chathistory Support #619

Are you sure you want to change the base?

chathistory Support #619

Conversation

andymandias commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarkah Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarkah left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andymandias Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andymandias commented Nov 14, 2024

`chathistory` Support #619

`chathistory` Support #619

tarkah Oct 21, 2024 •

edited

Loading

tarkah left a comment •

edited

Loading

andymandias Nov 8, 2024 •

edited

Loading