Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Smart Message Filter #221

Merged
merged 11 commits into from
Feb 28, 2024
Merged

Conversation

andymandias
Copy link
Collaborator

This is an attempt to address issue #94, which works locally but may need improving. In particular, I am concerned it will be brittle, since it relies on the formatting of the join/part/quit server messages. But, I didn't see any other way to get the username for the server message without modifying the data structure of the server message. If it would be preferable to modify the data structure of the server message, or if there's a better way to get the user nickname of a join/part/quit server message that I've missed, then I'd be happy to rework this implementation.

Serde was updated because I could not get it to recognize the seconds config for the smart filter otherwise.

@andymandias andymandias marked this pull request as draft January 22, 2024 19:01
@casperstorm casperstorm requested a review from tarkah January 23, 2024 19:57
@andymandias andymandias marked this pull request as ready for review February 3, 2024 23:31
@andymandias andymandias closed this Feb 6, 2024
@andymandias andymandias reopened this Feb 6, 2024
@andymandias
Copy link
Collaborator Author

andymandias commented Feb 6, 2024

I started work on a version that modifies the the Server struct in data/src/message/source.rs, but the changes were significant enough that I figure I should wait for an official ruling before going any further. Have been using this PR's version of the smart message filter for the last couple weeks and it has been working well locally, so I think it could potentially be used in the meantime.
(Edit to fix reference file path.)

data/src/history/manager.rs Outdated Show resolved Hide resolved
data/src/history/manager.rs Outdated Show resolved Hide resolved
data/src/history/manager.rs Outdated Show resolved Hide resolved
@andymandias
Copy link
Collaborator Author

Put a warning in the commit message but it's probably prudent to do so here as well; the latest commit stores the nickname with join/part/quit messages to check against, but that change results in chat history being clobbered when there's an error when deserializing it. I.e. it will wipe your chat history if you run it. Once that's fixed I think it might be good to go.

@tarkah
Copy link
Member

tarkah commented Feb 8, 2024

Awesome! We can prevent deser failure with the right serde magic. I'll take a look tomorrow 👍

@andymandias
Copy link
Collaborator Author

The last couple commits have (de)serialization working for me; no longer destroys old logs made using the old Server enum. Added a fallback to the server message filtering so it should work for server messages deserialized from the old Server enum as well (uses the old method that attempts to get the nickname from the server message itself, by necessity).

If there's better serde magic to be employed then I'm happy to wait for that; was just learning a bit more about serde and saw a potential fix.

@andymandias andymandias force-pushed the smart-message-filter branch 2 times, most recently from 5d2629f to cd4c50f Compare February 25, 2024 00:04
@andymandias andymandias mentioned this pull request Feb 25, 2024
Comment on lines 555 to 564
if most_recent_message_server_time.is_some() {
let duration_seconds = message
.server_time
.signed_duration_since(*most_recent_message_server_time.unwrap())
.num_seconds();

duration_seconds > *seconds
} else {
true
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you could use let else here:
(psuedo code)

    let Some(server_time) = most_recent_message_server_time else {
        return true
    };

    let duration_seconds = message.server_time.signed_duration_since(*server_time).num_seconds();
    duration_seconds > *seconds

Copy link
Collaborator Author

@andymandias andymandias Feb 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks clearer to me, thanks for the suggestion. Incorporated :)

@tarkah
Copy link
Member

tarkah commented Feb 27, 2024

@andymandias Checkout the changes I just pushed. It retains backwards compatibility, but uses some more well defined types vs serde magic. By using untagged, we can decode as the old value or the new value. All code paths now set the enum to the new value.

Apologies for the delay getting around to this

Copy link
Member

@tarkah tarkah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! LGTM

@casperstorm casperstorm merged commit 8b48fdb into squidowl:main Feb 28, 2024
1 check passed
@andymandias andymandias deleted the smart-message-filter branch February 28, 2024 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants