Closed Bug 1549938 Opened 5 years ago Closed 5 years ago

OTR Chat: important consecutive inline system messages can be collapsed/hidden

Categories

(Chat Core :: Security: OTR, defect, P1)

Product:
Chat Core
Component:
Security: OTR
Type:
defect

Tracking

(thunderbird76 fixed)

Status:
RESOLVED FIXED
Milestone:
Instantbird 77
Tracking Flags:
Tracking Status
thunderbird76 --- fixed

People

(Reporter: KaiE, Assigned: aleca)

References

Details

Attachments

(1 file, 2 obsolete files)

1549938-otr-system-messages.diff
(deleted), patch
aleca
: review+
wsmwk
: approval-comm-beta+
Details | Diff | Splinter Review

When OTR chat is used, multiple consecutive system message may appear quickly.

Currently, the implementation will automatically collapse the most recent ones and hide them behind a plus button.

This can hide important information, like:
"the following message was received without encryption: hello"

We need to find a better solution to this.
Let's handle this after the initial landing was done.

Is this about the notifications? Notifications have an importance you can set so that important notifications get put on top of lesser important ones.

No. In between the chat messages typed by humans, additional system messages are shown inline. It's about those.

Good stuff, once we're closed to landing the initial OTR implementation, I can do some mock-ups to help find a better solution for those higher priority messages.

Assignee: nobody → alessandro
Component: General → Security: OTR
Priority: -- → P1
Status: NEW → ASSIGNED
Attached patch 1549938-otr-system-messages.diff (obsolete) (deleted) — — Splinter Review

This does the trick for me.
I implemented a more generic noCollapse attribute tot he conversation object in order to be able to manage that feature independently from OTR, if we will ever need it.
The not-hiding part is done in CSS.

Attachment #9139341 - Flags: review?(kaie)
Attachment #9139341 - Flags: review?(clokep)
Comment on attachment 9139341 [details] [diff] [review] 1549938-otr-system-messages.diff Review of attachment 9139341 [details] [diff] [review]: ----------------------------------------------------------------- I think this looks good overall! I left a couple of minor comments (pretty much asking for updated comments in a couple of places). ::: chat/components/public/prplIMessage.idl @@ +64,5 @@ > /* PURPLE_MESSAGE_NOTIFY = 0x2000, /**< Message is a notification */ > readonly attribute boolean notification; > /* PURPLE_MESSAGE_NO_LINKIFY = 0x4000 /**< Message should not be auto-linkified */ > readonly attribute boolean noLinkification; > + /* PURPLE_MESSAGE_NO_COLLAPSE = 0x0040, /**< Do not collapse the message. */ It doesn't really matter since we don't actually use these constants, but this overlaps with noLog from above. We should probably just remove these comments at some point, we're no longer compatible with libpurple. Maybe just include the "Do not collapse" part and let's not pour more bad comments on top? ::: chat/modules/OTR.jsm @@ -1135,5 @@ > } > }, > > sendAlert(context, msg) { > - this.getUIConvFromContext(context).systemMessage(msg); Why does this not use `notificationOTR`? It is unclear to me what that method is for if we're not using it to send OTR messages... :) Maybe add a docstring to that method if it only to be used in specific cases? ::: chat/modules/imThemes.jsm @@ +474,5 @@ > if (aMsg.noFormat) { > msgClass.push("monospaced"); > } > + if (aMsg.noCollapse) { > + msgClass.push("no-collapse"); Please update our docs with this new class: https://developer.mozilla.org/en-US/docs/Mozilla/Chat_Core/Message_Styles#Replacement_in_both_messages_and_status_messages (I suspect these docs should be migrated, by the way, but I'm unsure where to...)

Why does this not use notificationOTR? It is unclear to me what that method is for if we're not using it to send OTR messages... :)
Maybe add a docstring to that method if it only to be used in specific cases?

The notificationOTR is used only to handle the first message you receive, when the chat buddy asks for a verification, and if you're don't have the focus on the chat.
That method will trigger a chat sound, alongside a counter update, as well as triggering a global notification to answer the verification request.
Indeed, a comment is much needed.

Please update our docs with this new class: https://developer.mozilla.org/en-US/docs/Mozilla/Chat_Core/Message_Styles#Replacement_in_both_messages_and_status_messages

I will once this lands.
We should probably migrate this to out DTN. I'll open a bug on GitHub.

Attached patch 1549938-otr-system-messages.diff (obsolete) (deleted) — — Splinter Review
Attachment #9139341 - Attachment is obsolete: true
Attachment #9139341 - Flags: review?(kaie)
Attachment #9139341 - Flags: review?(clokep)
Attachment #9139892 - Flags: review?(kaie)
Attachment #9139892 - Flags: review?(clokep)

Did you guys have the chance to review this?

(In reply to Alessandro Castellani (:aleca) from comment #6)

The notificationOTR is used only to handle the first message you receive, when the chat buddy asks for a verification, and if you're don't have the focus on the chat.
That method will trigger a chat sound, alongside a counter update, as well as triggering a global notification to answer the verification request.
Indeed, a comment is much needed.

Maybe rename the function to notifyVerifyOTR ?

Attachment #9139892 - Flags: review?(kaie) → review+

Thanks for the review, it needs a small rebase and here's a try-run to be sure we're not breaking any test: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b951d62397d81b9390b0f9ea22d5586053a3bf75

Do we have any OTR related tests?

Attachment #9139892 - Attachment is obsolete: true
Attachment #9139892 - Flags: review?(clokep)
Attachment #9140530 - Flags: review+

(In reply to Alessandro Castellani (:aleca) from comment #10)

Do we have any OTR related tests?

I'm afraid we don't.

Keywords: checkin-needed-tb
Target Milestone: --- → Instandbird 77

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/c3d742c66c80
[OTR] Prevent important consecutive inline system messages from being collapsed. r=KaiE

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
Comment on attachment 9140530 [details] [diff] [review] 1549938-otr-system-messages.diff [Approval Request Comment] Regression caused by (bug #): User impact if declined: Testing completed (on c-c, etc.): Risk to taking this patch (and alternatives if risky):
Attachment #9140530 - Flags: approval-comm-beta?
Comment on attachment 9140530 [details] [diff] [review] 1549938-otr-system-messages.diff We want to iterate on OTR issues before 78, so taking for beta.
Attachment #9140530 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: