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

chat: fix issues with the initial "New Chat" #2330

Merged
merged 5 commits into from May 15, 2024
Merged

Conversation

cebtenzzre
Copy link
Member

At startup:

  • Wait until the conversationList view has been connected to the ChatListModel before adding any chats to the list. Otherwise, the initial "New Chat" tends to be scrolled just above the top of the view if the user has enough chats.

When the "New chat" button is clicked:

  • If there is already a new chat, select the existing new chat instead of doing nothing. Buttons that do nothing are frustrating.
  • When the new chat button is clicked, always scroll to the top of the list, so the chat is visible.

Also, make the connections to ChatRestoreThread explicitly queued. I'm pretty sure m_chats was being modified from more than one thread without this change.

Signed-off-by: Jared Van Bortel <jared@nomic.ai>
Previously, the "New chat" button did absolutely nothing if there was
already a new chat.

Signed-off-by: Jared Van Bortel <jared@nomic.ai>
If the user clicks "New chat" while the top of the list is not visible,
we would previously select a chat that the user can't see.

Signed-off-by: Jared Van Bortel <jared@nomic.ai>
If we add the initial new chat before the view is connected to the
model, it ends up scrolled just above the top of the view if enough
chats are deserialized.

Signed-off-by: Jared Van Bortel <jared@nomic.ai>
m_chats is not guarded by a mutex, so we should only modify it from a
single thread.

Signed-off-by: Jared Van Bortel <jared@nomic.ai>
@cebtenzzre cebtenzzre requested a review from manyoso May 9, 2024 21:56
@manyoso
Copy link
Collaborator

manyoso commented May 15, 2024

I'd like to huddle about this one as well, because I'm not sure what this is fixing and I have questions that the explicit queuing of the connection is needed. To be sure, there is nothing wrong with making it explicitly queued, but I'd like to investigate to be sure this is actually necessary.

@cebtenzzre
Copy link
Member Author

I have questions that the explicit queuing of the connection is needed. To be sure, there is nothing wrong with making it explicitly queued, but I'd like to investigate to be sure this is actually necessary.

Heh, because this codebase is full of Qt::QueuedConnection whenever threads are involved, I didn't realize that Qt automatically queues signals when the sender and receiver are in different threads. Not actually a necessary change then, but probably good to have for consistency.

@cebtenzzre cebtenzzre merged commit fbbf810 into main May 15, 2024
6 of 10 checks passed
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

2 participants