Closed
Bug 953761
Opened 11 years ago
Closed 11 years ago
Check if topic on IRC channels is editable and make UI respond accordingly
Categories
(Chat Core :: IRC, enhancement)
Chat Core
IRC
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: benediktp, Assigned: clokep)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 9 obsolete files)
(deleted),
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
*** 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.
Comment 1•11 years ago
|
||
*** Original post on bio 318 at 2012-02-21 23:03:35 UTC ***
Dupe.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Comment 2•11 years ago
|
||
*** 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) ***
Assignee | ||
Comment 3•11 years ago
|
||
*** 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
Assignee | ||
Comment 4•11 years ago
|
||
*** Original post on bio 318 at 2012-02-29 21:55:39 UTC ***
This is not a duplicate.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee | ||
Comment 6•11 years ago
|
||
*** 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 | ||
Updated•11 years ago
|
Assignee: nobody → clokep
Status: REOPENED → ASSIGNED
Comment 7•11 years ago
|
||
*** 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 8•11 years ago
|
||
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-
Assignee | ||
Comment 9•11 years ago
|
||
*** 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!
Assignee | ||
Comment 10•11 years ago
|
||
*** 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)
Assignee | ||
Comment 11•11 years ago
|
||
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
Comment 12•11 years ago
|
||
*** 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 13•11 years ago
|
||
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-
Assignee | ||
Comment 14•11 years ago
|
||
*** 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)
Assignee | ||
Comment 15•11 years ago
|
||
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 16•11 years ago
|
||
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-
Comment 17•11 years ago
|
||
*** 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!
Assignee | ||
Comment 18•11 years ago
|
||
*** 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)
Assignee | ||
Comment 19•11 years ago
|
||
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
Assignee | ||
Comment 20•11 years ago
|
||
*** 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)
Assignee | ||
Comment 21•11 years ago
|
||
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 22•11 years ago
|
||
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-
Assignee | ||
Comment 23•11 years ago
|
||
*** 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)
Assignee | ||
Comment 24•11 years ago
|
||
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 25•11 years ago
|
||
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+
Updated•11 years ago
|
Whiteboard: [checkin-needed]
Assignee | ||
Comment 26•11 years ago
|
||
*** 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)
Assignee | ||
Comment 27•11 years ago
|
||
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
Comment 28•11 years ago
|
||
*** 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 ;).
Updated•11 years ago
|
Whiteboard: [checkin-needed]
Assignee | ||
Comment 29•11 years ago
|
||
*** 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)
Assignee | ||
Comment 30•11 years ago
|
||
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)
Assignee | ||
Comment 31•11 years ago
|
||
*** 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)
Assignee | ||
Comment 32•11 years ago
|
||
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 33•11 years ago
|
||
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+
Assignee | ||
Comment 34•11 years ago
|
||
*** 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: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
Assignee | ||
Comment 35•11 years ago
|
||
*** 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 36•11 years ago
|
||
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+
Assignee | ||
Comment 37•11 years ago
|
||
*** 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)
Assignee | ||
Comment 38•11 years ago
|
||
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 39•11 years ago
|
||
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+
Updated•11 years ago
|
Whiteboard: [checkin-needed]
Assignee | ||
Comment 40•11 years ago
|
||
*** 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 → ---
Assignee | ||
Comment 41•11 years ago
|
||
*** 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: 11 years ago → 11 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
You need to log in
before you can comment on or make changes to this bug.
Description
•