1538548 - [de-xbl] convert the imconv binding to <richlistitem is="chat-imconv">

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, task)

Description

[Magnus Melin [:mkmelin]]

The imconv binding should be a customized built-in based on richlistitem

https://searchfox.org/comm-central/rev/47187196fbf02cc0d635371b3879ddf5a49f491c/mail/components/im/content/imconv.xml

https://searchfox.org/comm-central/rev/47187196fbf02cc0d635371b3879ddf5a49f491c/mail/components/im/content/chat.css#32

https://searchfox.org/comm-central/rev/47187196fbf02cc0d635371b3879ddf5a49f491c/mail/components/im/content/chat-messenger.inc.xul#103

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1538548_de-xbl_imconv.patch

Now Bug 1543906 no longer exists.

Also, I have also updated patch according to https://bugzilla.mozilla.org/show_bug.cgi?id=1545398#c4

Comment 2

[Khushil Mistry [:khushil324]]

Attachment: Bug-1538548_de-xbl_imconv.patch

Comment 3

[Richard Marti (:Paenglab)]

Comment on attachment 9059443 [details] [diff] [review]
Bug-1538548_de-xbl_imconv.patch

Review of attachment 9059443 [details] [diff] [review]:
-----------------------------------------------------------------

f+ with the [unread] fixed and the following comment considered.

You can change all

:-moz-any(richlistitem[is="chat-imconv"], richlistitem[is="chat-contact"], richlistitem[is="chat-group"])

to

richlistitem:-moz-any([is="chat-imconv"], [is="chat-contact"], [is="chat-group"])

which makes the line shorter and better readable.

::: mail/components/im/themes/chat.css
@@ +115,4 @@
>  }
>  
>  richlistitem[is="chat-group"],
> +richlistitem[is="chat-imconv"] {

The [unread] is missing here.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9059443 [details] [diff] [review]
Bug-1538548_de-xbl_imconv.patch

Review of attachment 9059443 [details] [diff] [review]:
-----------------------------------------------------------------

Will try it out once you fix the items noted here

::: mail/components/im/content/chat-imconv.js
@@ +12,4 @@
>  
> +  /**
> +    * The MozChatConv widget displays conversation information about the user:
> +    * i.e name and icon. It gets displayed under Conversation expansion twisty

something wrong with the language here. And why is Conversation capitalized?

@@ +106,5 @@
> +    get displayName() {
> +      return this.conv.title;
> +    }
> +    /**
> +     * for compatibility with the  imgroup sortComparator

Add a blank line before the /**
And remove the double space. Capitalize, and period. so:

"This getter exists to provide compatibility with the imgroup sortComparator.

@@ +247,2 @@
>  
> +      let isMac = "nsILocalFileMac" in Ci;

Let's instead do

let accelKeyPressed = (AppConstants.platform == "macosx") ? event.metaKey : event.ctrlKey;

::: mail/components/im/content/chat-messenger.inc.xul
@@ +101,4 @@
>                               tooltip="imTooltip" flex="1">
>                    <richlistitem is="chat-group" id="conversationsGroup"
>                                  name="&conversationsHeader.label;"/>
> +                  <richlistitem is="chat-imconv" id="searchResultConv" displayname="&searchResultConversation.label;" hidden="true"/>

please new line for displayname and hiddden

::: mail/components/im/content/chat-messenger.js
@@ +284,5 @@
>      let shouldSelect =
>        gChatTab && gChatTab.tabNode.selected &&
>        (!selectedItem || (selectedItem == convs &&
> +                        convs.nextSibling.localName != "richlistitem" &&
> +                        convs.nextSibling.getAttribute("is") != "chat-imconv"));

these should indent by one more space since it's inside the parenthesis

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug-1538548_de-xbl_imconv.patch

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9060704 [details] [diff] [review]
Bug-1538548_de-xbl_imconv.patch

Review of attachment 9060704 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good thx! r=mkmelin

::: mail/components/im/content/chat-imconv.js
@@ +145,5 @@
> +        }
> +        this.setAttribute("unreadCount", "0");
> +        this.setAttribute("unreadTargetedCount", "0");
> +        this.removeAttribute("unread");
> +        this.removeAttribute("attention");

Please file a follow-up bug to make unread and attention classes only. There's no need for these to be custom attributes. If the are we should really listen to changes to them too.

Comment 7

[Magnus Melin [:mkmelin]]

[It's good to put bugs with patches (or at least active ongoing work) into ASSIGNED state so it's easier to get an overview.]

Comment 8

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #6)

Please file a follow-up bug to make unread and attention classes only.
There's no need for these to be custom attributes. If the are we should
really listen to changes to them too.

Sure, will do it in a while.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dc7943c0ecc9
[de-xbl] convert the chat's conv binding to <richlistitem is='chat-imconv'>. r=mkmelin DONTBUILD

Comment 10

[Jorg K (CEST = GMT+2)]

You (author and reviewer) should run the linter locally :-(
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/im/content/chat-imconv.js:249:30 | 'AppConstants' is not defined. (no-undef)

Comment 11

[:aceman]

Attachment: 1538548.patch fix eslint

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 9060936 [details] [diff] [review]
1538548.patch fix eslint

Thanks, I'll get this landed later since I've blown all my patches now.
EDIT: I forgot to say: I tried a local build, and chat appeared to be working without the patch.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c3ebd6f3872c
Follow-up: fix missing AppConstants in mail/components/im/content/chat-imconv.js. r=jorgk