Closed Bug 1538548 Opened 6 years ago Closed 6 years ago

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

Categories

(Thunderbird :: Instant Messaging, task)

Product:
Thunderbird
Component:
Instant Messaging
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0

People

(Reporter: mkmelin, Assigned: khushil324)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug-1538548_de-xbl_imconv.patch
(deleted), patch
mkmelin
: review+
Details | Diff | Splinter Review
1538548.patch fix eslint
(deleted), patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
Attached patch Bug-1538548_de-xbl_imconv.patch (obsolete) (deleted) — — Splinter Review

Now Bug 1543906 no longer exists.

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

Attachment #9059441 - Flags: review?(mkmelin+mozilla)
Attachment #9059441 - Flags: feedback?(richard.marti)
Attached patch Bug-1538548_de-xbl_imconv.patch (obsolete) (deleted) — — Splinter Review
Attachment #9059441 - Attachment is obsolete: true
Attachment #9059441 - Flags: review?(mkmelin+mozilla)
Attachment #9059441 - Flags: feedback?(richard.marti)
Attachment #9059443 - Flags: review?(mkmelin+mozilla)
Attachment #9059443 - Flags: feedback?(richard.marti)
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.
Attachment #9059443 - Flags: feedback?(richard.marti) → feedback+
Blocks: 1543906
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
Attachment #9059443 - Flags: review?(mkmelin+mozilla)
Attached patch Bug-1538548_de-xbl_imconv.patch (deleted) — — Splinter Review
Attachment #9059443 - Attachment is obsolete: true
Attachment #9060704 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9060704 - Flags: review?(mkmelin+mozilla) → review+

[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.]

Status: NEW → ASSIGNED
Type: defect → task

(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.

Keywords: checkin-needed

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

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0

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)

Attached patch 1538548.patch fix eslint (deleted) — — Splinter Review
Attachment #9060936 - Flags: review?(jorgk)
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.
Attachment #9060936 - Flags: review?(jorgk) → review+
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---

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

Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Regressions: 1555136
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: