Closed
Bug 1002621
Opened 8 years ago
Closed 8 years ago
Unfork buddytooltip.xml
Categories
(Thunderbird :: Instant Messaging, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 32.0
People
(Reporter: aleth, Assigned: aleth)
References
Details
Attachments
(2 files, 2 obsolete files)
65.88 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
1.27 KB,
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
There appears to be no good reason for this to be forked, and maintaining it is painful.
Assignee | ||
Comment 1•8 years ago
|
||
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•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
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•8 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 5•8 years ago
|
||
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•8 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 7•8 years ago
|
||
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•8 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.
Comment 9•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #8417922 -
Flags: review?(florian) → review+
Comment 10•8 years ago
|
||
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•8 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•8 years ago
|
||
https://hg.mozilla.org/comm-central/rev/0450f5bc266d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Version: Trunk → 32
Assignee | ||
Updated•8 years ago
|
Target Milestone: --- → Thunderbird 32.0
Comment 13•8 years ago
|
||
Attachment #8421394 -
Flags: review?(aleth)
Assignee | ||
Comment 14•8 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•7 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•