1412710 - Implement a chat-only-mode for users that primarily use Thunderbird for chat

Status: NEW

(Thunderbird :: Instant Messaging, enhancement)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

The idea is to have a button in the toolbar in the chat tab, which toggles a "chat-only-mode". In this mode:

a) No tabstrip
b) The chat toolbar can live in the titlebar
c) Cmd+W should close the window, and new windows should go straight to Chat and the chat-only UI should persist

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: WIP

I've attached a rough patch that makes this work. Before I spend time ironing out edge cases and making the patch pretty, I'd like feedback on the approach.

I've tried to touch as little code/markup/styles as possible outside of chat specific files/directories.

Let me know what you think!

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8923216 [details] [diff] [review]
WIP

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

This seems promising :-).

We should be very careful about not getting in the way of users who also have email accounts. Ideas to reduce risk:
- if there's at least an email account configured (other than "local folders"), never enter the chat-only mode at startup / when creating a first window by clicking on the dock on Mac.
- when using any keyboard shortcut that changes the selected tab, exit the chat-only mode automatically
- in the future we'll want to have the /about command again, and opening eg. "/about memory" should make the tab bar reappear.

::: mail/base/content/messenger.xul
@@ +49,5 @@
>          defaultTabTitle="&defaultTabTitle.label;"
>          onload="OnLoadMessenger()"
>          onunload="OnUnloadMessenger()"
>          screenX="10" screenY="10"
> +        persist="width height screenX screenY sizemode chatonlymode"

Can we explore what the code would look like if we used a pref instead of persisting the attribute?

::: mail/components/im/content/chat-messenger-overlay.js
@@ +13,5 @@
>  imServices = imServices.Services;
>  
>  var gBuddyListContextMenu = null;
>  
> +let windowCloseListener = aEvent => {

function windowCloseListener(aEvent) {

@@ +21,5 @@
> +  }
> +}
> +
> +function toggleChatOnlyMode() {
> +  let doc = document.documentElement;

s/doc/elt/g (having "doc" and "document" used in the same function is confusing)

@@ +1165,5 @@
>        this.ChatCore.init();
>        this._addObserver("chat-core-initialized");
>      }
> +
> +    if (document.documentElement.hasAttribute("chatonlymode")) {

So I guess we would check the pref and set the attribute here if needed?

::: mail/components/im/content/chat-messenger-overlay.xul
@@ +113,5 @@
>  
>          <toolbarpalette id="ChatToolbarPalette">
> +          <toolbarbutton id="button-chat-only-mode"
> +                         class="toolbarbutton-1"
> +                         label="Chat only mode"

We can figure out the final label later, but I think this should talk about "email" rather than "chat". Like "Show email tab"/"Hide email tab".

::: mail/themes/osx/mail/chat.css
@@ +211,5 @@
> +
> +/* Chat-only mode */
> +
> +:root[chatonlymode] #tabs-toolbar {
> +    visibility: hidden;

2 space indent please

@@ +215,5 @@
> +    visibility: hidden;
> +}
> +
> +:root[chatonlymode] #tabmail-container {
> +    margin-top: -30px;

Is this value going to be platform specific?

@@ +223,5 @@
> +    -moz-appearance: -moz-window-titlebar !important;
> +}
> +
> +:root[chatonlymode] #chat-toobar {
> +    margin-inline-start: 75px;

Is this specific to mac?

@@ +232,5 @@
> +}
> +
> +:root[chatonlymode] #ibcontent,
> +:root[chatonlymode] .conv-textbox {
> +    font: menu !important;

What is this about? None of the following rules seem related to this bug.

@@ +269,5 @@
> +    font: menu !important;
> +}
> +
> +:root[chatonlymode] #status-bar {
> +  display: none;

Isn't "View -> Toolbars -> Status bar" enough?

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: WIP 2

Started working on review comments

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: WIP 3

Much better logic. I'll elaborate in a future Bugzilla comment when I request review.

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch

This patch adds a pref that enables the following behavior:

- When the chat tab is selected, the tabstrip is hidden and the chat toolbar is shown in the titlebar area.

- When the chat tab is not selected, the tabstrip is shown as normal.

- When the chat tab is selected, Cmd+W closes the window rather than the chat tab.

The pref is controlled by a toolbar button that can be added to the toolbar via the customization palette.

Comment 6

[Florian Quèze [:florian]]

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

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

Pretty nice :-).

When the tabstrip is hidden, I have sometimes have trouble dragging the window from an empty space of the toolbar. It's also difficult to customize away some "space" elements of the toolbar (especially if I had a second "space" element after the existing one between "join chat" and the status selector; attempting to remove that second space element while the tabstript is hidden is impossible. When the tab strip is visible there's no problem).

I also frequently saw this JS error in my terminal while the tabstrip was hidden:
JavaScript error: chrome://messenger/content/mailWindow.js, line 62: TypeError: getBrowser(...) is null

I'm not sure if this is related to the difficulty to drag the window or customizable elements, but it does seem to indicate that non-chat UI code still runs somehow when it shouldn't.

::: mail/components/im/content/chat-messenger-overlay.js
@@ +19,5 @@
> +function showTabstrip() {
> +  document.documentElement.removeAttribute("hidetabstrip");
> +}
> +
> +function hideTabstrip() {

I'm uncomfortable exposing symbols with such generic names in the global scope of the mail window. Could all these functions move to methods in the existing chatHandler object?

@@ +29,5 @@
> +}
> +
> +function figureOutTabstripHiding() {
> +  let tabcontainer = document.getElementById("tabcontainer");
> +  if (tabcontainer.selectedItem.getAttribute("type") == "chat") {

I don't think this check is correct/enough. I think we should not enable the chat-only mode if there's any non-chat tab.

The case I tested that seems wrong has these steps:
- in the chat tab, go into chat-only mode.
- use the searchbox to search for something in your chat logs using gloda, the gloda search results tab gets selected.
- select the chat tab back.

At this point, there's no way to find the gloda tab anymore (unless you know the keyboard shortcuts).

@@ +951,5 @@
>    _onTabActivated() {
>      let convView = chatHandler._getActiveConvView();
>      if (convView)
>        convView.switchingToPanel();
> +    figureOutTabstripHiding();

Why do we need to do this on tab (de)activated?

@@ +957,5 @@
>    _onTabDeactivated() {
>      let convView = chatHandler._getActiveConvView();
>      if (convView)
>        convView.switchingAwayFromPanel();
> +    figureOutTabstripHiding();

Should we just clear the chat-only pref when the chat tab gets de-selected?

::: mail/components/im/content/chat-messenger-overlay.xul
@@ +114,5 @@
>          <toolbarpalette id="ChatToolbarPalette">
> +          <toolbarbutton id="button-chat-only-mode"
> +                         class="toolbarbutton-1"
> +                         label="&hideTabstripButton.label;"
> +                         oncommand="toggleTabstripPref()"/>

This button should visibly act as a toggle (ie. it should stay pressed when the tabstrip is hidden (copy the behavior of the "Quick Filter" button on the main tab).

::: mail/locales/en-US/chrome/messenger/chat.dtd
@@ +31,5 @@
>  
>  <!ENTITY openConversationButton.tooltip  "Start a conversation">
>  <!ENTITY closeConversationButton.tooltip "Close conversation">
>  
> +<!ENTITY hideTabstripButton.label      "Hide tabstrip">

We'll need a less jargony string, "tabstrip" doesn't make sense for someone who hasn't touched our tab code.

::: mail/themes/osx/mail/chat.css
@@ +213,5 @@
>    margin-right: -2px;
>    margin-bottom: 18px;
>  }
> +
> +/* Chat-only mode */

What about Windows and Linux?

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v2

(In reply to Florian Quèze [:florian] from comment #6)
> Comment on attachment 8940200 [details] [diff] [review]
> Patch
> 
> Review of attachment 8940200 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Pretty nice :-).

I agree!

> When the tabstrip is hidden, I have sometimes have trouble dragging the
> window from an empty space of the toolbar. It's also difficult to customize
> away some "space" elements of the toolbar (especially if I had a second
> "space" element after the existing one between "join chat" and the status
> selector; attempting to remove that second space element while the tabstript
> is hidden is impossible. When the tab strip is visible there's no problem).

I couldn't figure this out :(

> I also frequently saw this JS error in my terminal while the tabstrip was
> hidden:
> JavaScript error: chrome://messenger/content/mailWindow.js, line 62:
> TypeError: getBrowser(...) is null

I didn't see this :-/

> ::: mail/components/im/content/chat-messenger-overlay.js
> @@ +19,5 @@
> > +function showTabstrip() {
> > +  document.documentElement.removeAttribute("hidetabstrip");
> > +}
> > +
> > +function hideTabstrip() {
> 
> I'm uncomfortable exposing symbols with such generic names in the global
> scope of the mail window. Could all these functions move to methods in the
> existing chatHandler object?

Done.

> @@ +29,5 @@
> > +}
> > +
> > +function figureOutTabstripHiding() {
> > +  let tabcontainer = document.getElementById("tabcontainer");
> > +  if (tabcontainer.selectedItem.getAttribute("type") == "chat") {
> 
> I don't think this check is correct/enough. I think we should not enable the
> chat-only mode if there's any non-chat tab.
> 
> The case I tested that seems wrong has these steps:
> - in the chat tab, go into chat-only mode.
> - use the searchbox to search for something in your chat logs using gloda,
> the gloda search results tab gets selected.
> - select the chat tab back.
> 
> At this point, there's no way to find the gloda tab anymore (unless you know
> the keyboard shortcuts).

OK, so I've made it so we just reset the pref if the tab is deactivated. So if you switch away from the chat tab for any reason (opening a search, keyboard shortcuts, etc etc) it's equivalent to disabling chat-only-mode.

> @@ +951,5 @@
> >    _onTabActivated() {
> >      let convView = chatHandler._getActiveConvView();
> >      if (convView)
> >        convView.switchingToPanel();
> > +    figureOutTabstripHiding();
> 
> Why do we need to do this on tab (de)activated?

Not sure what you're thinking of, but this is to show the tabstrip when switching away from the tab. Other panels' UIs are not designed for a tabstrip-hidden mode.

> @@ +957,5 @@
> >    _onTabDeactivated() {
> >      let convView = chatHandler._getActiveConvView();
> >      if (convView)
> >        convView.switchingAwayFromPanel();
> > +    figureOutTabstripHiding();
> 
> Should we just clear the chat-only pref when the chat tab gets de-selected?

This latest patch does this.

> ::: mail/components/im/content/chat-messenger-overlay.xul
> @@ +114,5 @@
> >          <toolbarpalette id="ChatToolbarPalette">
> > +          <toolbarbutton id="button-chat-only-mode"
> > +                         class="toolbarbutton-1"
> > +                         label="&hideTabstripButton.label;"
> > +                         oncommand="toggleTabstripPref()"/>
> 
> This button should visibly act as a toggle (ie. it should stay pressed when
> the tabstrip is hidden (copy the behavior of the "Quick Filter" button on
> the main tab).

Added type="checkbox"

> ::: mail/locales/en-US/chrome/messenger/chat.dtd
> @@ +31,5 @@
> >  
> >  <!ENTITY openConversationButton.tooltip  "Start a conversation">
> >  <!ENTITY closeConversationButton.tooltip "Close conversation">
> >  
> > +<!ENTITY hideTabstripButton.label      "Hide tabstrip">
> 
> We'll need a less jargony string, "tabstrip" doesn't make sense for someone
> who hasn't touched our tab code.

I made it "Compact Mode", how's that?

> ::: mail/themes/osx/mail/chat.css
> @@ +213,5 @@
> >    margin-right: -2px;
> >    margin-bottom: 18px;
> >  }
> > +
> > +/* Chat-only mode */
> 
> What about Windows and Linux?

Can I file a follow-up bug for this in the interest of landing this soon? The toggle button is hidden away in the palette, and I'm honestly okay with just putting the markup for it in an #ifdef XP_MACOSX for now.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #7)
> Created attachment 8954939 [details] [diff] [review]
> Patch v2

BTW, this new patch also gets rid of the hook that closes the window on Cmd+W if the tabstrip is hidden. Now, it just unhides the tabstrip and closes the Chat tab as usual.

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Florian Quèze [:florian] from comment #6)
> Comment on attachment 8940200 [details] [diff] [review]
> Patch
> 
> Review of attachment 8940200 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I also frequently saw this JS error in my terminal while the tabstrip was
> hidden:
> JavaScript error: chrome://messenger/content/mailWindow.js, line 62:
> TypeError: getBrowser(...) is null
> 
> I'm not sure if this is related to the difficulty to drag the window or
> customizable elements, but it does seem to indicate that non-chat UI code
> still runs somehow when it shouldn't.

I managed to reproduce this. Seems like onCopyOrDragStart is being called in mailWindow.js. Worth investigating, and it'll probably solve the issue of difficulty dragging the window. Maybe in a follow-up bug though?

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8954939 [details] [diff] [review]
Patch v2

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

Seems reasonable overall, but IMO the _isTabstripHidden and _figureOutTabstripHiding method are over abstraction inherited from previous versions of the patch.

::: mail/components/im/content/chat-messenger-overlay.js
@@ +414,5 @@
> +    }
> +  },
> +
> +  _isTabstripHidden: function() {
> +    return document.documentElement.getAttribute("hidetabstrip") == "true";

Your css has :root[hidetabstrip], not :root[hidetabstrip="true"] so I think to match it this should use hasAttribute

This method is called only once, could we just inline it?

@@ +433,5 @@
> +  },
> +
> +  toggleTabstripPref: function() {
> +    Services.prefs.setBoolPref(this._kHideTabstripPref,
> +      !Services.prefs.getBoolPref(this._kHideTabstripPref, false));

Is there any way for this to be called without a chat tab being currently selected? If not, can we just put in a local variable the !Services.prefs.getBoolPref(this._kHideTabstripPref, false)) value, and then call _hideTabstrip or _showTabstrip?

_showTabstrip has a clearUserPref call, should _hideTabstrip do the setBoolPref call?

@@ +957,5 @@
>    _onTabActivated() {
>      let convView = chatHandler._getActiveConvView();
>      if (convView)
>        convView.switchingToPanel();
> +    chatHandler._figureOutTabstripHiding();

Can you give me an example of case where this line is needed?

@@ +963,5 @@
>    _onTabDeactivated() {
>      let convView = chatHandler._getActiveConvView();
>      if (convView)
>        convView.switchingAwayFromPanel();
> +    chatHandler._figureOutTabstripHiding();

Should this just call _showTabstrip()? (maybe with an hasAttribute("hidetabstrip") check before the call if we think the call would be expensive?

::: mail/components/im/content/chat-messenger-overlay.xul
@@ +111,5 @@
>  #endif
>                   defaultset="button-add-buddy,button-join-chat,spacer,chat-status-selector,button-chat-accounts,spacer,gloda-im-search,button-chat-appmenu"/>
>  
>          <toolbarpalette id="ChatToolbarPalette">
> +          <toolbarbutton id="button-chat-only-mode"

Given that you only implemented the CSS for mac, should this be ifdef'ed?

::: mail/locales/en-US/chrome/messenger/chat.dtd
@@ +31,5 @@
>  
>  <!ENTITY openConversationButton.tooltip  "Start a conversation">
>  <!ENTITY closeConversationButton.tooltip "Close conversation">
>  
> +<!ENTITY compactModeButton.label      "Compact Mode">

nit: one more space to aligne with the next 3 strings.