954862 - Fix up order and formatting of of IRC tooltips.

Status: RESOLVED FIXED

(Chat Core :: IRC, enhancement)

Description

[Benedikt Pfeifer]

*** 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 ;)

Comment 1

[Patrick Cloke [:clokep]]

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

Comment 2

[Patrick Cloke [:clokep]]

Attachment: 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

Comment 3

[Patrick Cloke [:clokep]]

*** 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

[aleth [:aleth]]

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?

Comment 5

[Patrick Cloke [:clokep]]

*** 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

[aleth [:aleth]]

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

Comment 7

[Patrick Cloke [:clokep]]

Attachment: 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.

Comment 8

[Patrick Cloke [:clokep]]

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 ***

Comment 9

[aleth [:aleth]]

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.

Comment 10

[Patrick Cloke [:clokep]]

Attachment: 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.

Comment 11

[Patrick Cloke [:clokep]]

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 ***

Comment 12

[aleth [:aleth]]

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 ***

Comment 13

[aleth [:aleth]]

*** 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

[aleth [:aleth]]

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

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

Thanks for adding this!