Closed
Bug 1197714
Opened 9 years ago
Closed 9 years ago
Should not create more than one "Saved Files" download manager tab (about:downloads)
Categories
(Thunderbird :: Mail Window Front End, defect)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 44.0
People
(Reporter: thomas8, Assigned: aceman)
References
Details
(Keywords: ux-efficiency, ux-minimalism)
Attachments
(2 files, 2 obsolete files)
3.38 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
1.17 KB,
patch
|
aryx
:
review+
|
Details | Diff | Splinter Review |
+++ 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)
Paenglab, is it possible to block this, e.g. by some tab type?
Flags: needinfo?(richard.marti)
Comment 2•9 years ago
|
||
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)
Reporter | ||
Comment 3•9 years ago
|
||
(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?
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)
Comment 5•9 years ago
|
||
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)
Reporter | ||
Comment 6•9 years ago
|
||
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?
(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.
Reporter | ||
Comment 8•9 years ago
|
||
(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! :)
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 10•9 years ago
|
||
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•9 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•9 years ago
|
||
Thanks.
Attachment #8660435 -
Attachment is obsolete: true
Attachment #8663452 -
Flags: review+
Keywords: checkin-needed
Comment 13•9 years ago
|
||
https://hg.mozilla.org/comm-central/rev/579d2e9c7c21
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 44.0
Comment 14•9 years ago
|
||
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•9 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)
Comment 16•9 years ago
|
||
How you prefer.
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8666465 -
Flags: review?(aryx.bugmail)
Updated•9 years ago
|
Attachment #8666465 -
Flags: review?(aryx.bugmail) → review+
Attachment #8663452 -
Attachment description: patch v2.1 → patch v2.1 [checked in]
Assignee | ||
Comment 19•9 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•9 years ago
|
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•