Closed Bug 959365 Opened 10 years ago Closed 10 years ago

IRC: /list is broken due to chat.irc.automaticList missing

Categories

(Thunderbird :: Instant Messaging, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: mark.yen, Assigned: aleth)

References

()

Details

Attachments

(1 file, 2 obsolete files)

/list doesn't work in Daily (Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Thunderbird/29.0a1) because the chat.irc.automaticList preference is not |true|.  See URL (chat log in #instantbird).

Of course, setting it to |false| will *also* fail, so I'm not quite sure what the point of that pref is... :p
aleth, nhnt11: Why is this just throwing when we set this to false? What's going on here?
Depends on: 955503
Attached patch Move pref from im/ to chat/ (obsolete) — Splinter Review
This won't really fix the problem of the automaticList pref not making any sense, but this at least moves the default value to the right place so TB will pick up on it.

aleth and I think that this pref was initially to disable the awesometab, we then hooked up the /list command to use the same codepath without taking into account that this could be disabled.
Attachment #8359733 - Flags: review?(aleth)
Comment on attachment 8359733 [details] [diff] [review]
Move pref from im/ to chat/

Thanks. This fixes the brokenness, I'll file a separate patch to clean up this pref.
Attachment #8359733 - Flags: review?(aleth) → review+
Whiteboard: checkin-needed
Blocks: 959545
I'd like to have time to look into this before it's checked in.
Flags: needinfo?(florian)
Whiteboard: checkin-needed
Comment on attachment 8359733 [details] [diff] [review]
Move pref from im/ to chat/

I don't think this patch fixes the issue.

It looks like the requestRoomInfo method in irc.js wants a second optional boolean parameter to skip the pref check for when /list is an explicit user action, ie. the call from ircCommands.jsm.

Not sure if that boolean parameter should be exposed in imIAccount.idl, or just be an internal detail of the IRC implementation.
Attachment #8359733 - Flags: review-
Flags: needinfo?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> Comment on attachment 8359733 [details] [diff] [review]
> Move pref from im/ to chat/
> 
> I don't think this patch fixes the issue.

That's why I filed bug 959545 ;) (see above)

> It looks like the requestRoomInfo method in irc.js wants a second optional
> boolean parameter to skip the pref check for when /list is an explicit user
> action, ie. the call from ircCommands.jsm.

Agreed.
Comment on attachment 8359733 [details] [diff] [review]
Move pref from im/ to chat/

Thinking about this again, true may be a reasonable default for Tb too, so moving the default to chat/ is fine I guess.

I would like the comment above the pref to be rephrased to explain what this prefs enables, instead of what it breaks. Something like "if <name of new optional parameter> is <value>, requestRoomInfo will work only if this pref is set to <value>"
Attached patch bug959365.patch (obsolete) — Splinter Review
Moves pref to /chat, improves comments, makes /list work regardless of pref.
Assignee: clokep → aleth
Attachment #8359733 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8361057 - Flags: review?(clokep)
Comment on attachment 8361057 [details] [diff] [review]
bug959365.patch

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

Thanks for taking another look at this, I think it'll be good. I'd like Florian to take a look too.

::: chat/protocols/irc/irc.js
@@ +933,5 @@
>    _lastListTime: 0,
>    get isRoomInfoStale() Date.now() - this._lastListTime > kListRefreshInterval,
>    // Called by consumers that want a list of available channels, which are
>    // provided through the callback (prplIRoomInfoCallback instance).
> +  requestRoomInfo: function(aCallback, aIsUserRequest) {

Just a question, not a requirement: do we want this to be in the interface or not? We can punt on it and say "If another protocol requires this".

@@ +934,5 @@
>    get isRoomInfoStale() Date.now() - this._lastListTime > kListRefreshInterval,
>    // Called by consumers that want a list of available channels, which are
>    // provided through the callback (prplIRoomInfoCallback instance).
> +  requestRoomInfo: function(aCallback, aIsUserRequest) {
> +    if (!aIsUserRequest &&

Can you add a comment what this if statement does before we check this in? It's kind of complicated now with the two negatives.
Attachment #8361057 - Flags: superreview?(florian)
Attachment #8361057 - Flags: review?(clokep)
Attachment #8361057 - Flags: review+
(In reply to Patrick Cloke [:clokep] from comment #9)

[adding the optional parameter to the .idl file]
> We can punt on it and say "If another protocol requires this".

That's fine with me.
Attachment #8361057 - Flags: superreview?(florian) → feedback+
(In reply to Patrick Cloke [:clokep] from comment #9)
> > +  requestRoomInfo: function(aCallback, aIsUserRequest) {
> 
> Just a question, not a requirement: do we want this to be in the interface
> or not? We can punt on it and say "If another protocol requires this".

Yes, we only need to put it in the interface if we later add requestRoomInfo to other protocols /and/ those protocols also have a command equivalent. The latter seems unlikely.

> @@ +934,5 @@
> >    get isRoomInfoStale() Date.now() - this._lastListTime > kListRefreshInterval,
> >    // Called by consumers that want a list of available channels, which are
> >    // provided through the callback (prplIRoomInfoCallback instance).
> > +  requestRoomInfo: function(aCallback, aIsUserRequest) {
> > +    if (!aIsUserRequest &&
> 
> Can you add a comment what this if statement does before we check this in?
> It's kind of complicated now with the two negatives.

What kind of comment are you thinking of? Would a copy of the one above the pref do?
Attachment #8361057 - Attachment is obsolete: true
Attachment #8361133 - Flags: review?(clokep)
Comment on attachment 8361133 [details] [diff] [review]
bug959365.patch 2

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

Looks good, thanks!
Attachment #8361133 - Flags: review?(clokep) → review+
Whiteboard: checkin-needed
https://hg.mozilla.org/comm-central/rev/3f8221754cf9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: checkin-needed
Target Milestone: --- → Thunderbird 29.0
You need to log in before you can comment on or make changes to this bug.