Closed Bug 954862 Opened 10 years ago Closed 10 years ago

Fix up order and formatting of of IRC tooltips.

Categories

(Chat Core :: IRC, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benediktp, Assigned: clokep)

Details

Attachments

(1 file, 2 obsolete files)

*** 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 ;)
*** 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.
Summary: Show "Connected to" and "Connected from" in IRC tooltips next to each other → Fix up order and formatting of of IRC tooltips.
Attached patch Patch v1 (obsolete) — Splinter Review
*** 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: nobody → clokep
Status: NEW → ASSIGNED
*** 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 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-
*** 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.
*** 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.
Attached patch Patch v2 (obsolete) — Splinter Review
*** 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)
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 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-
Attached patch Patch v3Splinter Review
*** 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)
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 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+
Whiteboard: [checkin-needed]
*** Original post on bio 1427 at 2012-06-08 08:16:02 UTC ***

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

Thanks for adding this!
*** 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
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.