Closed Bug 1034971 Opened 10 years ago Closed 9 years ago

Regression: IRC topic not always settable on freshly-joined channels

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

(Whiteboard: [1.6-blocking])

Attachments

(3 files)

STR
Add IRC channel to autojoin list and connect that account. Try to set the topic for that account in the tab that pops up.
Putting the channel on hold and reopening or using /topic fixes the problem.
There's more to the STR than this. I know for a fact that being able to set topics works properly for some of the channels I join.
(In reply to Patrick Cloke [:clokep] from comment #1)
> There's more to the STR than this. I know for a fact that being able to set
> topics works properly for some of the channels I join.

Suspicion after a quick glance at the code: http://mxr.mozilla.org/comm-central/source/chat/protocols/irc/irc.js#511 doesn't always send the notification when it should when joining a channel, if previousTopicSettable (which "should" be undefined but isn't) happens to have the same value as the actual value.
You still haven't given an example of when this happens. By default, does the UI code allow the topic to be settable or not?

I can't reproduce this.
This happens whenever the participant isn't in the room when the mode arrives (cf http://mxr.mozilla.org/comm-central/source/chat/protocols/irc/irc.js#541).

This workaround (inspired by the code from before your patch landed) fixes the problem, but hopefully there is a better way.

--- a/chat/protocols/irc/ircBase.jsm
+++ b/chat/protocols/irc/ircBase.jsm
@@ -923,16 +923,18 @@ var ircBase = {
     "366": function(aMessage) { // RPL_ENDOFNAMES
       // <target> <channel> :End of NAMES list
       // All participants have already been added by the 353 handler.
 
       // This assumes that this is the last message received when joining a
       // channel, so a few "clean up" tasks are done here.
       let conversation = this.getConversation(aMessage.params[1]);
 
+      conversation.notifyObservers(this, "chat-update-topic");
+
       // If we haven't received the MODE yet, request it.
       if (!conversation._receivedInitialMode)
         this.sendMessage("MODE", aMessage.params[1]);
 
       return true;
     },
     /*
      * End of a bunch of lists
I think that's going to be our best bet here. Can you do a patch with a comment above it saying that we forceibly update the topic at this point for blah blah?
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Attachment #8454557 - Flags: review?(clokep)
Comment on attachment 8454557 [details] [diff] [review]
topicset.diff [checked in comment 8]

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

Thanks! Now that we have a comment here I'll be less likely to nuke this code again. :(
Attachment #8454557 - Flags: review?(clokep) → review+
https://hg.mozilla.org/comm-central/rev/a08cb6f59154
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Does this need to get uplifted to aurora or beta?
(In reply to Patrick Cloke [:clokep] from comment #9)
> Does this need to get uplifted to aurora or beta?

If bug 1012666 is in TB31, yes. I don't think it is though?
(In reply to aleth [:aleth] from comment #10)
> (In reply to Patrick Cloke [:clokep] from comment #9)
> > Does this need to get uplifted to aurora or beta?
> 
> If bug 1012666 is in TB31, yes. I don't think it is though?

It's in c-a, not c-b: https://hg.mozilla.org/releases/comm-aurora/log?rev=Bug+1012666
Still not fixed. I noticed today that I can click to change the topic on channels with plenty of participants, but not on channels with a small list of participants.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file Debug log
Here is a log of joining a channel where I couldn't click to set the topic (/topic worked).
I can't reproduce this. Does anyone have more detailed STR?
I still see this happening, but there's no STR, it just seems to be when I sign in. We can play w/ adding/removing you as an op at some point and see if that "fixes" it or triggers it, maybe it'll give us some idea?
Note: putting a channel on hold and reopening it 'fixes' it.

I wonder if this bug could be happening only when channels have been automatically rejoined. I'll check the next time I have an update-restart if I can reproduce the bug.
Attachment #8454557 - Attachment description: topicset.diff → topicset.diff [checked in comment 8]
Attached patch Fix v1Splinter Review
Well this bug was a pain...but I think I figured it out. If a new mode is set that *only* changes user modes than we were returning earlier. But a user mode changing can change topicSettable, so we need to ensure this code is called before we return.

Sorry this has taken me so long to get back to.
Attachment #8531017 - Flags: review?(aleth)
Comment on attachment 8531017 [details] [diff] [review]
Fix v1

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

Ah, that makes sense! Hopefully the bug will finally succumb now.
Attachment #8531017 - Flags: review?(aleth) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/cc7951cb37a2

We need to land this on m-a, at least, I think.
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Comment on attachment 8531017 [details] [diff] [review]
Fix v1

[Approval Request Comment]

This approval is only for attachment 8531017 [details] [diff] [review], attachment 8454557 [details] [diff] [review] was committed for Tb 33.

Regression caused by (bug #): bug 1012666 (Tb 32)
User impact if declined: Sometimes the UI will not allow users to set the IRC topic when they are actually allowed to.
Testing completed (on c-c, etc.): Tested locally and currently on c-c.
Risk to taking this patch (and alternatives if risky): I don't see this patch as risky, there is not really an alternative I can think of. We'll let it bake on trunk for a few days first though!
Attachment #8531017 - Flags: approval-comm-beta?
Attachment #8531017 - Flags: approval-comm-aurora?
Attachment #8531017 - Flags: approval-comm-beta?
Attachment #8531017 - Flags: approval-comm-beta+
Attachment #8531017 - Flags: approval-comm-aurora?
Attachment #8531017 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.