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 settings to increase split and usercard scrollback #3811

Merged

Conversation

goldbattle
Copy link
Contributor

@goldbattle goldbattle commented Jun 10, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Currently the problem is that for fast chats, the user popup log history doesn't provide much information since only 1000 messages are kept for each channel (. This of course makes the memory of the program very small, but basically only allows the logs to have the most recent message from a user (instead of messages in the past 5 minutes).

This adds two params to configure how many messages are displayed on-screen ChannelView and how many are kept in memory for use in the chat log / searching Channel.

image

Use of the shared pointer is not ideal, if there is a better way let me know. I didn't dig deep but it errors on me just re-constructing the variable in the constructor (is there a unique pointer someplace in the LimitedQueue? not sure..).

@pajlada
Copy link
Member

pajlada commented Jun 10, 2022

@goldbattle Have you tested this functionality with more than 1000 messages in a channel? I suspect it would cause crashes before #3798 has been merged in

@Felanbird
Copy link
Collaborator

Seems interesting, just wondering how global search will like 8000+ message look-back in every channel.

I suspect it would cause crashes

Seems to be fine, tested with 1150 messages being render-able, seems the original issue is just loading all of them at once, 800 message history+350 new messages (and 1150 new is fine also).

@goldbattle
Copy link
Contributor Author

goldbattle commented Jun 10, 2022

I tested up to 5k messages for logs with 2k on screen. Works as expected, but the crash with inserting a lot of historical messages is still an issue in this PR. I saw #1421 a couple times when I ran the program.

EDIT: I tested searching and it works as expected and is responsive with 6k in the history.

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

The messages_ member of Channel does not need to be a shared_ptr, you just need to initialize it in the constructor (see how name_, type_, lastDate_ and completionModel are initialized in the constructor)

Get your branch up to date with the main branch since we've changed a lot of the LimitedQueue

@goldbattle goldbattle requested a review from pajlada July 28, 2022 02:48
src/singletons/Settings.hpp Outdated Show resolved Hide resolved
src/singletons/Settings.hpp Outdated Show resolved Hide resolved
src/widgets/settingspages/GeneralPage.cpp Outdated Show resolved Hide resolved
@goldbattle goldbattle requested a review from pajlada August 6, 2022 00:39
@goldbattle
Copy link
Contributor Author

Any updates on what is blocking this?

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@pajlada
Copy link
Member

pajlada commented Sep 19, 2022

Any updates on what is blocking this?

Code looks fine, I need to test this out a bit

@pajlada pajlada self-assigned this Sep 19, 2022
@goldbattle
Copy link
Contributor Author

Just want to re-ping on this one. Don't want it to get too far behind and have to keep it up to master. Let me know what I need to do.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

clang-tidy review says "All clean, LGTM! 👍"

UserInfoPopup is now responsible for creating a ChannelView with the
correct message limit
Split is now responsible for creating a ChannelView with the correct
message limit

The logic has been moved one step out to ensure we don't change the
logic for things like the emote popup
Also changes the step from 1000 to 100
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Changes look good to me now, I'd just like a final review from you @goldbattle to ensure it solves for your issues

My commits should be fairly well-organized to read through, but in short I have:

  1. Split up & reworded the changelog entries
  2. Ensured the ChannelView's limit is ONLY changed for Splits and UserInfoPopups (i.e. usercards)
  3. Renamed the settings & all phrasing around it to fit with our glossary

Once you've approved of my changes (or come with feedback) I'd be happy to merge this in.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@goldbattle
Copy link
Contributor Author

Looks good to me. I would still recommend that the default chat log is for usercard is increased Right now usercard log history isn't very helpful in streams > 10k viewers. But I am good either way, thanks for the cleanup.

@pajlada pajlada changed the title Add ability to specify number of messages in channel Add settings to increase split and usercard scrollback Nov 12, 2022
@pajlada pajlada enabled auto-merge (squash) November 12, 2022 15:35
@pajlada pajlada merged commit 3ed7489 into Chatterino:master Nov 12, 2022
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

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.

None yet

3 participants