1002621 - Unfork buddytooltip.xml

Status: RESOLVED FIXED

Description

[aleth [:aleth]]

There appears to be no good reason for this to be forked, and maintaining it is painful.

Comment 1

[aleth [:aleth]]

Attachment: unfork.patch

Only required a single TB/IB if clause and one duplicated string. This also ports a couple of recent fixes/enhancements to TB.

Comment 2

[aleth [:aleth]]

Attachment: unforker.patch 2

Changed to stop the "Tags" entry appearing in TB where it is pointless.

Comment 3

[Florian Quèze [: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?

Comment 4

[aleth [:aleth]]

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

[Florian Quèze [: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;

Per IRC discussion, we are going to use an ifdef to address this.

Comment 6

[aleth [:aleth]]

Attachment: unforker.patch 3

* s/buddytooltip/imtooltip
* No more duplicated strings
* ifdef TB instead of if TB

Comment 7

[Florian Quèze [: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?

Comment 8

[aleth [:aleth]]

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

Comment 9

[Florian Quèze [:florian]]

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

Comment 10

[Patrick Cloke [:clokep]]

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?

Comment 11

[aleth [:aleth]]

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

Comment 12

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/0450f5bc266d

Comment 13

[Patrick Cloke [:clokep]]

Attachment: Remove status.css from jar.mn for Windows aero

Comment 14

[aleth [:aleth]]

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

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

Thanks!

Comment 15

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/a232575572bb