954270 - Give new tabs opened via IRC commands focus

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, defect)

Description

[Instantbot]

*** Original post on bio 837 by Gustavo <gustavoadolforivera AT gmail.com> at 2011-06-11 01:24:00 UTC ***

When we are on IRC and type the command 

/join chatroom_name

or 

/query nickname

we open a new conversation from the conversation were we are already. It would be great if that, as others, after typing those commands and opening the new conversation window, the new conversation takes the focus, or in other words, after opening the new conversatio it switches to it

Comment 1

[aleth [:aleth]]

*** Original post on bio 837 at 2012-02-16 23:41:34 UTC ***

Not sure this is desired behaviour (it might be - after all opening a channel via "Join Chat" focuses the new tab), but changing title to better reflect what is requested here.

Comment 2

[Patrick Cloke [:clokep]]

*** Original post on bio 837 at 2012-02-20 14:59:50 UTC ***

This should act in the same way as the Join Chat menu. Marking as new.

Comment 3

[aleth [:aleth]]

*** Original post on bio 837 at 2012-06-06 12:39:31 UTC ***

Should this really be fixed in Services.conversations.addConversation, or the constructor of UIConversation? 

The IRC code just ends up calling the appropriate _init in jsProtoHelper when a new conversation is opened. The only place one could add a focus call would be http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#914, but I'm not sure that UI issue really belongs in the IRC code.

"Join Chat" currently focuses the new conversation explicitly in http://lxr.instantbird.org/instantbird/source/instantbird/content/joinchat.js#119.

Comment 4

[Patrick Cloke [:clokep]]

*** Original post on bio 837 at 2012-06-06 12:44:05 UTC ***

(In reply to comment #3)
> Should this really be fixed in Services.conversations.addConversation, or the
> constructor of UIConversation? 
Possibly, but do we really always want to focus a new conversation when it's created? Probably not.

Comment 5

[aleth [:aleth]]

*** Original post on bio 837 at 2012-06-06 12:44:29 UTC ***

To be clear, my question is whether there are instances where we don't want new conversations to be focused.

Comment 6

[Florian Quèze [:florian]]

*** Original post on bio 837 at 2012-06-06 12:56:07 UTC ***

Focusing a conversation tab/window is something that has the potential to interrupt the user, so it needs to either:
- always come from a user action
- be cancelable from the interruption manager.

I don't really see how we could guarantee this if the focus call comes from the code of a protocol plugin.

Comment 7

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 837 as attmnt 1570 at 2012-06-06 15:05:00 UTC ***

Not very elegant, but it does work, and only focuses the new tab when triggered by a user command (/join or /msg or /query).

Potential issue: Usage of Conversations and imWindows in protocol code

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8353325 [details] [diff] [review]
Patch

*** Original change on bio 837 attmnt 1570 at 2012-06-06 15:07:25 UTC ***

(In reply to comment #7)

> Potential issue: Usage of Conversations and imWindows in protocol code

Obvious reason for r- actually :-(. imWindows.jsm is in instantbird/, chat/protocols/irc is also used by Thunderbird.

Comment 9

[aleth [:aleth]]

*** Original post on bio 837 at 2012-06-06 15:09:52 UTC ***

(In reply to comment #8)
> Obvious reason for r- actually :-(. imWindows.jsm is in instantbird/,
> chat/protocols/irc is also used by Thunderbird.

Well, I was expecting that ;) Any suggestions? A please-focus-this-conversation event?

Comment 10

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 837 as attmnt 1571 at 2012-06-06 21:23:00 UTC ***

So here's another WIP. It has the conversation binding check for flag via the wrappedJSObject of the purpleConversation to find out whether the conversation that has just been loaded should be focused. Obviously this is not ideal and I suppose the flag should be in the interface. But then the libpurple protocols would cause errors as they don't have it. If one did have such a flag (across all protocols) it should probably be set via the constructor as an optional parameter.

Not sure what to do with this. It does make joining channels and opening DMs via the keyboard much more pleasant however.

Comment 11

[aleth [:aleth]]

Comment on attachment 8353325 [details] [diff] [review]
Patch

*** Original change on bio 837 attmnt 1570 at 2012-06-06 21:23:32 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[Patrick Cloke [:clokep]]

*** Original post on bio 837 at 2012-06-06 21:31:12 UTC ***

(In reply to comment #10)
> Created attachment 8353326 [details] [diff] [review] (bio-attmnt 1571) [details]
> Patch
> 
> So here's another WIP. It has the conversation binding check for flag via the
> wrappedJSObject of the purpleConversation to find out whether the conversation
> that has just been loaded should be focused. Obviously this is not ideal and I
> suppose the flag should be in the interface. But then the libpurple protocols
> would cause errors as they don't have it.
Couldn't we just always set it to false for libpurple?

> If one did have such a flag (across
> all protocols) it should probably be set via the constructor as an optional
> parameter.
This seems to be much more ideal, yes.

What is the purpose of the _shouldFocusList? It seems to me you could just pass an optional second parameter to getConversation instead?

Comment 13

[aleth [:aleth]]

*** Original post on bio 837 at 2012-06-06 22:07:05 UTC ***

(In reply to comment #11)
> > So here's another WIP. It has the conversation binding check for flag via the
> > wrappedJSObject of the purpleConversation to find out whether the conversation
> > that has just been loaded should be focused. Obviously this is not ideal and I
> > suppose the flag should be in the interface. But then the libpurple protocols
> > would cause errors as they don't have it.
> Couldn't we just always set it to false for libpurple?

That would work to some extent, but then you're patching libpurple, right? Also it would mean you have to keep the existing code for joinChat and opening DMs for other protocols, as the flag won't have any effect for libpurple.

> > If one did have such a flag (across
> > all protocols) it should probably be set via the constructor as an optional
> > parameter.
> This seems to be much more ideal, yes.

> What is the purpose of the _shouldFocusList? It seems to me you could just pass
> an optional second parameter to getConversation instead?

You'd still need _shouldFocusList to decide whether to pass that parameter along or not (are we waiting for a conversation to be opened in response to a user command?), but with added duplication. (Actually you might be able to do without it for /msg, but not for /join). Or am I missing something?

Anyhow, as I can't test modifications to idl's and libpurple code myself, anyone want to take over this bug? ;)

Comment 14

[Patrick Cloke [:clokep]]

*** Original post on bio 837 at 2012-06-06 22:12:41 UTC ***

(In reply to comment #12)
> (In reply to comment #11)
> > > So here's another WIP. It has the conversation binding check for flag via the
> > > wrappedJSObject of the purpleConversation to find out whether the conversation
> > > that has just been loaded should be focused. Obviously this is not ideal and I
> > > suppose the flag should be in the interface. But then the libpurple protocols
> > > would cause errors as they don't have it.
> > Couldn't we just always set it to false for libpurple?
> 
> That would work to some extent, but then you're patching libpurple, right? Also
> it would mean you have to keep the existing code for joinChat and opening DMs
> for other protocols, as the flag won't have any effect for libpurple.

You could do it in purplexpcom, I think, which is code written entirely by us (by Florian, really).

> > > If one did have such a flag (across
> > > all protocols) it should probably be set via the constructor as an optional
> > > parameter.
> > This seems to be much more ideal, yes.
> 
> > What is the purpose of the _shouldFocusList? It seems to me you could just pass
> > an optional second parameter to getConversation instead?
> 
> You'd still need _shouldFocusList to decide whether to pass that parameter
> along or not (are we waiting for a conversation to be opened in response to a
> user command?), but with added duplication. (Actually you might be able to do
> without it for /msg, but not for /join). Or am I missing something?

I'm not sure if you're missing something, I still don't understand why this would be necessary. Couldn't you just give it to getConversation, then pass it to the constructor?

Comment 15

[aleth [:aleth]]

*** Original post on bio 837 at 2012-06-06 22:22:34 UTC ***

(In reply to comment #13)
> I'm not sure if you're missing something, I still don't understand why this
> would be necessary. Couldn't you just give it to getConversation, then pass it
> to the constructor?

An example: We then receive a message from the server that we have joined a channel, at which point getConversation is called. We need to know at this point if this join is in response to a user command (so that we should focus) or not. Only if we assume that any channel join is focus-worthy, can we do without shouldFocus in that instance. I thought we didn't want to do this (because of autojoins etc.)

Comment 16

[aleth [:aleth]]

*** Original post on bio 837 at 2013-06-15 11:52:12 UTC ***

For future reference, I don't like the patch in this bug, anyone picking up from there should make sure to read the comments and come up with something better ;)

Comment 17

[aleth [:aleth]]

Attachment: Patch

The changes made in bug 955703 make this fixable without too awful hacks (all the edge cases discussed in earlier comments no longer happen).

This also fixes the keyboard command loophole left open in bug 955680.

Comment 18

[Patrick Cloke [:clokep]]

Comment on attachment 8385375 [details] [diff] [review]
Patch

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

r- mostly for the comment changes, but please answer the questions I have too!

::: chat/protocols/irc/irc.js
@@ +1440,5 @@
>      // No need to join a channel we are already in.
> +    if (this.hasConversation(channel)) {
> +      let conv = this.getConversation(channel);
> +      if (!conv.left)
> +        return conv;

joinChat is supposed to return void according to the interface. Will this mess anything up?

::: chat/protocols/irc/ircCommands.jsm
@@ +43,2 @@
>  // aMsg is <user> <message>
>  function messageCommand(aMsg, aConv) {

Please update the comment to say it also focuses the conversation.

@@ +89,5 @@
>  function privateMessage(aConv, aMsg, aNickname) {
>    if (!aMsg.length)
>      return false;
>  
>    // This will open the conversation, send and display the text

Please update this comment (which should also probably be at the top of the function, by the way).

@@ +205,5 @@
>        params.forEach(function(joinParam) {
> +        if (joinParam) {
> +          let chatroomfields = account.getChatRoomDefaultFieldValues(joinParam);
> +          let conv = account.joinChat(chatroomfields);
> +          Services.obs.notifyObservers(conv, "user-focus-conv-request", null);

If we have multiple channels here...we focus each one in turn? That seems wrong.

::: im/modules/imWindows.jsm
@@ +101,5 @@
>    },
>  
>    init: function() {
>      let os = Services.obs;
> +    ["new-text", "user-focus-conv-request",

I assume TB doesn't need any updates for this, right? It should just ignore the extra topic, but do we WANT it to handle it?

Comment 19

[aleth [:aleth]]

Attachment: 954270.patch 2

(In reply to Patrick Cloke [:clokep] from comment #18)
> joinChat is supposed to return void according to the interface. Will this
> mess anything up?

As the return value is only used within IRC, this change is fine. It would be nice to add this behaviour to the interface, as that would improve the code in the other instances where joinChat is called, but it's not trivial to ensure a return value for all libpurple protocols. In any case that would be a separate bug as it does not affect commands.

> I assume TB doesn't need any updates for this, right? It should just ignore
> the extra topic, but do we WANT it to handle it?

This patch doesn't affect TB. If some equivalent of this feature is wanted for TB, a handler for the notification would have to be added. I think we can leave that for a followup, as I don't know the TB UI code well enough to trivially add it ;)

Comment 20

[Florian Quèze [:florian]]

Comment on attachment 8385412 [details] [diff] [review]
954270.patch 2

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

Thanks for looking into this.

I really dislike the prpl code sending a notification that the UI will blindly use to steal the user's focus. :(

I think the good fix here would be to change the command interface so that commands can return a conversation (or a Promise of a conversation?). Then the code that ran the command should be responsible for focusing the result of the command if that seems wanted.

::: im/modules/imWindows.jsm
@@ +124,5 @@
>        return;
>      }
>  
> +    if (aTopic == "user-focus-conv-request") {
> +      this.focusConversation(aSubject);

This would need to go through the interruption manager.

Comment 21

[aleth [:aleth]]

Attachment: 954270.patch 3

Adds an out parameter for commands.

I did not use the Interruptions manager as (now the patch uses a return value) we can be sure the user interrupted himself.

Comment 22

[aleth [:aleth]]

Attachment: 954270.patch 4

Sorry about the previous patch - forgot to hg qref before exporting.

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 8386721 [details] [diff] [review]
954270.patch 4

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

Thanks! I'm happy to see this is getting fixed; and really fixed, not hacked around :-).

::: chat/components/public/imICommandsService.idl
@@ +35,5 @@
>    // Will return true if the command handled the message (it should not be sent).
>    // The leading slash, the command name and the following space are not included
>    // in the aMessage parameter.
> +  // If aConvToFocus.value is returned, the caller is requested to focus
> +  // that conversation.

Only a comment on the wording here: I dislike "aConvToFocus" and "the caller is required to focus that conversation", because that implies the chat/ back end is making UI decisions.

I would prefer something along the lines of "aReturnedConv"/"aResultConv" and "If a conversation is returned as a result of executing the command, the caller should consider focusing it."

@@ +70,5 @@
>    // Will return true if a command handled the message (it should not be sent).
>    // The aConversation parameters is required to execute protocol specific
>    // commands. Application global commands will work without it.
> +  // If aConvToFocus.value is returned, the caller is requested to focus
> +  // that conversation.

My wording comment applies here too.

::: chat/components/src/imCommands.js
@@ +220,5 @@
>  
>      // Sort the matching commands by priority before returning the array.
>      return cmdArray.sort(function(a, b) b.priority - a.priority);
>    },
> +  executeCommand: function (aMessage, aConversation, aConvToFocus) {

Nit: we usually don't have a space between 'function' and '('. I know you didn't add it, but as you are editing the line, can you fix it?

Comment 24

[aleth [:aleth]]

Attachment: 954270.patch 5

> Only a comment on the wording here: I dislike "aConvToFocus" and "the caller
> is required to focus that conversation", because that implies the chat/ back
> end is making UI decisions.

The word I used was "request", implying the UI can ignore it, but I'm fine with the changes you suggested as that is clearer.

Comment 25

[Florian Quèze [:florian]]

Comment on attachment 8386750 [details] [diff] [review]
954270.patch 5

Thanks!

r=me for the chat/components and im/content/conversation.xml changes. I want clokep to review the IRC changes.

Comment 26

[Patrick Cloke [:clokep]]

Comment on attachment 8386750 [details] [diff] [review]
954270.patch 5

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

Overall this looks good! :)

::: chat/protocols/irc/ircCommands.jsm
@@ +215,3 @@
>        });
> +      if (aReturnedConv)
> +        aReturnedConv.value = conv;

I think we should focus the first conversation, not the last.

Comment 27

[aleth [:aleth]]

Comment on attachment 8386750 [details] [diff] [review]
954270.patch 5

(In reply to Patrick Cloke [:clokep] from comment #26)
> I think we should focus the first conversation, not the last.

My mental model here was to consider /join #a,#b to be /join #a followed by /join #b.

One edge case not addressed here is /join #a,#b when #a is on hold (in which case #a will stay on hold). This could be improved by returning an array of conversations, at the cost of some complexity (since we wouldn't want to focus all of them in turn). I'm not sure if it's worth it.

Comment 28

[Patrick Cloke [:clokep]]

Comment on attachment 8386750 [details] [diff] [review]
954270.patch 5

We talked about this over IRC and aleth has convinced me otherwise. At the very least I'd like to try it and see if it seems very counter intuitive to me.

Thanks for looking at this!

Comment 29

[Patrick Cloke [:clokep]]

I think this is a big enough improvement we should take it even with that edge case identified. Let's fix this in another bug.

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

Comment 31

[Patrick Cloke [:clokep]]

This broke the builds, we need to update purpleCommands for the interface changes.

Comment 32

[Florian Quèze [:florian]]

Attachment: Fix purple bustage

Comment 33

[aleth [:aleth]]

Comment on attachment 8388264 [details] [diff] [review]
Fix purple bustage

Thanks for fixing this!

>+                                 prplIConversation * *aReturnedConv,

I'm surprised at the space between ** - is that acceptable C++ style or a typo?

Comment 34

[Florian Quèze [:florian]]

(In reply to aleth from comment #33)
> Comment on attachment 8388264 [details] [diff] [review]

> >+                                 prplIConversation * *aReturnedConv,
> 
> I'm surprised at the space between ** - is that acceptable C++ style or a
> typo?

I didn't really pay attention to that; it's just a copy/paste from the .h generated from the .idl file.
I agree it's slightly strange. I guess we could go look for other out params in C++ and see what we did there.

Comment 35

[Patrick Cloke [:clokep]]

Oops, I pushed this. I forgot about the style issue...copying from the .h file seems like the "right thing to do" though, in my opinion.

http://hg.mozilla.org/users/florian_queze.net/purple/rev/93b8dd3e84b2