Closed Bug 953761 Opened 6 years ago Closed 6 years ago

Check if topic on IRC channels is editable and make UI respond accordingly

Categories

(Chat Core :: IRC, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Mic, Assigned: clokep)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 9 obsolete files)

*** Original post on bio 318 at 2010-01-22 13:12:00 UTC ***

On IRC, check if a user has sufficient rights to set the topic and make the UI respond accordingly. That is for exmaple by showing the edit box as disabled and a message about insufficient rights.

Pointer: if the topic is changeable by operators only, the mode +t will be set on a channel.
Depends on: 953944
Blocks: 954011
Blocks: 954063
*** Original post on bio 318 at 2012-02-21 23:03:35 UTC ***

Dupe.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 954133
*** Original post on bio 318 at 2012-02-21 23:07:34 UTC ***



*** This bug has been marked as a duplicate of bug 954232 (bio 798) ***
*** Original post on bio 318 at 2012-02-27 15:37:49 UTC ***

Moving IRC bugs to new IRC component.
Component: Conversation → IRC
Product: Instantbird → Chat Core
*** Original post on bio 318 at 2012-02-29 21:55:39 UTC ***

This is not a duplicate.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Duplicate of this bug: 954734
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 318 as attmnt 1341 at 2012-04-13 01:48:00 UTC ***

This tracks the mode for each channel and uses that information to decide if the topic is settable or not. I'm more of looking for feedback on this one (rather than a full review).
Attachment #8353094 - Flags: review?(bugzilla)
Assignee: nobody → clokep
Status: REOPENED → ASSIGNED
*** Original post on bio 318 at 2012-04-13 16:25:35 UTC ***

This looks like it should work to me, but I haven't tested it. 

I'm not a fan of the if...else...else structure of the MODE handler - I think it would be better just to use if's and put a return true within each subhandler.

Shouldn't there be a chat-update-topic sent at the end of the RPL_NAMREPLY handler?

There is an i++ somewhere too ;)
Comment on attachment 8353094 [details] [diff] [review]
Patch

*** Original change on bio 318 attmnt 1341 at 2012-04-13 16:26:57 UTC ***

Just clearing the flag as I have looked at it.
Attachment #8353094 - Flags: review?(bugzilla) → review-
*** Original post on bio 318 at 2012-04-13 17:27:46 UTC ***

(In reply to comment #7)
> This looks like it should work to me, but I haven't tested it. 
> 
> I'm not a fan of the if...else...else structure of the MODE handler - I think
> it would be better just to use if's and put a return true within each
> subhandler.
Yeah, I guess there isn't any real shared code afterward.

> Shouldn't there be a chat-update-topic sent at the end of the RPL_NAMREPLY
> handler?
No. This can't affect the topic.

> There is an i++ somewhere too ;)
I'll fix that.

Thanks!
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 318 as attmnt 1351 at 2012-04-13 21:17:00 UTC ***

Taking into account aleth's comments.
Attachment #8353104 - Flags: review?(bugzilla)
Comment on attachment 8353094 [details] [diff] [review]
Patch

*** Original change on bio 318 attmnt 1341 at 2012-04-13 21:17:47 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353094 - Attachment is obsolete: true
*** Original post on bio 318 at 2012-04-13 22:17:08 UTC ***

I still think setRestriction should be called setModesFromRestriction (or similar), both to show it has something to do with modes, and because we are not keeping track of the "restriction" separately in any way.
Comment on attachment 8353104 [details] [diff] [review]
Patch v2

*** Original change on bio 318 attmnt 1351 at 2012-04-14 12:48:06 UTC ***

* The topicSettable implementation works great!

* So, I autojoin a channel in which I am the only member, and 

irc: :adev MODE adev :+ixz
irc: Unhandled IRC message: :adev MODE adev :+ixz      

(This is marked TODO in the code.)

Then we get the cryptic system message

mode (+n #testroom) by (null).

This is in response to the MODE sent out by JS-IRC.

* "/mode" on its own does not produce any response, not even a help message (for which it should return false)

* "/mode nick" (which the user might expect to print the current modes of nick) returns nothing. (Should there be a help/error message?)

* Generally it is far too easy to send a /mode command which results in no feedback whatsoever. (Which makes the user wonder: did it work? did I do something wrong?)

* The help message for /mode should probably include a list of the mode letters together with an explanation of their meaning, as these are impenetrable to the uninitiated.

* Similarly the system message produced by the MODE handler should have a plaintext part, explaining the returned mode.

* Should there be a checking of the arguments the user gives to the /mode command to make sure they represent a valid mode? And handle
irc: :concrete.mozilla.org 501 adev :Unknown MODE flag
irc: Unhandled IRC message: :concrete.mozilla.org 501 adev :Unknown MODE flag

* We should handle 482: You're not a channel operator (e.g. in response to trying to set the topic when lacking the permissions to do so) or stop the possibility from occuring by checking the permission before sending the request.

* and 499:You're not a channel owner

* I also managed to produce the following somehow
irc: Unhandled IRC message: :adev MODE adev :+sdp
irc: :concrete.mozilla.org 008 adev :Server notice mask (+ks)
irc: Unhandled IRC message: :concrete.mozilla.org 008 adev :Server notice mask (+ks)
Attachment #8353104 - Flags: review?(bugzilla) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
*** Original post on bio 318 as attmnt 1424 at 2012-05-02 01:25:00 UTC ***

I think this meets all the review comments you made, but I'm not positive as some of those were fixed by other bugs (to my knowledge).
Attachment #8353176 - Flags: review?(bugzilla)
Comment on attachment 8353104 [details] [diff] [review]
Patch v2

*** Original change on bio 318 attmnt 1351 at 2012-05-02 01:25:24 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353104 - Attachment is obsolete: true
Comment on attachment 8353176 [details] [diff] [review]
Patch v3

*** Original change on bio 318 attmnt 1424 at 2012-05-03 09:20:47 UTC ***

Testing this with the latest nightly. I will include some problems I encountered with /mode and the mode responses here first, but you may want to spin those off into another bug. I couldn't find an open bug for them.

* /mode on its own still produces no response, since 221 (RPL_UMODEIS) is not handled.

* 329 is not handled (I am not sure what it is, but it's also a mode response)

* 499:You're not a channel owner is not handled.

Problems with this patch:

* The 482 handler should return true, right? 

* The 482 response is not printed to the channel (a problem with conversationErrorMessage in channels?). 

* /mode #channelname doesn't actually tell the user what the channel mode is. This is odd as it seems from 324/setMode it should? It leads me to wonder whether setMode is even called.

* Did you test this? It no longer works as advertised (e.g. the #ubuntu topic is editable).

I also encountered the following error messages which I have not seen before:
Timestamp: 05/03/2012 11:17:05 AM
Error: item is undefined
Source File: chrome://instantbird/content/conversation.xml
Line: 1142
Timestamp: 05/03/2012 11:17:05 AM
Error: Error running command MODE with handler RFC 2812:
{"rawMessage":":FloodBot1!~floodbot@ubuntu/bot/floodbot MODE #ubuntu +zq esak!*@*","command":"MODE","params":["#ubuntu","+zq","esak!*@*"],"nickname":"FloodBot1","user":"~floodbot","host":"ubuntu/bot/floodbot","source":"~floodbot@ubuntu/bot/floodbot"}
Source File: resource:///modules/ircHandlers.jsm
Line: 117
Source Code:
irc
Timestamp: 05/03/2012 11:17:05 AM
Error: [Exception... "'[JavaScript Error: "item is undefined" {file: "chrome://instantbird/content/conversation.xml" line: 1142}]' when calling method: [nsIObserver::observe]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: resource:///modules/jsProtoHelper.jsm :: <TOP_LEVEL> :: line 443"  data: yes]
Source File: resource:///modules/ircHandlers.jsm
Line: 118
Attachment #8353176 - Flags: review?(bugzilla) → review-
Blocks: 954854
*** Original post on bio 318 at 2012-05-03 13:38:41 UTC ***

OK, lets try this again with an uncorrupted nightly ;) Sorry for the confusion.

This works well and is a really nice patch. 

Remaining issues:

The 482 handler should return true, and insert the channel name into "You are not a channel operator on $S." 

As you already mentioned, setMode for a participant could possibly take over the task of sending the observer notifications (just as the channel one updates the topic), for "chat-buddy-update" and maybe even (if the participant in question is the user himself) "chat-update-topic". This would remove notifyObservers completely from the MODE handler.

Sorry for the confusion!
Attached patch Patch v4 (obsolete) — Splinter Review
*** Original post on bio 318 as attmnt 1432 at 2012-05-03 23:53:00 UTC ***

Thanks for the review, don't worry about the confusion. :)
Attachment #8353184 - Flags: review?(bugzilla)
Comment on attachment 8353176 [details] [diff] [review]
Patch v3

*** Original change on bio 318 attmnt 1424 at 2012-05-03 23:53:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353176 - Attachment is obsolete: true
Attached patch Patch v5 (obsolete) — Splinter Review
*** Original post on bio 318 as attmnt 1433 at 2012-05-04 00:25:00 UTC ***

This moves some more code into the setMode function for a participant. It also adds a reference to the conversation for each participant this apparently this didn't exist.

This also fixes the usage of "/mode <new mode> <nick>" which is supposed to set the mode for a nick in a channel...that command is way overloaded, but oh well.
Attachment #8353185 - Flags: review?(bugzilla)
Comment on attachment 8353184 [details] [diff] [review]
Patch v4

*** Original change on bio 318 attmnt 1432 at 2012-05-04 00:25:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353184 - Attachment is obsolete: true
Attachment #8353184 - Flags: review?(bugzilla)
Comment on attachment 8353185 [details] [diff] [review]
Patch v5

*** Original change on bio 318 attmnt 1433 at 2012-05-04 09:36:14 UTC ***

This has turned into a really nice patch! :)

It works really well and would be r+ apart from the following regression:
/join a new channel (in which you will be the owner), you'll see the user doesn't get the +o star displayed. Strangely enough for autojoined channels in which you are the only participant, this issue does not occur. I can see no difference in the IRC messages being exchanged, so this might be a race condition where the UI notification is not arriving correctly.

irc: :adev JOIN :#tckk       <--- autojoin
irc: :gravel.mozilla.org MODE #tckk +n
irc: :gravel.mozilla.org 353 adev = #tckk :@adev
irc: :gravel.mozilla.org 366 adev #tckk :End of /NAMES list.
irc: :gravel.mozilla.org 303 adev :aleth Even
irc: Sending:                <-- manual join
JOIN :#tczz
irc: onTransportStatus(STATUS_SENDING_TO)
irc: onTransportStatus(STATUS_RECEIVING_FROM)
irc: :adev JOIN :#tczz
irc: :gravel.mozilla.org MODE #tczz +n
irc: :gravel.mozilla.org 353 adev = #tczz :@adev
irc: :gravel.mozilla.org 366 adev #tczz :End of /NAMES list.
Attachment #8353185 - Flags: review?(bugzilla) → review-
Attached patch Patch v6 (obsolete) — Splinter Review
*** Original post on bio 318 as attmnt 1443 at 2012-05-04 20:19:00 UTC ***

Fixes the issue with manually joining a room and not having op set.
Attachment #8353195 - Flags: review?(bugzilla)
Comment on attachment 8353185 [details] [diff] [review]
Patch v5

*** Original change on bio 318 attmnt 1433 at 2012-05-04 20:19:53 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353185 - Attachment is obsolete: true
Comment on attachment 8353195 [details] [diff] [review]
Patch v6

*** Original change on bio 318 attmnt 1443 at 2012-05-04 20:34:14 UTC ***

Done :)

Thanks for fixing this, and with a nice patch!
Attachment #8353195 - Flags: review?(bugzilla) → review+
Whiteboard: [checkin-needed]
Attached patch Patch v7 (obsolete) — Splinter Review
*** Original post on bio 318 as attmnt 1454 at 2012-05-08 10:38:00 UTC ***

Now only fires notifications when things change.
Attachment #8353207 - Flags: review?(florian)
Comment on attachment 8353195 [details] [diff] [review]
Patch v6

*** Original change on bio 318 attmnt 1443 at 2012-05-08 10:38:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353195 - Attachment is obsolete: true
*** Original post on bio 318 at 2012-05-16 15:43:25 UTC ***

Comment on attachment 8353207 [details] [diff] [review] (bio-attmnt 1454)
Patch v7


>+  _previousTopicSettable: null,
>+  checkTopicSettable: function() {
>+    if (this.topicSettable == this._previousTopicSettable &&
>+        this._preivousTopicSettable != null)

preivous?

I haven't really reviewed this new patch, but I have a feeling it hasn't been thoroughly tested ;).
Whiteboard: [checkin-needed]
Attached patch Patch v8 (obsolete) — Splinter Review
*** Original post on bio 318 as attmnt 1482 at 2012-05-16 23:42:00 UTC ***

Fixes the typo.

I tested this patch again for a few minutes and it seems to work as expected...
Attachment #8353235 - Flags: review?(florian)
Comment on attachment 8353207 [details] [diff] [review]
Patch v7

*** Original change on bio 318 attmnt 1454 at 2012-05-16 23:42:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353207 - Attachment is obsolete: true
Attachment #8353207 - Flags: review?(florian)
Attached patch Patch v8.1Splinter Review
*** Original post on bio 318 as attmnt 1512 at 2012-05-22 22:51:00 UTC ***

Fixes typo (for real) and removes dead comment.
Attachment #8353264 - Flags: review?(florian)
Comment on attachment 8353235 [details] [diff] [review]
Patch v8

*** Original change on bio 318 attmnt 1482 at 2012-05-22 22:51:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353235 - Attachment is obsolete: true
Attachment #8353235 - Flags: review?(florian)
Comment on attachment 8353264 [details] [diff] [review]
Patch v8.1

*** Original change on bio 318 attmnt 1512 at 2012-05-22 23:04:01 UTC ***

I haven't carefully reviewed the whole patch, but I believe aleth has. As I don't see any obvious issue any more, I think it's time to try this on nightlies :). Thanks for fixing this old bug! :)
Attachment #8353264 - Flags: review?(florian) → review+
*** Original post on bio 318 at 2012-05-22 23:07:58 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/4e2cb72c9a2f

Thanks for the reviews!
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
Attached patch Followup Patch v1 (obsolete) — Splinter Review
*** Original post on bio 318 as attmnt 1514 at 2012-05-23 02:14:00 UTC ***

Looks like I made a typo at one point when moving things around and we started requesting the MODE for the restriction parameter instead of the channel name. This patch fixes this.
Attachment #8353266 - Flags: review?(bugzilla)
Comment on attachment 8353266 [details] [diff] [review]
Followup Patch v1

*** Original change on bio 318 attmnt 1514 at 2012-05-23 09:02:46 UTC ***

Right, good catch.

>      // This assumes that this is the last message received when joining
>      // channel, so a few "clean up" tasks are done here.
>      // Update whether the topic is editable.
>      conversation.checkTopicSettable();
>
>      // If we haven't received the MODE yet, request it.
>      if (!conversation._receivedInitialMode)
>        this.sendMessage("MODE", aMessage.params[1]);

It strikes me that this is not actually the last message received on joining - that would be RPL_ENDOFNAMES. Is it worth moving these from 353 to 366?
Attachment #8353266 - Flags: review?(bugzilla) → review+
*** Original post on bio 318 as attmnt 1515 at 2012-05-23 10:31:00 UTC ***

(In reply to comment #28)
> Comment on attachment 8353266 [details] [diff] [review] (bio-attmnt 1514) [details]
> Followup Patch v1
> 
> Right, good catch.
> 
> >      // This assumes that this is the last message received when joining
> >      // channel, so a few "clean up" tasks are done here.
> >      // Update whether the topic is editable.
> >      conversation.checkTopicSettable();
> >
> >      // If we haven't received the MODE yet, request it.
> >      if (!conversation._receivedInitialMode)
> >        this.sendMessage("MODE", aMessage.params[1]);
> 
> It strikes me that this is not actually the last message received on joining -
> that would be RPL_ENDOFNAMES. Is it worth moving these from 353 to 366?

Yes, you're right. This would make more sense.
Attachment #8353267 - Flags: review?(bugzilla)
Comment on attachment 8353266 [details] [diff] [review]
Followup Patch v1

*** Original change on bio 318 attmnt 1514 at 2012-05-23 10:31:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353266 - Attachment is obsolete: true
Comment on attachment 8353267 [details] [diff] [review]
Followup Patch v2

*** Original change on bio 318 attmnt 1515 at 2012-05-23 10:37:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353267 - Flags: review?(bugzilla) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 318 at 2012-05-23 15:10:40 UTC ***

(Reopening so this goes into the checkin-needed list.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
*** Original post on bio 318 at 2012-05-24 17:40:59 UTC ***

(In reply to comment #29)
> Created attachment 8353267 [details] [diff] [review] (bio-attmnt 1515) [details]
> Followup Patch v2

Checked in as http://hg.instantbird.org/instantbird/rev/6742ed7806d2
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
You need to log in before you can comment on or make changes to this bug.