The default bug view has changed. See this FAQ.

Add-ons Manager window shows multiple times

RESOLVED FIXED in seamonkey2.1a3

Status

SeaMonkey
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Worcester12345, Assigned: Ian Neal)

Tracking

Trunk
seamonkey2.1a3
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -, blocking-seamonkey2.1 a3+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100517 SeaMonkey/2.1a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100517 SeaMonkey/2.1a2pre

When going to the Add-ons Manager, then to another window, then to Add-ons Manager again, it opens a new window instead of changing to the already open one.

Reproducible: Always

Steps to Reproduce:
1.Open Add-ons Manager
2.Switch to another window
3.Open Add-ons Manager

Actual Results:  
This will open a second Add-ons Manager window.

Expected Results:  
It should switch to the already open Add-ons Manager window.

No additional information.
(Reporter)

Updated

7 years ago
blocking2.0: --- → ?
Version: unspecified → Trunk
Blocks: 561600
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Thank you for the report, It is a known issue I plan to address. But having this as an open bug saves me a step anyway!
Assignee: nobody → bugspam.Callek
Status: NEW → ASSIGNED

Comment 2

7 years ago
Could we just make goAbout() implement the logic Firefox has in switchToTabHavingURI()? I think that probably all about pages want this - we can make it listen to a param, though, of course.
requesting blocking; (I am not sure if we need this to block a2, or if it can slip to a3)
blocking-seamonkey2.1: --- → ?

Updated

7 years ago
blocking-seamonkey2.1: ? → a3+

Comment 4

7 years ago
(In reply to comment #3)
> requesting blocking; (I am not sure if we need this to block a2, or if it can
> slip to a3)

too late for it to block a2 as SM 2.1a2 had been already released a few days ago.  SM 2.1a2 also has the multiple addon manager window problem as I've checked myself.

Comment 5

7 years ago
That's why it's not marked as blocking a2, but blocking a3 instead.
blocking2.0: ? → -
(Reporter)

Comment 6

7 years ago
Was this fixed? I just checked in on my bugs, and don't really remember seeing this lately.

Build identifier: Mozilla/5.0 (Windows; Windows NT 5.1; rv:2.0b2pre) Gecko/20100720 SeaMonkey/2.1a3pre
I have not done any work [yet] to prevent this, but its really now, "Addons manager tab shows multiple times". Though this is still on my plate.

Comment 8

7 years ago
Also, IMHO addon manager tab shoud be focused when opened, regardless of tab settings.
(In reply to comment #8)
> Also, IMHO addon manager tab shoud be focused when opened, regardless of tab
> settings.

+1. If the Add-on Manager was still a window and we opened that in the background that wouldn't make sense either, would it? Opening a tab in the background means that the user might want to look at it later. I doubt many people would like to do that with something they opened from the menus (directly or via a shortcut, once that is implemented).
(Reporter)

Comment 10

7 years ago
Feel free to migrate or change this bug as you please. You may even close it, as I don't experience it as far as I can tell any more.
(Assignee)

Comment 11

7 years ago
Created attachment 459645 [details] [diff] [review]
port switchToTabHavingURI to SM patch v0.1

This patch:
* Ports switchToTabHavingURI to SM
* Changes toEM to use switchToTabHavingURI

There may still be a few bugs left to iron out...
Assignee: bugspam.Callek → iann_bugzilla
Attachment #459645 - Flags: review?(bugspam.Callek)
(Assignee)

Updated

7 years ago
Attachment #459645 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 12

7 years ago
Created attachment 459659 [details] [diff] [review]
port switchToTabHavingURI to SM with newURI patch v0.1a [Checkin: Comment 19]

Changes since v0.1:
* Use newURI instead of makeURI so it works from Error Console
Attachment #459645 - Attachment is obsolete: true
Attachment #459659 - Flags: review?(bugspam.Callek)
Comment on attachment 459659 [details] [diff] [review]
port switchToTabHavingURI to SM with newURI patch v0.1a [Checkin: Comment 19]

>+  function switchIfURIInWindow(aWindow) {
>+    if (!("gBrowser" in aWindow))
>+      return false;

To me this might be concerning. Has this been tested when opened from our shipped-mini-applications. e.g. MailNews, Chatzilla, Composer, Venkman, etc.

I suspect it would only work when opened in Navigator, and it should work from at the _least_ Mailnews. I'd argue tools->Addons should _always_ work for us when able to be selected. So I'm not sure a direct port from firefox is possible here.
(Assignee)

Comment 14

7 years ago
(In reply to comment #13)
> Comment on attachment 459659 [details] [diff] [review]
> port switchToTabHavingURI to SM with newURI patch v0.1a
> 
> >+  function switchIfURIInWindow(aWindow) {
> >+    if (!("gBrowser" in aWindow))
> >+      return false;
> 
> To me this might be concerning. Has this been tested when opened from our
> shipped-mini-applications. e.g. MailNews, Chatzilla, Composer, Venkman, etc.
> 
> I suspect it would only work when opened in Navigator, and it should work from
> at the _least_ Mailnews. I'd argue tools->Addons should _always_ work for us
> when able to be selected. So I'm not sure a direct port from firefox is
> possible here.

Yes, I have tested from all the listed "shipped-mini-applications" even for mailnews using -mail on startup and it works. One of the things I tried before posting my first patch on here.

Comment 15

7 years ago
(In reply to comment #13)
> Comment on attachment 459659 [details] [diff] [review]
> port switchToTabHavingURI to SM with newURI patch v0.1a
> 
> >+  function switchIfURIInWindow(aWindow) {
> >+    if (!("gBrowser" in aWindow))
> >+      return false;
> 
> To me this might be concerning. Has this been tested when opened from our
> shipped-mini-applications. e.g. MailNews, Chatzilla, Composer, Venkman, etc.

If you read through to code to see when this function is actually called, you'll see that this is perfectly fine after all.
(In reply to comment #15)
> (In reply to comment #13)
> > Comment on attachment 459659 [details] [diff] [review] [details]
> > port switchToTabHavingURI to SM with newURI patch v0.1a
> > 
> > >+  function switchIfURIInWindow(aWindow) {
> > >+    if (!("gBrowser" in aWindow))
> > >+      return false;
> > 
> > To me this might be concerning. Has this been tested when opened from our
> > shipped-mini-applications. e.g. MailNews, Chatzilla, Composer, Venkman, etc.
> 
> If you read through to code to see when this function is actually called,
> you'll see that this is perfectly fine after all.

Bah right, stupid nested functions threw me for a loop on my quick-skim.  I also suspect this may fix: Bug 508223 (after that bugs prefs land) due to noticing the difference in http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/extensions/test/browser/head.js?mark=90-90,97-97#88
Comment on attachment 459659 [details] [diff] [review]
port switchToTabHavingURI to SM with newURI patch v0.1a [Checkin: Comment 19]

nit add to this patch: 
http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js?mark=7706-7708#7690

>+  // This can be passed either nsIURI or a string.
>+  if (!(aURI instanceof Components.interfaces.nsIURI))
>+    aURI = Services.io.newURI(aURI, null, null);

I like this use of the Services module rather than using makeURI like firefox does

>+  if (aOpenNew) {
>+    let browser = openUILinkIn(aURI.spec, "tab");

I'm not convinced actually that openUILinkIn is what we want (we might want to force-focus the tab, similar to Firefox here since it is obvious in most cases the user WANTS to see the addon manager right away), but frankly its how we handle it today, so sounds good anyway :-)

>+    if (aCallback) {
>+      // let browser = tabWindow.selectedBrowser;

nit: kill this comment entirely.

Otherwise good to land!
Attachment #459659 - Flags: review?(bugspam.Callek) → review+
(In reply to comment #17)
> Comment on attachment 459659 [details] [diff] [review]
> port switchToTabHavingURI to SM with newURI patch v0.1a
> 
> nit add to this patch: 
> http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js?mark=7706-7708#7690
> 

actually we don't have an impl of isTabEmpty() yet. And http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#6700 looks like it would need a review on our side, so you can ignore this one for now, at your discretion.
(Assignee)

Comment 19

7 years ago
Comment on attachment 459659 [details] [diff] [review]
port switchToTabHavingURI to SM with newURI patch v0.1a [Checkin: Comment 19]

http://hg.mozilla.org/comm-central/rev/adc6cec598be
Attachment #459659 - Attachment description: port switchToTabHavingURI to SM with newURI patch v0.1a → port switchToTabHavingURI to SM with newURI patch v0.1a [Checkin: Comment 19]
(Assignee)

Comment 20

7 years ago
(In reply to comment #18)
> (In reply to comment #17)
> > Comment on attachment 459659 [details] [diff] [review] [details]
> > port switchToTabHavingURI to SM with newURI patch v0.1a
> > 
> > nit add to this patch: 
> > http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js?mark=7706-7708#7690
> > 
> 
> actually we don't have an impl of isTabEmpty() yet. And
> http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#6700
> looks like it would need a review on our side, so you can ignore this one for
> now, at your discretion.

I'm not sure that we would want to delete the previous empty tab, but as you said that would need a review to implement isTabEmpty(). At the moment, I don't think we need it, but if you think either/both is required, please log bug(s) for it/them. I would say outside scope of this bug though.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 21

7 years ago
Please make sure you file a followup on isTabEmpty() and using it in that function. I suppose that's another of the tab API compat issues we want to fix for 2.1 if possible.

Comment 22

7 years ago
> +      browser.addEventListener("pageshow", function(event) {
> +        if (event.target.location.href != aURI.spec)
> +          return;
> +        browser.removeEventListener("pageshow", arguments.callee, true);

Doesn't this mean that in one code path, the event listener is never removed?

Updated

7 years ago
Blocks: 580223
(In reply to comment #22)
> > +      browser.addEventListener("pageshow", function(event) {
> > +        if (event.target.location.href != aURI.spec)
> > +          return;
> > +        browser.removeEventListener("pageshow", arguments.callee, true);
> Doesn't this mean that in one code path, the event listener is never removed?

No, this is here to catch the pageshow event that might fire on the about:blank of a new tab, once we get the addonmgr url (or really whatever url we want) we remove the handler.

Updated

7 years ago
Target Milestone: --- → seamonkey2.1a3

Updated

7 years ago
Blocks: 583997

Updated

7 years ago
Depends on: 586360
You need to log in before you can comment on or make changes to this bug.