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)
Chat Core
IRC
Tracking
(Not tracked)
RESOLVED
FIXED
1.6
People
(Reporter: aleth, Assigned: aleth)
References
Details
(Whiteboard: [1.6-blocking])
Attachments
(3 files)
1.32 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
2.01 KB,
text/x-log
|
Details | |
1.59 KB,
patch
|
aleth
:
review+
standard8
:
approval-comm-aurora+
standard8
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
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.
Assignee | ||
Comment 2•10 years ago
|
||
(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.
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
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
Comment 5•10 years ago
|
||
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 | ||
Comment 6•10 years ago
|
||
Comment 7•10 years ago
|
||
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+
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/a08cb6f59154
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Comment 9•10 years ago
|
||
Does this need to get uplifted to aurora or beta?
Assignee | ||
Comment 10•10 years ago
|
||
(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?
Comment 11•10 years ago
|
||
(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
Comment 12•10 years ago
|
||
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 → ---
Comment 13•10 years ago
|
||
Here is a log of joining a channel where I couldn't click to set the topic (/topic worked).
Assignee | ||
Comment 14•10 years ago
|
||
I can't reproduce this. Does anyone have more detailed STR?
Comment 15•10 years ago
|
||
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?
Comment 16•10 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8454557 -
Attachment description: topicset.diff → topicset.diff [checked in comment 8]
Comment 17•9 years ago
|
||
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)
Assignee | ||
Comment 18•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 19•9 years ago
|
||
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 ago → 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Comment 20•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8531017 -
Flags: approval-comm-beta?
Attachment #8531017 -
Flags: approval-comm-beta+
Attachment #8531017 -
Flags: approval-comm-aurora?
Attachment #8531017 -
Flags: approval-comm-aurora+
Comment 21•9 years ago
|
||
Fallen checked these in for me: https://hg.mozilla.org/releases/comm-aurora/rev/0e47678128e6 https://hg.mozilla.org/releases/comm-beta/rev/14c4dcd54ef6
You need to log in
before you can comment on or make changes to this bug.
Description
•