Fix up checking if IRC topics are settable

RESOLVED FIXED in 1.6

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: clokep, Assigned: clokep)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
The logic around when we update the UI for if a topic is settable or not is...pretty stupid. We should fix it up.
(Assignee)

Comment 1

5 years ago
I'm apparently on a roll with cleaning up random IRC code. This cleans up when we update the UI for setting the topic. I think it's a pretty nice change, hopefully I didn't miss any corner cases. I tested:
 - Joining a channel with a mode of -t and setting the topic.
 - Setting the mode to +t and being unable to set the topic.
 - Setting the mode back to -t and being able to set the topic.
 - Setting the mode to +t, opping the user and being able to set the topic.
 - Deopping th euser and being unable to set the topic.
 - Joining a channel that is already +t and being unable to set the topic.
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attachment #8426674 - Flags: review?(aleth)

Comment 2

5 years ago
A nice cleanup :)

Would it be even nicer to have the topicSettable getter remember the previously returned value and fire off a notification if it changed? Or doesn't that help as the mode setter would have to call the getter for no good reason then?
(Assignee)

Comment 3

5 years ago
(In reply to aleth [:aleth] from comment #2)
> Would it be even nicer to have the topicSettable getter remember the
> previously returned value and fire off a notification if it changed? Or
> doesn't that help as the mode setter would have to call the getter for no
> good reason then?

I don't know how I feel about topicSettable itself sending that notification, it feels wrong to me, but I can't really give a good reason not to do it. topicSettable is part of the interface though, so it would allow outside objects to cause this to send a notification (which seems wrong to me). I don't think it would be any nicer, and would also require an extra property on the conversation, while this implementation only keeps a local variable.

If there was some way the topicSettable result could change BESDIES a mode change, I agree this would make more sense.

Comment 4

5 years ago
Comment on attachment 8426674 [details] [diff] [review]
Patch v1 [checked in comment #5]

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

I agree with all of that, I was just unsure :)
Attachment #8426674 - Flags: review?(aleth) → review+

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/comm-central/rev/a9c18429042c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Fix typo causing bustage.
Assignee: clokep → nhnt11
Status: RESOLVED → REOPENED
Attachment #8431087 - Flags: review?(clokep)
Resolution: FIXED → ---
(Assignee)

Updated

5 years ago
Attachment #8431087 - Flags: review?(clokep) → review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/comm-central/rev/16295350aca3

Thanks for spotting the fix. TB was busted so never saw this issue (and this was pushed on a CLOSED TREE). Next IB nightly should have this.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

5 years ago
Posted patch Fix regression (obsolete) — Splinter Review
This was just not my patch. . .

This fixes a regression which I saw when testing another patch. Joining a channel where I was the only user, it wouldn't show me as having op status. This seems to be because the participant for myself would be created BEFORE the nick came through with the prefixes.

Pretty much checking if we're in the room or not isn't good enough (since this is set to true when the JOIN comes back), we need to actually check if we exist as a participant in the channel yet.
Attachment #8431262 - Flags: review?(aleth)

Comment 9

5 years ago
Comment on attachment 8431262 [details] [diff] [review]
Fix regression

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

::: chat/protocols/irc/irc.js
@@ +544,5 @@
>        return false;
>  
>      // If the channel mode is +t, hops and ops can set the topic; otherwise
>      // everyone can.
>      let participant = this.getParticipant(this.nick);

How about moving the first check here like so:
let participant = this._participants.get(this.nick);
if (!participant)
  return false;
return ...
(Assignee)

Comment 10

5 years ago
Yes, that looks nicer! I added a comment too.
Attachment #8431262 - Attachment is obsolete: true
Attachment #8431262 - Flags: review?(aleth)
Attachment #8431556 - Flags: review?(aleth)

Updated

5 years ago
Attachment #8431556 - Flags: review?(aleth) → review+
(Assignee)

Updated

5 years ago
Attachment #8426674 - Attachment description: Patch v1 → Patch v1 [checked in comment #5]
(Assignee)

Comment 11

5 years ago
Comment on attachment 8431087 [details] [diff] [review]
Fix bustage [checked in, comment #7]

># HG changeset patch
># User Nihanth Subramanya <nhnt11@gmail.com>
># Date 1401394996 -19800
>#      Fri May 30 01:53:16 2014 +0530
># Node ID b1b17a79f6681a5d36030f6860b055855bdd2190
># Parent  114feac4036f4ae23000c49b208b8cb2ff1e7eaf
>Bug 1012666 - Fix up checking if IRC topics are settable: fix typo causing bustage. r=clokep
>
>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>--- a/chat/protocols/irc/irc.js
>+++ b/chat/protocols/irc/irc.js
>@@ -535,17 +535,17 @@ ircChannel.prototype = {
>   get topic() this._topic, // can't add a setter without redefining the getter
>   set topic(aTopic) {
>     // Note that the UI isn't updated here because the server will echo back the
>     // TOPIC to us and we'll set it on receive.
>     this._account.sendMessage("TOPIC", [this.name, aTopic]);
>   },
>   get topicSettable() {
>     // We must be in the room to set the topic.
>-    if (!this.left)
>+    if (this.left)
>       return false;
> 
>     // If the channel mode is +t, hops and ops can set the topic; otherwise
>     // everyone can.
>     let participant = this.getParticipant(this.nick);
>     return !this._modes.has("t") || participant.op || participant.halfOp;
>   }
> };
Attachment #8431087 - Attachment description: Fix bustage → Fix bustage [checked in, comment #7]
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Assignee: nhnt11 → nobody
(Assignee)

Updated

5 years ago
Assignee: nobody → clokep
(Assignee)

Comment 12

5 years ago
Sorry for the bug spam, wasn't expecting the tree to open again so soon.

https://hg.mozilla.org/comm-central/rev/4d0cc29b6341
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 1034971
You need to log in before you can comment on or make changes to this bug.