The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 44.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Thomas D. (currently busy elsewhere; needinfo?me), Assigned: aceman)

Tracking

({ux-efficiency, ux-minimalism})

Trunk
Thunderbird 44.0
ux-efficiency, ux-minimalism

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

+++ 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)
(Assignee)

Comment 1

2 years ago
Paenglab, is it possible to block this, e.g. by some tab type?
Flags: needinfo?(richard.marti)
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 :(.
Flags: needinfo?(richard.marti)
(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?
(Assignee)

Comment 4

2 years ago
Created attachment 8657896 [details] [diff] [review]
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? :)
Flags: needinfo?(richard.marti)
Flags: needinfo?(mkmelin+mozilla)
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.
Flags: needinfo?(richard.marti)
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?
(Assignee)

Comment 7

2 years ago
(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.
(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! :)
(Assignee)

Comment 9

2 years ago
Created attachment 8660435 [details] [diff] [review]
patch v2

This seems to work more reliably for me. I can find the proper tab via the aboutDownloads id.
Assignee: nobody → acelists
Attachment #8657896 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Attachment #8660435 - Flags: ui-review?(richard.marti)
Attachment #8660435 - Flags: review?(mkmelin+mozilla)
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.
Attachment #8660435 - Flags: ui-review?(richard.marti) → ui-review+

Comment 11

2 years ago
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)
Attachment #8660435 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 12

2 years ago
Created attachment 8663452 [details] [diff] [review]
patch v2.1 [checked in]

Thanks.
Attachment #8660435 - Attachment is obsolete: true
Attachment #8663452 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/579d2e9c7c21
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 44.0
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 {
>   ...
> }
Flags: needinfo?(acelists)
(Assignee)

Comment 15

2 years ago
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?
Flags: needinfo?(acelists)
How you prefer.
(Assignee)

Comment 17

2 years ago
Created attachment 8666465 [details] [diff] [review]
patch, remove unneded variable [checked in]
Attachment #8666465 - Flags: review?(aryx.bugmail)
Attachment #8666465 - Flags: review?(aryx.bugmail) → review+
(Assignee)

Comment 18

2 years ago
Thanks
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Attachment #8663452 - Attachment description: patch v2.1 → patch v2.1 [checked in]
(Assignee)

Comment 19

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

https://hg.mozilla.org/comm-central/rev/4248c8b2abae
Attachment #8666465 - Attachment description: patch, remove unneded variable → patch, remove unneded variable [checked in]

Updated

a year ago
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.