-
Notifications
You must be signed in to change notification settings - Fork 23
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
Update updatedAtTime
only on sending a message instead of receiving it (fixes #1248)
#1278
Update updatedAtTime
only on sending a message instead of receiving it (fixes #1248)
#1278
Conversation
updatedAtTime
only on sending a message instead of receiving it (fixes #1248)
@@ -41,7 +41,7 @@ import javax.swing.tree.DefaultTreeModel | |||
import javax.swing.tree.TreePath | |||
import javax.swing.tree.TreeSelectionModel | |||
|
|||
class HistoryTree( | |||
class ChatHistoryPanel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NTOE: I piggyback this change.
This class is a panel actually, it extends SimpleToolWindowPanel
.
val found = getOrCreateChat(internalId) | ||
found.messages = chatMessages.map(::convertToMessageState).toMutableList() | ||
if (chatMessages.lastOrNull()?.speaker == Speaker.HUMAN) { | ||
if (shouldUpdateTime) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get all that changes, and they are correct, but I do not find that variable passing particularly clean.
I was thinking if we can simplify it somehow and I get one idea:
Shouldn't we update only, if found.messages
count increased compared to what already is in the history? What would be one-line local check in this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, sounds good!
@@ -253,6 +253,7 @@ private constructor( | |||
|
|||
ChatMessage(speaker = parsed, message.text) | |||
} | |||
this.messages.addAll(chatMessages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed and reasonable too. Let's add all these message in advance. addMessageAtIndex
will not even execute in receiveMessage
as we check the condition: sessionMessage != incomingMessage
.
f06dd71
to
96b3eea
Compare
@@ -140,7 +140,9 @@ class HistoryTree( | |||
} | |||
} else { | |||
val currentPeriodText = DurationGroupFormatter.format(chat.getUpdatedTimeAt()) | |||
val currentPeriod = root.periods().find { it.periodText == currentPeriodText } ?: return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a bug 🐛
24fbc1a
to
65ac961
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #1248.
Test plan
See the issue for specific steps.
Expected:
The restored chat remains as the last one in the list (it is not bumped to the top).