Give new tabs opened via IRC commands focus

RESOLVED FIXED in 1.6

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bugzilla, Assigned: aleth)

Tracking

Dependency tree / graph

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
*** 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.
Severity: enhancement → normal
Summary: Change to the conversation recently open on IRC → Give new tabs opened via IRC commands focus
Version: 0.2 → trunk
*** 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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

5 years ago
Blocks: 954747
(Assignee)

Comment 3

5 years ago
*** 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.
*** 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.
(Assignee)

Comment 5

5 years ago
*** 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.
*** 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.
(Assignee)

Comment 7

5 years ago
Posted patch Patch (obsolete) — Splinter Review
*** 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
Attachment #8353325 - Flags: review?(clokep)
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.
Attachment #8353325 - Flags: review?(clokep) → review-
(Assignee)

Comment 9

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

Comment 10

5 years ago
Posted patch Patch (obsolete) — Splinter Review
*** 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.
(Assignee)

Comment 11

5 years ago
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 ***
Attachment #8353325 - Attachment is obsolete: true
*** 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?
(Assignee)

Comment 13

5 years ago
*** 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? ;)
*** 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?
(Assignee)

Comment 15

5 years ago
*** 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.)
(Assignee)

Comment 16

5 years ago
*** 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 ;)
(Assignee)

Comment 17

5 years ago
Posted patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → aleth
Attachment #8353326 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8385375 - Flags: review?(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?
Attachment #8385375 - Flags: review?(clokep) → review-
(Assignee)

Comment 19

5 years ago
Posted patch 954270.patch 2 (obsolete) — Splinter Review
(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 ;)
Attachment #8385375 - Attachment is obsolete: true
Attachment #8385412 - Flags: review?(clokep)
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.
Attachment #8385412 - Flags: review?(clokep) → review-
(Assignee)

Comment 21

5 years ago
Posted patch 954270.patch 3 (obsolete) — Splinter Review
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.
Attachment #8385412 - Attachment is obsolete: true
Attachment #8386716 - Flags: review?(clokep)
(Assignee)

Updated

5 years ago
Attachment #8386716 - Flags: review?(clokep) → review-
(Assignee)

Comment 22

5 years ago
Posted patch 954270.patch 4 (obsolete) — Splinter Review
Sorry about the previous patch - forgot to hg qref before exporting.
Attachment #8386716 - Attachment is obsolete: true
Attachment #8386721 - Flags: review?(clokep)
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?
Attachment #8386721 - Flags: feedback+
(Assignee)

Comment 24

5 years ago
> 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.
Attachment #8386721 - Attachment is obsolete: true
Attachment #8386721 - Flags: review?(clokep)
Attachment #8386750 - Flags: review?(florian)
Attachment #8386750 - Flags: review?(clokep)
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.
Attachment #8386750 - Flags: review?(florian) → review+
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.
Attachment #8386750 - Flags: review?(clokep) → review-
(Assignee)

Comment 27

5 years ago
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.
Attachment #8386750 - Flags: review- → review?
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!
Attachment #8386750 - Flags: review? → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
(Assignee)

Updated

5 years ago
Duplicate of this bug: 954747
This broke the builds, we need to update purpleCommands for the interface changes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 33

5 years ago
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?
Attachment #8388264 - Flags: review? → review+
(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.
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
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Blocks: 987420
You need to log in before you can comment on or make changes to this bug.