If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Thunderbird 29.0

Status

Thunderbird
Instant Messaging
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Mook (work), Assigned: aleth)

Tracking

Trunk
Thunderbird 29.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
/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
Created attachment 8359733 [details] [diff] [review]
Move pref from im/ to chat/

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)
(Assignee)

Comment 3

4 years ago
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+
(Assignee)

Updated

4 years ago
Whiteboard: checkin-needed
(Assignee)

Updated

4 years ago
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)
(Assignee)

Comment 6

4 years ago
(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>"
(Assignee)

Comment 8

4 years ago
Created attachment 8361057 [details] [diff] [review]
bug959365.patch

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+
(Assignee)

Comment 11

4 years ago
(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?
(Assignee)

Comment 12

4 years ago
Created attachment 8361133 [details] [diff] [review]
bug959365.patch 2
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+
(Assignee)

Updated

4 years ago
Whiteboard: checkin-needed
https://hg.mozilla.org/comm-central/rev/3f8221754cf9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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.