1197714 - Should not create more than one "Saved Files" download manager tab (about:downloads)

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #689034 +++

STR

1 Tools > Saved Files (or Ctrl+J)
2 Tools > Saved Files (or Ctrl+J) again

Actual result

two "Saved Files" tabs; yet another one every time; visual tab bar clutter without any purpose because they are all identical

Expected result

refocus first "Saved Files" tab; never create more than one "Saved Files" tab
(fyi, Addons tab has the desired behaviour)

Comment 1

[:aceman]

Paenglab, is it possible to block this, e.g. by some tab type?

Comment 2

[Richard Marti (:Paenglab)]

I'm not a specialist in this. For the prefs tab I'm using shouldSwitchTo instead of openTab when the tab is already open. Mostly I copied it from existing code, so I'm unfortunately not really a great help :(.

Comment 3

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #2)
> I'm not a specialist in this. For the prefs tab I'm using shouldSwitchTo
> instead of openTab when the tab is already open. Mostly I copied it from
> existing code

Maybe the same method could be tried here?

Comment 4

[:aceman]

Attachment: WIP patch

Unfortunately, it seems the Downloads tab has not a special tab type (it is a "chromeTab") so it is hard to find it via tab type (prefs tab has special "preferencesTab" type). I don't think we want to create a new type for it. So we can't use shouldSwitchTo. We can still use switchToTab(), if we somehow find out if the tab is already open, or have a variable referencing it.

So this patch would work for the normal case. However, when restoring a session with Saved files tab open, the gDownloadsTab variable of course does not get set. Any ideas here? :)

Comment 5

[Richard Marti (:Paenglab)]

This is already an improvement and could be used as a workaround when no better solution can be found.

But after the session restore you can open a second downloads tab and after closing this tab CTRL + j doesn't neither open a tab nor switch to the old downloads tab.

I hope Magnus has a solution how this could be solved.

Comment 6

[Thomas D. (:thomas8)]

Aceman, it looks like tab type "preferencesTab" is created exclusively in
http://mxr.mozilla.org/comm-central/source/mail/components/preferences/preferencesTab.js

Why not just copy that entire file into downloadsTab.js and tweak it a little to create a new tab type "downloadsTab"? It looks pretty straightforward at first sight.

Otherwise, any way of finding a tab based on properties like chromeTab and URL about:downloads?

Comment 7

[:aceman]

(In reply to Thomas D. from comment #6)
> Why not just copy that entire file into downloadsTab.js and tweak it a
> little to create a new tab type "downloadsTab"? It looks pretty
> straightforward at first sight.
Exactly because I do not like duplicating code if it is not needed. If we need all this just for not opening 2 downloads tabs. Preferences tab is probably special enough to define its own type.

> Otherwise, any way of finding a tab based on properties like chromeTab and
> URL about:downloads?
Yes, or maybe we could have an id on the tab. I think we do not have accessor functions to search for these properties, but I would support adding some.

Comment 8

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #7)
> (In reply to Thomas D. from comment #6)
> > Why not just copy that entire file into downloadsTab.js and tweak it a
> > little to create a new tab type "downloadsTab"? It looks pretty
> > straightforward at first sight.
> Exactly because I do not like duplicating code if it is not needed. If we
> need all this just for not opening 2 downloads tabs. Preferences tab is
> probably special enough to define its own type.

Well, even preferencesTab might get away with being found by an ID...

Indeed, it's a lot of code just for finding something which we clearly know is there, and which we can easily ensure to be unique.

> > Otherwise, any way of finding a tab based on properties like chromeTab and
> > URL about:downloads?
> Yes, or maybe we could have an id on the tab. I think we do not have
> accessor functions to search for these properties, but I would support
> adding some.

Sounds good. I support your support! :)

Comment 9

[:aceman]

Attachment: patch v2

This seems to work more reliably for me. I can find the proper tab via the aboutDownloads id.

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 8660435 [details] [diff] [review]
patch v2

Great, this works and uses always the same tab. And with the getBrowserForDocument this could be used for future things in a tab.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 8660435 [details] [diff] [review]
patch v2

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

LGTM! r=mkmelin

::: mail/base/content/tabmail.xml
@@ +1090,5 @@
>            return null;
>          ]]></body>
>        </method>
> +      <!-- getBrowserForDocument is used to find the browser for a specific
> +           document via its ID attribute -->

nit: id (lowercase)

Comment 12

[:aceman]

Attachment: patch v2.1 [checked in]

Thanks.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/comm-central/rev/579d2e9c7c21

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This patch added gDownloadsTab which is only there if a downloads tab has been opened and doesn't get used anywhere at the moment. Please followup with a variable declaration on the global level like at http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#54 or remove it. At the moment, code which wants to call it has to check like this:
> if (typeof(gDownloadsTab) === "undefined") {
>   ...
> } else {
>   ...
> }

Comment 15

[:aceman]

Yeah, sorry, the variable is a remnant of the previous solution in attachment 8657896 [details] [diff] [review]. It can be removed, thanks for catching it. Should I attach the patch here or you create a bug?

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

How you prefer.

Comment 17

[:aceman]

Attachment: patch, remove unneded variable [checked in]

Comment 18

[:aceman]

Thanks

Comment 19

[:aceman]

Comment on attachment 8666465 [details] [diff] [review]
patch, remove unneded variable [checked in]

https://hg.mozilla.org/comm-central/rev/4248c8b2abae