1012666 - Fix up checking if IRC topics are settable

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Patrick Cloke [:clokep]]

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

Comment 1

[Patrick Cloke [:clokep]]

Attachment: Patch v1 [checked in comment #5]

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.

Comment 2

[aleth [:aleth]]

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?

Comment 3

[Patrick Cloke [:clokep]]

(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

[aleth [:aleth]]

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 :)

Comment 5

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/a9c18429042c

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Fix bustage [checked in, comment #7]

Fix typo causing bustage.

Comment 7

[Patrick Cloke [:clokep]]

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.

Comment 8

[Patrick Cloke [:clokep]]

Attachment: Fix regression

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.

Comment 9

[aleth [:aleth]]

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 ...

Comment 10

[Patrick Cloke [:clokep]]

Attachment: Fix regression v2

Yes, that looks nicer! I added a comment too.

Comment 11

[Patrick Cloke [:clokep]]

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

># HG changeset patch
># User Nihanth Subramanya <[e-mail]>
># 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;
>   }
> };

Comment 12

[Patrick Cloke [:clokep]]

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

https://hg.mozilla.org/comm-central/rev/4d0cc29b6341