Fix up order and formatting of of IRC tooltips.

RESOLVED FIXED in 1.2

Status

--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Mic, Assigned: clokep)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
*** Original post on bio 1427 at 2012-05-06 21:03:00 UTC ***

I always feel these fields should sit next to each other on IRC tooltips. Please 'wontfix' or resolve as invalid if you think that is silly ;)
(Assignee)

Comment 1

5 years ago
*** Original post on bio 1427 at 2012-05-06 21:43:27 UTC ***

I'd actually like the order to be hard coded as well to give the same display every time.
(Assignee)

Updated

5 years ago
Summary: Show "Connected to" and "Connected from" in IRC tooltips next to each other → Fix up order and formatting of of IRC tooltips.
(Assignee)

Comment 2

5 years ago
Created attachment 8353315 [details] [diff] [review]
Patch v1

*** Original post on bio 1427 as attmnt 1560 at 2012-06-05 02:08:00 UTC ***

This is really just porting over some Twitter code [1] to IRC (I wonder if other protocols will want this code, should it be abstracted somehow?)

Anyway, it enforces an order and allows formatting functions to be used (as with Twitter). This isn't super important now, but it actually blocks bug 954728 (bio 1296), which I have a partial patch done for. (Pretty much all the responses in that bug are just booleans: Are you using a secure connection? Are you registered?)

[1] http://lxr.instantbird.org/instantbird/source/chat/protocols/twitter/twitter.js#925
Attachment #8353315 - Flags: review?(bugzilla)
(Assignee)

Updated

5 years ago
Assignee: nobody → clokep
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
*** Original post on bio 1427 at 2012-06-05 10:20:21 UTC ***

So I realized I'm missing the l10n changes (the strings for yes and no), which makes this an r-. But please review the rest and I'll post an updated patch with the new strings. Thanks!

Comment 4

5 years ago
Comment on attachment 8353315 [details] [diff] [review]
Patch v1

*** Original change on bio 1427 attmnt 1560 at 2012-06-05 12:29:21 UTC ***

This looks good overall.

Please add to the comment that the first kFields entry maps to tooltip.* in the l10n. (It's obvious from the code, but still...)

Should tooltip.serverValue now be moved to a transform function for consistency?

What happens with whois fields that are not in the list? Currently they are ignored (on purpose I think): can you be sure you have them all?
Attachment #8353315 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 5

5 years ago
*** Original post on bio 1427 at 2012-06-05 12:40:19 UTC ***

(In reply to comment #4)
> Comment on attachment 8353315 [details] [diff] [review] (bio-attmnt 1560) [details]
> Patch v1
> 
> This looks good overall.
> 
> Please add to the comment that the first kFields entry maps to tooltip.* in the
> l10n. (It's obvious from the code, but still...)
I can do that. :)

> Should tooltip.serverValue now be moved to a transform function for
> consistency?
I'm not sure what you mean here. What would be moved into the transform, isn't it just a string?

> What happens with whois fields that are not in the list? Currently they are
> ignored (on purpose I think): can you be sure you have them all?
I'm not sure I understand this question. We iterate over each field we want, if it has a value we show it, otherwise we ignore it. If a field isn't wanted it isn't listed.

Comment 6

5 years ago
*** Original post on bio 1427 at 2012-06-05 12:51:01 UTC ***

(In reply to comment #5)
> > Should tooltip.serverValue now be moved to a transform function for
> > consistency?
> I'm not sure what you mean here. What would be moved into the transform, isn't
> it just a string?

It's just a string that is used here http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/ircBase.jsm#688, but it does construct a value (like a transform).

I think the best thing to do would be to leave it as is, but move tooltip.serverValue in irc.properties so it's clear it's not one of the tooltip fields.

> > What happens with whois fields that are not in the list? Currently they are
> > ignored (on purpose I think): can you be sure you have them all?
> I'm not sure I understand this question. We iterate over each field we want, if
> it has a value we show it, otherwise we ignore it. If a field isn't wanted it
> isn't listed.

Yes, my question was "are we sure we know all the fields that are wanted" ;) i.e. are there possibly server- or service-specific entries we don't currently handle explicitly.
(Assignee)

Comment 7

5 years ago
Created attachment 8353322 [details] [diff] [review]
Patch v2

*** Original post on bio 1427 as attmnt 1567 at 2012-06-05 22:47:00 UTC ***

Changes the order of the strings in irc.properties and includes the yes/no strings. Also meets the other review comments.
Attachment #8353322 - Flags: review?(bugzilla)
(Assignee)

Comment 8

5 years ago
Comment on attachment 8353315 [details] [diff] [review]
Patch v1

*** Original change on bio 1427 attmnt 1560 at 2012-06-05 22:47:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353315 - Attachment is obsolete: true

Comment 9

5 years ago
Comment on attachment 8353322 [details] [diff] [review]
Patch v2

*** Original change on bio 1427 attmnt 1567 at 2012-06-05 23:10:41 UTC ***

Looks good and works!

If 312 is in response to WHOWAS rather than WHOIS, serverLocation actually contains a timestamp rather than a location. Not sure if it is worth changing the string? Maybe for localizers...

We don't always receive a 312 in response to WHOIS/WHOWAS. This assumption is made in the serverValue call. Example: killer in #developers is a Service and we only get a 313 for that.
Attachment #8353322 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 10

5 years ago
Created attachment 8353323 [details] [diff] [review]
Patch v3

*** Original post on bio 1427 as attmnt 1568 at 2012-06-05 23:22:00 UTC ***

Wraps the serverValue call in an if statement & adds a more verbose comment for localizers.
Attachment #8353323 - Flags: review?(bugzilla)
(Assignee)

Comment 11

5 years ago
Comment on attachment 8353322 [details] [diff] [review]
Patch v2

*** Original change on bio 1427 attmnt 1567 at 2012-06-05 23:22:25 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353322 - Attachment is obsolete: true

Comment 12

5 years ago
Comment on attachment 8353323 [details] [diff] [review]
Patch v3

*** Original change on bio 1427 attmnt 1568 at 2012-06-05 23:26:47 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353323 - Flags: review?(bugzilla) → review+

Updated

5 years ago
Whiteboard: [checkin-needed]

Comment 13

5 years ago
*** Original post on bio 1427 at 2012-06-08 08:16:02 UTC ***

http://hg.instantbird.org/instantbird/rev/ef344ab33302

Thanks for adding this!

Comment 14

5 years ago
*** Original post on bio 1427 at 2012-06-08 08:16:36 UTC ***

http://hg.instantbird.org/instantbird/rev/ef344ab33302

Thanks for adding this!
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.