Unfork buddytooltip.xml

RESOLVED FIXED in Thunderbird 32.0

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

32 Branch
Thunderbird 32.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
There appears to be no good reason for this to be forked, and maintaining it is painful.
(Assignee)

Comment 1

5 years ago
Posted patch unfork.patch (obsolete) — Splinter Review
Only required a single TB/IB if clause and one duplicated string. This also ports a couple of recent fixes/enhancements to TB.
Attachment #8414339 - Flags: review?(florian)
(Assignee)

Updated

5 years ago
Blocks: 1003200
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Posted patch unforker.patch 2 (obsolete) — Splinter Review
Changed to stop the "Tags" entry appearing in TB where it is pointless.
Attachment #8414339 - Attachment is obsolete: true
Attachment #8414339 - Flags: review?(florian)
Attachment #8417300 - Flags: review?(florian)
Comment on attachment 8417300 [details] [diff] [review]
unforker.patch 2

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

::: chat/content/buddytooltip.xml
@@ +412,2 @@
>  
>           let localName = elt.localName;

Can we think of anything better than checking for a bunch of possible localName values here?

Maybe checking directly for the attributes we care about? (eg checking only for elt.conv rather than localName == "(im)conv" && elt.conv)?

::: chat/locales/en-US/buddytooltip.properties
@@ +8,5 @@
> +
> +#LOCALIZATION NOTE
> +# Should match the corresponding string in instantbird.properties
> +# (and chat.properties for Thunderbird).
> +noTopic=No topic message for this room.

Duplicating this is unfortunate :(.

::: chat/themes/buddytooltip.css
@@ -15,5 @@
>    min-width: 16px;
>    min-height: 16px;
>    width: 16px;
>    height: 16px;
> -  -moz-appearance: none;

Do you know why this is no longer needed?
(Assignee)

Comment 4

5 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> ::: chat/content/buddytooltip.xml
> @@ +412,2 @@
> >  
> >           let localName = elt.localName;
> 
> Can we think of anything better than checking for a bunch of possible
> localName values here?
> 
> Maybe checking directly for the attributes we care about? (eg checking only
> for elt.conv rather than localName == "(im)conv" && elt.conv)?

I don't think that's very safe. It also wouldn't work to distinguish e.g. imcontact from contact (both have elt.contact) and I don't see how it could work for listitems and tabs.
Comment on attachment 8417300 [details] [diff] [review]
unforker.patch 2

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

::: chat/content/buddytooltip.xml
@@ +412,2 @@
>  
>           let localName = elt.localName;

Per IRC discussion, we are going to use an ifdef to address this.
Attachment #8417300 - Flags: review?(florian) → review-
(Assignee)

Comment 6

5 years ago
* s/buddytooltip/imtooltip
* No more duplicated strings
* ifdef TB instead of if TB
Attachment #8417300 - Attachment is obsolete: true
Attachment #8417922 - Flags: review?(florian)
Comment on attachment 8417922 [details] [diff] [review]
unforker.patch 3

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

One question. The rest looks ready to go :-).

::: im/components/ibConvStatsService.js
@@ +753,5 @@
>    get displayName() this._roomInfo.name,
>    get lowerCaseName()
>      this._lowerCaseName || (this._lowerCaseName = this.displayName.toLowerCase()),
> +  get statusText()
> +    "(" + this._roomInfo.participantCount + ") " + this._roomInfo.topic,

Are we no longer displaying the No Topic string in the awesometab with this change? Or is _roomInfo.topic now handling the No Topic string?
(Assignee)

Comment 8

5 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> Are we no longer displaying the No Topic string in the awesometab with this
> change? Or is _roomInfo.topic now handling the No Topic string?

We're no longer displaying it. I think we shouldn't have been displaying it there in the first place, it's just visual noise.
(In reply to aleth [:aleth] from comment #8)
> (In reply to Florian Quèze [:florian] [:flo] from comment #7)
> > Are we no longer displaying the No Topic string in the awesometab with this
> > change? Or is _roomInfo.topic now handling the No Topic string?
> 
> We're no longer displaying it. I think we shouldn't have been displaying it
> there in the first place, it's just visual noise.

Ok.
Attachment #8417922 - Flags: review?(florian) → review+
Comment on attachment 8417922 [details] [diff] [review]
unforker.patch 3

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

::: chat/locales/jar.mn
@@ +6,5 @@
>  
>  @AB_CD@.jar:
>  % locale chat @AB_CD@ %locale/@AB_CD@/chat/
>  	locale/@AB_CD@/chat/accounts.properties (%accounts.properties)
> +	locale/@AB_CD@/chat/imtooltip.properties (%imtooltip.properties)

I don't see imtooltip.properties defined anywhere. Did you forget to hg add it, or am I blind?
(Assignee)

Comment 11

5 years ago
(In reply to Patrick Cloke [:clokep] from comment #10)
> I don't see imtooltip.properties defined anywhere. Did you forget to hg add
> it, or am I blind?

It's a hg mv.
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/comm-central/rev/0450f5bc266d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Version: Trunk → 32
(Assignee)

Updated

5 years ago
Target Milestone: --- → Thunderbird 32.0
(Assignee)

Comment 14

5 years ago
Comment on attachment 8421394 [details] [diff] [review]
Remove status.css from jar.mn for Windows aero

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

Thanks!
Attachment #8421394 - Flags: review?(aleth) → review+
(Assignee)

Updated

5 years ago
Depends on: 1013236
(Assignee)

Updated

4 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.