773437 - tabmail needs a "unregisterTabType" method

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Thomas Schmid]

Currently there is no way to remove a tabtype form tabmail.

You can register new tabs via registerTabType but you can not unregister them.

This makes it impossible for a restartless addon to remove it's self cleanly upon deactivation or uninstall.

To make things even worse a registered tabtypes can't be replaced or updated. So ironically restartless addons using custom tab types can not be updated without a restart...

Comment 1

[Thomas Schmid]

Implementing a removeTabType Method is easier than expected. The code below is a quick & dirty hack but seems to work but has still issues.

If I find some time I'll try to post a patch.

What is the best way to handle open tabs? Just kill them, as soon as the tabtype is gone? Seems to be most suitable. Especially with restartless addons you cannot abort disable or uninstall.


function removeTabType(aTabType,tabmail)
{
  if ((aTabType.name in tabmail.tabTypes) == false)
    return;
 
  for (let [modeName] in Iterator(aTabType.modes))
  {
    if ( !tabmail.tabModes[modeName])
      continue;
        
    while (tabmail.tabModes[modeName].tabs.length)
    {
      // As we cannot abort deactivation or uninstall with restartless...
      // ... extension we need a force close option here...
      tabmail.closeTab(tabmail.tabModes[modeName].tabs[0],true)
    }
        
    delete tabmail.tabModes[modeName];
  }
    
  delete tabmail.tabTypes[aTabType.name]; 
    
  if (aTabType.name in tabmail.tabTypes)
    throw "error";
}

Comment 2

[Mark Banner (:standard8)]

(In reply to Thomas Schmid from comment #1)
> What is the best way to handle open tabs? Just kill them, as soon as the
> tabtype is gone? Seems to be most suitable. Especially with restartless
> addons you cannot abort disable or uninstall.

Can we not throw an exception back to the addon/caller? That way we can enforce the requirement that the add-on should close it tabs (in a nice user-facing way) first.

Comment 3

[Thomas Schmid]

(In reply to Mark Banner (:standard8) from comment #2)
> (In reply to Thomas Schmid from comment #1)
> > What is the best way to handle open tabs? Just kill them, as soon as the
> > tabtype is gone? Seems to be most suitable. Especially with restartless
> > addons you cannot abort disable or uninstall.
> 
> Can we not throw an exception back to the addon/caller? That way we can
> enforce the requirement that the add-on should close it tabs (in a nice
> user-facing way) first.

yes would be definitely the easiest. So you endup with something like the snipplet below. It looks strange but you need the doubled for loop. Tab modes are defined within a tab type object. You have to first check all tabmodes for open tabs, and then close tabmodes. E.g. you have three tab modes, the second one has open tabs. So this code first checks if all tabs are closed and then deletes tabmodes. If you would do it with one loop, it would delete the first mode, then throws as the second mode has open tabs. The third mode is untouched, so you would endup in an inconsistent state with some modes being removed.

function removeTabType(aTabType,tabmail)
{
  if ((aTabType.name in tabmail.tabTypes) == false)
    return;

  for (let [modeName] in Iterator(aTabType.modes))
    if ( tabmail.tabModes[modeName] && tabmail.tabModes[modeName].tabs.length)
      throw "Tab mode "+modeName+" still in use, close tabs";

  for (let [modeName] in Iterator(aTabType.modes))
    if ( tabmail.tabModes[modeName] && tabmail.tabModes[modeName].tabs.length)
      delete tabmail.tabModes[modeName];

  delete tabmail.tabModes[aTabType.name];
}

Anyhow a force close method for tabs would be great. Should I file a new bug?

Comment 4

[Thomas Schmid]

Attachment: Implements a method to unregister tab types, needed for bootstrapped extensions.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 648987 [details] [diff] [review]
Implements a method to unregister tab types, needed for bootstrapped extensions.

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

Hey, glad to have someone else hacking on tabmail! :)

This looks pretty good, just a few comments. See below.

Thanks,

-Mike

::: mail/base/content/tabmail.xml
@@ +111,5 @@
>      -   example of thinking.  This impacts spinny-thing feedback as well as
>      -   potential providing tab display hints.  aThinkingState may be a
>      -   boolean or a localized string explaining what you are thinking about.
>      -
> +    - Tab contributing code should define a tab type object and (un)register it

While I appreciate the effort to save space, I find this kind of documentation confusing. I'd prefer we explicitly mention unregistering tabs in a second sentence immediately after talking about tab type registration.

@@ +352,5 @@
>        </method>
> +      <method name="unregisterTabType">
> +        <parameter name="aTabType"/>
> +        <body><![CDATA[
> +        

Whitespace

@@ +357,5 @@
> +          // we can skip if the tabtype was never registered...
> +          if ((aTabType.name in this.tabTypes) == false)
> +             return;
> +
> +          // ... if the tabtype is stil in use, we can not remove it without

Typo: stil -> still

@@ +361,5 @@
> +          // ... if the tabtype is stil in use, we can not remove it without
> +          // breaking the UI. So we throw an exception.
> +          for (let [modeName] in Iterator(aTabType.modes))
> +            if ( this.tabModes[modeName] && this.tabModes[modeName].tabs.length)
> +              throw "Tab mode "+modeName+" still in use, close tabs";

spaces on either side of +.

Also, instead of just throwing a string, throw an Error so we can get a stack, like this:

throw new Error("Tab mode " + modeName + " still in use. Close tabs.");

Comment 6

[Thomas Schmid]

Attachment: v2 addressing the review comments

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 651498 [details] [diff] [review]
v2 addressing the review comments

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

See below.

::: mail/base/content/tabmail.xml
@@ +113,5 @@
>      -   boolean or a localized string explaining what you are thinking about.
>      -
>      - Tab contributing code should define a tab type object and register it
> +    -  with us by calling registerTabType. You can remove a registered tab
> +    -  type (eg when unloading a restartless addon) by calling unregister.

Should be "by calling unregisterTabType".

Also, please remove the trailing whitespace that was added to the lines below.

@@ +355,5 @@
> +      <method name="unregisterTabType">
> +        <parameter name="aTabType"/>
> +        <body><![CDATA[
> +          // we can skip if the tabtype was never registered...
> +          if ((aTabType.name in this.tabTypes) == false)

Instead of == false, we generally just use the ! boolean operator, so

if (!(aTabType.name in this.tabTypes))
  return;

@@ +361,5 @@
> +
> +          // ... if the tabtype is still in use, we can not remove it without
> +          // breaking the UI. So we throw an exception.
> +          for (let [modeName] in Iterator(aTabType.modes))
> +            if ( this.tabModes[modeName] && this.tabModes[modeName].tabs.length)

No space after (.

Also, if we're iterating through the aTabType.modes dictionary keys, we can assume that this.tabModes[modeName] exists, so you don't need to check that.

@@ +365,5 @@
> +            if ( this.tabModes[modeName] && this.tabModes[modeName].tabs.length)
> +              throw new Error("Tab mode " + modeName + " still in use. Close tabs");
> +
> +          // ... finally get rid of the tabtype
> +          for (let [modeName] in Iterator(aTabType.modes))

Hm. This doesn't make sense.

Line 364 starts a for-loop that iterates through aTabType.modes, checking this.tabModes[modeName] && this.tabModes[modeName].tabs.length. If that boolean resolves to true, we throw a new Error.

Line 369 starts a similar for-loop, that does the same checks, deleting the tabModes where they exist.

Line 371 will never be executed, because the only case where it *could* be executed is if this.tabModes[modeName] && this.tabModes[modeName].tabs.length resolves to true - but if that's the case, we would have thrown before we got here.

Or am I missing something here?

Comment 8

[Thomas Schmid]

Attachment: Addresses rereview comments

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 652141 [details] [diff] [review]
Addresses rereview comments

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

Yes, this looks way better.

I haven't tested this, but I assume you have. By inspection, this looks good to me. With the one nit I found fixed, r=me.

::: mail/base/content/tabmail.xml
@@ +114,5 @@
>      -
>      - Tab contributing code should define a tab type object and register it
> +    -  with us by calling registerTabType. You can remove a registered tab
> +    -  type (eg when unloading a restartless addon) by calling unregisterTabType.
> +    -  Each tab type can provide multiple tab modes. The rationale behind this 

Still some whitespace introduced here. Please remove it.

Comment 10

[Thomas Schmid]

> Yes, this looks way better.
> 
> I haven't tested this, but I assume you have. By inspection, this looks good
> to me. With the one nit I found fixed, r=me.

yes. I am using the logic already in as hack in my addon. So it's even tested in the fields. But instead of throwing an exception I am closing tabs. So I double checked the logic and it's now similar. The last if statement was obviously a copy and paste error.

> ::: mail/base/content/tabmail.xml
> @@ +114,5 @@
> >      -
> >      - Tab contributing code should define a tab type object and register it
> > +    -  with us by calling registerTabType. You can remove a registered tab
> > +    -  type (eg when unloading a restartless addon) by calling unregisterTabType.
> > +    -  Each tab type can provide multiple tab modes. The rationale behind this 
> 
> Still some whitespace introduced here. Please remove it.

What are the next steps to get this into the repository?
Does it need a second review?

Comment 11

[Thomas Schmid]

Attachment: just whitespae chaneges

Comment 12

[Mike Conley (:mconley) (:⚙️)]

(In reply to Thomas Schmid from comment #10)
> What are the next steps to get this into the repository?
> Does it need a second review?

With the whitespace fixed, follow these instructions to prepare the patch for check-in:  https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

And then put checkin-needed into the Keywords field for this bug.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 652149 [details] [diff] [review]
just whitespae chaneges

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

::: mail/base/content/tabmail.xml
@@ +116,5 @@
> +    -  with us by calling registerTabType. You can remove a registered tab
> +    -  type (eg when unloading a restartless addon) by calling unregisterTabType.
> +    -  Each tab type can provide multiple tab modes. The rationale behind this
> +    -  organization is that Thunderbird historically/currently uses a single
> +    -  3-pane view to display both three-pane folder browsing and single message 

The trailing whitespace is still here...

Comment 14

[Thomas Schmid]

Attachment: final patch for checkin

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/8a5384783bbb