Last Comment Bug 778276 - The topic editor for IRC channels does not work
: The topic editor for IRC channels does not work
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: 15 Branch
: x86 All
: -- normal (vote)
: Thunderbird 17.0
Assigned To: Florian Quèze [:florian] [:flo]
:
:
Mentors:
: 779276 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-27 13:33 PDT by Mike Conley (:mconley) - (needinfo me!)
Modified: 2012-08-07 10:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
Patch v1 (6.65 KB, patch)
2012-08-01 14:21 PDT, Mike Conley (:mconley) - (needinfo me!)
florian: review-
Details | Diff | Splinter Review
Patch v2 (4.35 KB, patch)
2012-08-03 07:26 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Splinter Review
Patch v3 (4.91 KB, patch)
2012-08-07 07:48 PDT, Florian Quèze [:florian] [:flo]
bwinton: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (needinfo me!) 2012-07-27 13:33:17 PDT
STR:

1) Join an IRC channel where you have topic-changing privileges
2) Click on the topic in the "conversation-info" panel to change it

What happens?

Nothing! Or, if there was no topic before, the "No topic" indicator goes away, but that's it.

What's expected?

We should be able to set the topic!
Comment 1 Patrick Cloke [:clokep] 2012-07-27 20:58:43 PDT
Mike, do we know if this is a UI level or a protocol level bug? (Are there errors in the error console that seem to be from sending a malformed message? Do you know what messages are being sent to the server?)

If we think it's a protocol level thing, knowing the server, channel (mode) and user permissions will be necessary to fully diagnose this issue.
Comment 2 Florian Quèze [:florian] [:flo] 2012-07-30 02:22:48 PDT
(In reply to Patrick Cloke [:clokep] from comment #1)
> Mike, do we know if this is a UI level or a protocol level bug?

It's a UI bug; the implementation has never been finished.

The situation is very similar to bug 735331, a CSS rule similar to 
http://lxr.instantbird.org/instantbird/source/instantbird/content/instantbird.css#18
 18 .statusMessage[editing] {
 19   -moz-appearance: textfield;
 20   -moz-binding: url('chrome://global/content/bindings/textbox.xml#textbox');
 21 }
is missing.
After adding it, someone will need to check that things actually work, and debug what doesn't.

The situation isn't exactly the same as for Instantbird because Thunderbird uses the same conv-top-info toolbar for all conversations and just recycles it instead of having a separate one for each conversation, so there may be details to fix.
Comment 3 Mike Conley (:mconley) - (needinfo me!) 2012-08-01 09:54:15 PDT
So adding some CSS to attach the textbox binding and appearance is no problem.

The problem lies within the conv-info-large binding in imconversation.xml - this binding attempts to get a property "_conv" from its binding parent (see http://mxr.mozilla.org/comm-central/source/mail/components/im/content/imconversation.xml#1344). Unfortunately, the binding parent (the toolbar with id="conv-top-info"), has no property _conv.

So what is the best way for the conv-info-large binding to get access to the current conversation?
Comment 4 Florian Quèze [:florian] [:flo] 2012-08-01 10:09:51 PDT
A simple solution would be to set cti._conv = this._conv in updateTopic http://mxr.mozilla.org/comm-central/source/mail/components/im/content/imconversation.xml#957
If we do that we will want to ensure that reference is removed if when the conversation is closed or whenever we update the conv-top-info toolbar to display something else, otherwise we would keep alive the underlying conversation object for a while.
Comment 5 Mike Conley (:mconley) - (needinfo me!) 2012-08-01 12:17:18 PDT
*** Bug 779276 has been marked as a duplicate of this bug. ***
Comment 6 Mike Conley (:mconley) - (needinfo me!) 2012-08-01 14:21:19 PDT
Created attachment 648080 [details] [diff] [review]
Patch v1

This is my first crack at it...I'm not too happy with how coupled conv-info-large is with the conversation binding and the stuff in chat-messenger-overlay.js...but I don't know how else to do this. Open to suggestions.

Are there any edge cases I'm missing?
Comment 7 Florian Quèze [:florian] [:flo] 2012-08-03 03:49:47 PDT
Comment on attachment 648080 [details] [diff] [review]
Patch v1

Review of attachment 648080 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/im/content/imconversation.xml
@@ +51,5 @@
>         var browser = this.browser;
>         browser.addEventListener("keypress", this.browserKeyPress);
>         browser.addEventListener("dblclick", this.browserDblClick.bind(this));
>         Services.obs.addObserver(this, "conversation-loaded", false);
> +       Services.obs.addObserver(this, "ui-conversation-closed", false);

I think you neither need nor want to observe this global notification.

The "observe" method will already be notified of conversation-specific notifications, including ui-conversation-closed.

@@ +1129,5 @@
>       <method name="initConversationUI">
>         <body>
>         <![CDATA[
> +         let cti = document.getElementById("conv-top-info");
> +

What is this change for?

@@ +1174,5 @@
>  
> +         if (aTopic == "ui-conversation-closed") {
> +           let cti = document.getElementById("conv-top-info");
> +           if (aSubject == cti._conv)
> +             cti._conv = null;

I would suggest moving this to the _forgetConv method, and test if (this._conv == cti._conv)

@@ +1400,5 @@
>  
>       <method name="startEditTopic">
>         <body>
>         <![CDATA[
> +          if (!this._conv || !this._conv.topicSettable)

Do you have a reason to expect this._conv to be null here?
The this._conv.topicSettable is duplicating the elt.hasAttribute("editable") test a few lines further down, isn't it?
Comment 8 Mike Conley (:mconley) - (needinfo me!) 2012-08-03 07:26:47 PDT
Created attachment 648698 [details] [diff] [review]
Patch v2

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> Comment on attachment 648080 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 648080 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/im/content/imconversation.xml
> @@ +51,5 @@
> >         var browser = this.browser;
> >         browser.addEventListener("keypress", this.browserKeyPress);
> >         browser.addEventListener("dblclick", this.browserDblClick.bind(this));
> >         Services.obs.addObserver(this, "conversation-loaded", false);
> > +       Services.obs.addObserver(this, "ui-conversation-closed", false);
> 
> I think you neither need nor want to observe this global notification.

Okie doke...the whole notification model is a little mystifying here, but I'll trust your expertise. :)

> 
> The "observe" method will already be notified of conversation-specific
> notifications, including ui-conversation-closed.
> 
> @@ +1129,5 @@
> >       <method name="initConversationUI">
> >         <body>
> >         <![CDATA[
> > +         let cti = document.getElementById("conv-top-info");
> > +
> 
> What is this change for?

Leftover cruft from an earlier iteration. Thanks for spotting it - removed.

> 
> @@ +1174,5 @@
> >  
> > +         if (aTopic == "ui-conversation-closed") {
> > +           let cti = document.getElementById("conv-top-info");
> > +           if (aSubject == cti._conv)
> > +             cti._conv = null;
> 
> I would suggest moving this to the _forgetConv method, and test if
> (this._conv == cti._conv)

Done.

> 
> @@ +1400,5 @@
> >  
> >       <method name="startEditTopic">
> >         <body>
> >         <![CDATA[
> > +          if (!this._conv || !this._conv.topicSettable)
> 
> Do you have a reason to expect this._conv to be null here?

Yes - in the event that we're looking at a conversation where topicSettable is false (for example, a GChat conversation with some contact), then conv is set to null in updateTopic.  Let me know if there's a better way to go about things there.

> The this._conv.topicSettable is duplicating the elt.hasAttribute("editable")
> test a few lines further down, isn't it?

Good call. Removed that condition.
Comment 9 Florian Quèze [:florian] [:flo] 2012-08-03 07:44:59 PDT
(In reply to Mike Conley (:mconley) from comment #8)

> > > +       Services.obs.addObserver(this, "ui-conversation-closed", false);
> > 
> > I think you neither need nor want to observe this global notification.
> 
> Okie doke...the whole notification model is a little mystifying here, but
> I'll trust your expertise. :)

Ok, let me explain then. We have global notifications, dispatched through Services.obs, and local notifications, dispatched directly by the objects firing the notifications. Most notifications are fired both locally and globally.

To register for local notifications, instead of calling Services.obs.addObserver with a topic, we call object.addObserver(observer) and we will receive notifications for all topics, but only from this object.

We typically use local notifications for conversations and contacts, so that each instance of contact or conversation XBL bindings can receive all updates for the contact/conversation it is displaying without us sending all notifications to all bindings.

For a (unfortunately not complete) list of notifications fired by the chat core, see https://wiki.instantbird.org/Instantbird:Notifications:trunk


> > 
> > @@ +1400,5 @@
> > >  
> > >       <method name="startEditTopic">
> > >         <body>
> > >         <![CDATA[
> > > +          if (!this._conv || !this._conv.topicSettable)
> > 
> > Do you have a reason to expect this._conv to be null here?
> 
> Yes - in the event that we're looking at a conversation where topicSettable
> is false.

That case is taken care of by the elt.hasAttribute("editable") test, isn't it?
Comment 10 Mike Conley (:mconley) - (needinfo me!) 2012-08-03 07:48:07 PDT
(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> (In reply to Mike Conley (:mconley) from comment #8)
> 
> > > > +       Services.obs.addObserver(this, "ui-conversation-closed", false);
> > > 
> > > I think you neither need nor want to observe this global notification.
> > 
> > Okie doke...the whole notification model is a little mystifying here, but
> > I'll trust your expertise. :)
> 
> Ok, let me explain then. We have global notifications, dispatched through
> Services.obs, and local notifications, dispatched directly by the objects
> firing the notifications. Most notifications are fired both locally and
> globally.
> 
> To register for local notifications, instead of calling
> Services.obs.addObserver with a topic, we call object.addObserver(observer)
> and we will receive notifications for all topics, but only from this object.
> 
> We typically use local notifications for conversations and contacts, so that
> each instance of contact or conversation XBL bindings can receive all
> updates for the contact/conversation it is displaying without us sending all
> notifications to all bindings.
> 
> For a (unfortunately not complete) list of notifications fired by the chat
> core, see https://wiki.instantbird.org/Instantbird:Notifications:trunk
> 

Ok, yes, that's a bit clearer. Thanks. :)

> 
> > > 
> > > @@ +1400,5 @@
> > > >  
> > > >       <method name="startEditTopic">
> > > >         <body>
> > > >         <![CDATA[
> > > > +          if (!this._conv || !this._conv.topicSettable)
> > > 
> > > Do you have a reason to expect this._conv to be null here?
> > 
> > Yes - in the event that we're looking at a conversation where topicSettable
> > is false.
> 
> That case is taken care of by the elt.hasAttribute("editable") test, isn't
> it?

I don't think so. Removing the if (!this._conv) condition results in me being able to edit status messages in GChat conversations.
Comment 11 Florian Quèze [:florian] [:flo] 2012-08-03 08:13:09 PDT
(In reply to Mike Conley (:mconley) from comment #10)

> > > > >       <method name="startEditTopic">
> > > > >         <body>
> > > > >         <![CDATA[
> > > > > +          if (!this._conv || !this._conv.topicSettable)
> > > > 
> > > > Do you have a reason to expect this._conv to be null here?
> > > 
> > > Yes - in the event that we're looking at a conversation where topicSettable
> > > is false.
> > 
> > That case is taken care of by the elt.hasAttribute("editable") test, isn't
> > it?
> 
> I don't think so. Removing the if (!this._conv) condition results in me
> being able to edit status messages in GChat conversations.

Does that mean we don't correctly remove the topicEditable attribute when switching from a conversation to another? If so, we should probably fix that too.
Comment 12 Florian Quèze [:florian] [:flo] 2012-08-07 07:48:25 PDT
Created attachment 649639 [details] [diff] [review]
Patch v3

It turns out my suggestion in comment 4 or setting cti._conv to this._conv to access the conversation object from the topic toolbar wasn't a great idea.
When I tried to focus the input box of the conversation after the user has finished editing the topic, I couldn't and had to find a way to access the conversation binding (document.getElementById("conversationsDeck").selectedPanel). Then accessing the conversation object is just convBinding._conv, so I removed all the cti._conv parts of the previous patch. Sorry about that Mike :-/.

I also caught a few edge cases where we didn't properly remove some attributes from the conv-top-info toolbar, and I added the CSS rule to change the cursor above the topic when it's editable (it's the only way to discover this feature!).
Comment 13 Blake Winton (:bwinton) (:☕️) 2012-08-07 08:40:15 PDT
Comment on attachment 649639 [details] [diff] [review]
Patch v3

>+++ b/mail/components/im/content/chat.css
>@@ -65,21 +65,21 @@ browser[type="content-conversation"] {
>-#statusMessage[editing] {
>+#statusMessage[editing], .statusMessage[editing] {

Do we still need the #statusMessage if we're also triggering on .statusMessage?

r=me with that fixed or explained.

Thanks,
Blake.
Comment 14 Florian Quèze [:florian] [:flo] 2012-08-07 08:48:40 PDT
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #13)
> Comment on attachment 649639 [details] [diff] [review]
> Patch v3
> 
> >+++ b/mail/components/im/content/chat.css
> >@@ -65,21 +65,21 @@ browser[type="content-conversation"] {
> >-#statusMessage[editing] {
> >+#statusMessage[editing], .statusMessage[editing] {
> 
> Do we still need the #statusMessage if we're also triggering on
> .statusMessage?

#statusMessage is the status message of the user, in the status selector of the chat toolbar.
Comment 15 Florian Quèze [:florian] [:flo] 2012-08-07 10:18:28 PDT
https://hg.mozilla.org/comm-central/rev/42b6de505bcd

Note You need to log in before you can comment on or make changes to this bug.