Closed Bug 954725 Opened 10 years ago Closed 10 years ago

Tooltip improvements for IRC

Categories

(Chat Core :: IRC, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

Details

(Whiteboard: [1.2-wanted])

Attachments

(2 files)

*** Original post on bio 1293 at 2012-02-27 16:22:00 UTC ***

> flo: I would like to see the "is connected with SSL" and "is identified" information as they are important for security (it's the only way to know you are actually talking to the person owning the nick)
> flo: and I think we can simplify the tooltip significantly to make them readable by someone who isn't a computer programmer 
> flo: example of simplifications: drop the "Server" line
> flo: merge "Real Name" and "Username" into a single line labeled "Name" containing both: <name1> (<name2>)
> flo: (not sure which one should be displayed first, as the way they are used doesn't seem consistent across clients; but the difference between them is mostly irrelevant to users)
> flo: ""Connected from" seems more understandable than "Host name"" do you think that would help?

I like:
Name: <real name> (<username>)

And I agree "Connected from" is better than "Host name" (or maybe "Connected to").
Whiteboard: [1.2-wanted]
*** Original post on bio 1293 at 2012-02-29 21:34:46 UTC ***

Imho <username> (which is really confusingly titled as it is generally not the user's name, and often set by the client and not the user) should be part of the "connected from/with" info as opposed to part of the Name.

"Account" seems unnecessary (nothing to do with the nick)
*** Original post on bio 1293 at 2012-03-11 22:56:17 UTC ***

New proposal:

Name: <realname>
Connected from: <username>@<hostname>
Connected to: <server>

Thoughts? If we agree on this, I'd like to implement this ASAP as the review comments in bmo 714733 dislikes the current "Host name", "Real name", "Username" set of strings.
*** Original post on bio 1293 at 2012-03-11 23:48:42 UTC ***

Sounds good to me. Especially "username" was really misleading.

This is not going to be a perfect solution, because some clients do strange things. I have seen real names set (presumably by the clients) to "http://www.mibbit.com", "None", "country 8 *", and various other URLs are common. But that's just clients behaving badly.
*** Original post on bio 1293 at 2012-03-12 00:16:43 UTC ***

(In reply to comment #2)
> New proposal:
> 
> Name: <realname>
> Connected from: <username>@<hostname>
> Connected to: <server>
> 
> Thoughts? If we agree on this, I'd like to implement this ASAP as the review
> comments in bmo 714733 dislikes the current "Host name", "Real name",
> "Username" set of strings.

I think this proposal looks good!
*** Original post on bio 1293 as attmnt 1247 at 2012-03-12 02:21:00 UTC ***

This patch will fix the names I think.

Sorry if this patch is a bit of a mess, I'm doing it hastily before sleep! (I also hand edited it at one point...but I think I fixed the diff #s properly...)
Attachment #8353000 - Flags: review?(florian)
*** Original post on bio 1293 at 2012-03-12 10:04:28 UTC ***

>+    // If the buddy isn't in the list yet, add it.
>+    if (!hasOwnProperty(aAccount.whoisInformation, buddyName))
>+      aAccount.whoisInformation[buddyName] = {};

Why did you move this to requestBuddyInfo? I don't think that's a good idea, first because requestBuddyInfo is not the only route new whois entries can be added (it may be via a command), second because at that point you don't necessarily know yet that buddyName is a valid online nick, unless you assume something about who is calling requestBuddyInfo.
*** Original post on bio 1293 at 2012-03-12 10:37:53 UTC ***

(In reply to comment #6)
> >+    // If the buddy isn't in the list yet, add it.
> >+    if (!hasOwnProperty(aAccount.whoisInformation, buddyName))
> >+      aAccount.whoisInformation[buddyName] = {};
> 
> Why did you move this to requestBuddyInfo?
I moved this because it only needs to be done once as an initialization. I think it's the right change to make; but I don't really care that much.

> I don't think that's a good idea,
> first because requestBuddyInfo is not the only route new whois entries can be
> added (it may be via a command),
All code paths requesting WHOIS should go through this method: that way we're only building and sending the command in one place (even though this is a very simple command...).

> second because at that point you don't
> necessarily know yet that buddyName is a valid online nick, unless you assume
> something about who is calling requestBuddyInfo.
If it's not a valid, online nick then you end up with an empty entry in the object. I don't know how well defined our behavior is in this case anyway, we could always delete the entry when we get the error response.
*** Original post on bio 1293 at 2012-03-12 11:08:30 UTC ***

(In reply to comment #7)

> > I don't think that's a good idea,
> > first because requestBuddyInfo is not the only route new whois entries can be
> > added (it may be via a command),
> All code paths requesting WHOIS should go through this method: that way we're
> only building and sending the command in one place (even though this is a very
> simple command...).

There is also WHOWAS that modifies the same database. So it won't be the only route.

It seems better to me to put the code that actually modifies the underlying table together in one method (setWhoIs) and so create the entry when you are actually writing to it.
 
> > second because at that point you don't
> > necessarily know yet that buddyName is a valid online nick, unless you assume
> > something about who is calling requestBuddyInfo.
> If it's not a valid, online nick then you end up with an empty entry in the
> object. I don't know how well defined our behavior is in this case anyway, we
> could always delete the entry when we get the error response.

It seems a more roundabout way to go about things, but if you prefer it, sure...
*** Original post on bio 1293 at 2012-03-12 11:18:48 UTC ***

(In reply to comment #7)
> (In reply to comment #6)
> > >+    // If the buddy isn't in the list yet, add it.
> > >+    if (!hasOwnProperty(aAccount.whoisInformation, buddyName))
> > >+      aAccount.whoisInformation[buddyName] = {};
> > 
> > Why did you move this to requestBuddyInfo?
> I moved this because it only needs to be done once as an initialization. I
> think it's the right change to make; but I don't really care that much.

It feels safer to me to ensure aAccount.whoisInformation[buddyName] is a property of the object (and not the prototype) before attempting to write to it, plus we can't trust the server to always send us something that matches what we have requested; especially when not using SSL ;). So... I would prefer not including this specific change.
Comment on attachment 8353000 [details] [diff] [review]
Patch to fix names

*** Original change on bio 1293 attmnt 1247 at 2012-03-12 11:19:39 UTC ***

The other changes in this patch look good (although I'll commit the additional XXX comment changes in a separate changeset). Thanks for fixing this! :)
Attachment #8353000 - Flags: review?(florian) → review+
*** Original post on bio 1293 at 2012-03-13 00:10:06 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/bdffb00e8b22
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
*** Original post on bio 1293 at 2012-03-13 00:14:32 UTC ***

We still need to:
 - Handle the SSL message
 - Handle whether the nick is identified
 - Add a specific order to the tooltip
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch PatchSplinter Review
*** Original post on bio 1293 as attmnt 1248 at 2012-03-13 10:35:00 UTC ***

This simple change gives the correct order (at least as long as the server doesn't reply to whois with messages in an unusual order - don't know how standardized that is).
Attachment #8353001 - Flags: review?(clokep)
*** Original post on bio 1293 at 2012-03-13 10:38:16 UTC ***

(In reply to comment #13)
> Created attachment 8353001 [details] [diff] [review] (bio-attmnt 1248) [details]
> Patch
> 
> This simple change gives the correct order (at least as long as the server
> doesn't reply to whois with messages in an unusual order - don't know how
> standardized that is).

I'm not sure if any other orders were to be flipped, for reference (by the way), this was solved for Twitter using: http://lxr.instantbird.org/instantbird/source/chat/protocols/twitter/twitter.js#961
Comment on attachment 8353001 [details] [diff] [review]
Patch

*** Original change on bio 1293 attmnt 1248 at 2012-03-15 23:17:48 UTC ***

This is a definite improvement from what we have...but we probably need to hardcode some order (like Twitter) as I don't know if the ordering we'll iterate over is always the same (or desired) and I don't know if it's affected by the order we make the keys in (i.e. the order the server sends us stuff in). :)

I agree though that this trivial patch is an improvement!
Attachment #8353001 - Flags: review?(clokep) → review+
*** Original post on bio 1293 at 2012-03-15 23:20:20 UTC ***

Thanks for the r+ (This trivial improvement is useful also because the patch adds the whowas equivalent missing from the original patch).
*** Original post on bio 1293 at 2012-03-20 14:10:11 UTC ***

Attachment 8353001 [details] [diff] (bio-attmnt 1248) needs to be checked-in.
Whiteboard: [1.2-wanted] → [1.2-wanted][checkin-needed]
*** Original post on bio 1293 at 2012-03-20 22:39:47 UTC ***

(In reply to comment #17)
> Attachment 8353001 [details] [diff] (bio-attmnt 1248) [details] needs to be checked-in.

Checked in as http://hg.instantbird.org/instantbird/rev/a52a4269c6a1

Thanks aleth!

Let's open any new bugs for further improvements to this.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [1.2-wanted][checkin-needed] → [1.2-wanted]
You need to log in before you can comment on or make changes to this bug.