Closed Bug 1621952 Opened 5 years ago Closed 4 years ago

Icons for Matrix conversations

Categories

(Chat Core :: Matrix, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird_esr78 wontfix)

RESOLVED FIXED
90 Branch
Tracking Status
thunderbird_esr78 --- wontfix

People

(Reporter: clokep, Assigned: clokep)

References

Details

Attachments

(3 files, 3 obsolete files)

Matrix supports icons for any type of conversation, unfortunately the chat framework only supports buddy icons (e.g. icons for individual conversations, not chats).

We should:

  1. Extend the backend to support icons for any type of conversation.
  2. Pass through the icons from Matrix for all conversations.

Note that we currently support icons in tooltips, so there's already some code to retrieve / parse icons from the matrix-js-sdk.

Attached patch Patch v1 (obsolete) — Splinter Review
  • Allow conversations (both chat or IM) to set an icon. (IM conversations fallback to the buddy icon.)
  • Implement using the m.room.avatar for Matrix conversations, if available.

With this change there's a few states the image can be in:

  • Chat conversations:
    • No icon if the chat has no icon.
    • An icon if the chat has an icon set (currently this is only for Matrix, I don't think any other protocols implement this).
  • IM conversations:
    • An icon from the conversation, if one is set. (Matrix can override the icon that's used.)
    • The icon from the buddy object, if one exists.
    • The fallback icon (showing the outline of a human).

No icon vs. the fallback is set by using null vs. an empty string which I don't really love. I wonder if it would be nicer to have a different fallback icon for chats?

Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attachment #9220408 - Flags: review?(martin)
Comment on attachment 9220408 [details] [diff] [review] Patch v1 Review of attachment 9220408 [details] [diff] [review]: ----------------------------------------------------------------- Fails to apply on c-c due to the m.room.tombstone patch having landed. Also, this doesn't add the icon to the tooltips, but I guess that's forgiven due to the maze that tooltips are. ::: chat/components/public/prplIConversation.idl @@ +41,5 @@ > /* Unique identifier of the conversation */ > /* Setable only once by purpleCoreService while calling addConversation. */ > attribute unsigned long id; > > + /** URI of the icon for the conversation */ Should this mention the null vs. empty behavior? ::: chat/protocols/matrix/matrix.jsm @@ +232,5 @@ > } > + > + const avatarUrl = room.getAvatarUrl(this._account._baseURL); > + if (avatarUrl) { > + this.convIconFilename = room.getAvatarUrl(this._account._baseURL) || ""; Should we set a size we want here? The #addons:mozilla.org room has a default image size that is big enough for me to see the image loading (and even make the ui lag a bit?)... I guess we should also limit size on user avatars. @@ +541,5 @@ > } > return false; > }, > + > + get iconFilename() { This is unused I think? @@ +622,5 @@ > } > } > }, > + > + get iconFilename() { Also unused?
Attachment #9220408 - Flags: review?(martin) → review-

Should this mention the null vs. empty behavior?

I was actually hoping to push this behavior into the UI layer for that to decide about whether it should show the fallback based on the type of conversation, but it seems like the right info might not be available when necessary to show that... Maybe I'll do that as a separate patch.

Attached patch Patch v2 (obsolete) — Splinter Review

This is a pretty big update that:

  • Correctly sets the size each time we pull Matrix avatars.
  • Removes unused code from an older version of this patch.
  • Pushes the null vs. undefined vs. "" behavior into the UI by telling it directly whether to use a fallback icon.
  • Supports tooltips.
  • Unbitrots / rebases.
Attachment #9220408 - Attachment is obsolete: true
Attachment #9220819 - Flags: review?(martin)
Comment on attachment 9220819 [details] [diff] [review] Patch v2 Review of attachment 9220819 [details] [diff] [review]: ----------------------------------------------------------------- So close, this is going to be great :) ::: chat/content/chat-tooltip.js @@ +318,5 @@ > + * fallback, or null to hide the icon. > + * @param {boolean} useFallback - True if the "fallback" icon should be shown > + * if iconUri isn't provided. > + */ > + setUserIcon(iconUri, useFalback) { useFallback seems unused and instead we always pass true. This leads to many rooms ending up looking like humans ;) @@ +344,5 @@ > + /** > + * Regenerate the tooltip based on a buddy. > + * > + * @param {prplIAccountBuddy} aBuddy - The buddy to generate the conversation. > + * @param {imIConversation|null} aConv - A conversation associated with this buddy. aConv is optional/can be left out? If so, use [aConv] @@ +479,5 @@ > this.setAttribute("displayname", aNick); > this.setProtocolIcon(account.protocol); > this.setStatusIcon("unknown"); > this.setMessage(LazyModules.Status.toLabel("unknown")); > + this.setUserIcon(aParticipant.buddyIconFilename, true); If you make this aParticipant?.buddyIconFilename this also fixes the tooltip bug with missing participants in scrollback. ::: chat/protocols/matrix/matrix.jsm @@ +42,5 @@ > InteractiveBrowser: "resource:///modules/InteractiveBrowser.jsm", > getMatrixTextForEvent: "resource:///modules/matrixTextForEvent.jsm", > }); > > +// This matches the configuration of the .userIcon class in chat.css. as an aside, should we be respecting dpi scaling? @@ +44,5 @@ > }); > > +// This matches the configuration of the .userIcon class in chat.css. > +const USER_ICON_WIDTH = 48; > +const USER_ICON_HEIGHT = 48; Should we just use one "USER_ICON_SIZE" constant because our UI assumes square avatars? ::: mail/components/im/content/chat-conversation.js @@ +1622,5 @@ > let statusText = ""; > let statusType = Ci.imIStatusInfo.STATUS_UNKNOWN; > > let buddy = this._conv.buddy; > + if (buddy && buddy.account.connected) { Could probably just use optional chaining?
Attachment #9220819 - Flags: review?(martin) → review-
Attached patch Patch v3 (obsolete) — Splinter Review

If you make this aParticipant?.buddyIconFilename this also fixes the tooltip bug with missing participants in scrollback.

Is there a separate bug filed about this?

as an aside, should we be respecting dpi scaling?

Probably, but I think that's a bit orthogonal to this. Can we file a separate issue about that? (It also applies to other protocols most likely?)

I think I hit the other comments!

Attachment #9220819 - Attachment is obsolete: true
Attachment #9220839 - Flags: review?(martin)
Comment on attachment 9220839 [details] [diff] [review] Patch v3 Review of attachment 9220839 [details] [diff] [review]: ----------------------------------------------------------------- Just one small nit, but apart from that it works perfectly. ::: chat/content/chat-tooltip.js @@ +348,5 @@ > + /** > + * Regenerate the tooltip based on a buddy. > + * > + * @param {prplIAccountBuddy} aBuddy - The buddy to generate the conversation. > + * @param [imIConversation] aConv - A conversation associated with this buddy. almost, but not quite. The syntax is {imIConversation} [aConv] because the param being optional is somehow a property of the param and not its type. ::: mail/components/im/content/imStatusSelector.js @@ +20,5 @@ > displayUserIcon() { > let icon = Services.core.globalUserStatus.getUserIcon(); > ChatIcons.setUserIconSrc( > document.getElementById("userIcon"), > + icon?.spec ?? "", Probably technically don't need the fallback to empty string anymore.
Attachment #9220839 - Flags: review?(martin) → review+

(In reply to Patrick Cloke [:clokep] from comment #7)

If you make this aParticipant?.buddyIconFilename this also fixes the tooltip bug with missing participants in scrollback.

Is there a separate bug filed about this?

I don't think so.

as an aside, should we be respecting dpi scaling?

Probably, but I think that's a bit orthogonal to this. Can we file a separate issue about that? (It also applies to other protocols most likely?)

Sure, will do. Though I think Matrix is the only one that can control the size, it seems XMPP just has a base64 encoded image in the vCard.

Attached patch Patch v3.1Splinter Review

Thank you for the reviews!

Attachment #9220839 - Attachment is obsolete: true
Attachment #9220853 - Flags: review+

(currently this is only for Matrix, I don't think any other protocols implement this).

Martin had pointed out a proto-XEP for XMPP for this: https://xmpp.org/extensions/inbox/muc-avatars.html if this gets standardized we should be able to add support easily, but seems too early still. Wanted to include it for posterity though.

Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/d825d65d09eb
Use conversation icons for Matrix. r=freaktechnik

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
See Also: → 1710207
See Also: → 1730329
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: