Last Comment Bug 665678 - Open Add-ons Manager, Data Manager etc. according to Link Behavior preferences (i.e. in a window if the user chose so)
: Open Add-ons Manager, Data Manager etc. according to Link Behavior preference...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.7
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-20 13:37 PDT by Dan Mellem
Modified: 2012-02-14 14:12 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch [Checkin: comment 8] (1.08 KB, patch)
2011-09-30 14:56 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
philip.chee: feedback+
Details | Diff | Review

Description Dan Mellem 2011-06-20 13:37:33 PDT
Opening the new Data Manager, Cookie Manager, Popup Manager, or Image Permissions opens as a new tab, instead as a separate window such as the Preferences or Bookmark Manager, or how all of these used to work. First, I think this is awkward since you normally don't expect a new tab for system settings, especially if you don't normally use tabs. Second, this should probably, as a minimum, honor Preferences/Browser/Link Behavior so it's not confusing to users that don't use tabs. Thanks.
Comment 1 Jens Hatlak (:InvisibleSmiley) 2011-08-08 09:11:38 PDT
According to the below, it's at least Add-ons Manager, Data Manager, and possibly Tabs From Other Computers (but I'd say the correct thing in the latter case would be to do as with Home, i.e. replace the current page). Adjusting summary and moving.

http://mxr.mozilla.org/comm-central/search?string=switchToTabHavingURI&find=suite&tree=comm-central

The interesting part therein is probably this:
  openUILinkIn(aURI.spec, "tabfocused");
Comment 2 Robert Kaiser (not working on stability any more) 2011-08-08 09:44:52 PDT
switchToTabHavingURI currently takes for granted that it's always tabs are are wanted, I guess we might want to look into changing that.
Comment 3 Jens Hatlak (:InvisibleSmiley) 2011-09-30 14:56:24 PDT
Created attachment 563858 [details] [diff] [review]
patch [Checkin: comment 8]

Not taking yet, I need some feedback first. Could it really be this easy? I found that it did what it should, including cases like Manage Stored Cookies or Manage Image Permissions which open the DM with a certain view.

You'll see that the code is ripped from loadAddSearchEngines. Would it make sense to let that one call switchToTabHavingURI, too?

Philip, is changing switchToTabHavingURI OK from an FF/add-ons compat POV?
Comment 4 Philip Chee 2011-10-01 01:59:23 PDT
Comment on attachment 563858 [details] [diff] [review]
patch [Checkin: comment 8]

> Not taking yet, I need some feedback first. Could it really be this easy? I
> found that it did what it should, including cases like Manage Stored Cookies
> or Manage Image Permissions which open the DM with a certain view.

1. Looks logical. I assume you tested all callers.

> You'll see that the code is ripped from loadAddSearchEngines. Would it make
> sense to let that one call switchToTabHavingURI, too?

Sounds reasonable, but I'd check to see if you don't regress anything.

> Philip, is changing switchToTabHavingURI OK from an FF/add-ons compat POV?

It should not be a problem as the "new window" case opens the manager in a _tab_ in a new tabbed browser window anyway.
Comment 5 neil@parkwaycc.co.uk 2011-10-06 02:38:34 PDT
Comment on attachment 563858 [details] [diff] [review]
patch [Checkin: comment 8]

Looks reasonable, but I think we should be using open_external rather than open_newwindow which should be solely for diverting clicked links. (Sorry for not spotting it in the search engine code before.)
Comment 6 Robert Kaiser (not working on stability any more) 2011-10-06 05:04:17 PDT
IMHO, using open_external for anything coming from _inside_ SeaMonkey was an error all along, but unfortunately it's where we ended up. :(
Comment 7 Jens Hatlak (:InvisibleSmiley) 2011-10-06 15:19:31 PDT
(In reply to neil@parkwaycc.co.uk from comment #5)
> I think we should be using open_external rather than
> open_newwindow which should be solely for diverting clicked links.

Well, it's choosing between bad and worse. The one is meant to be used only for links, the other only for things opened from external applications (cf. Preferences/Browser/Link Behavior wording). The only clean solution would be to introduce yet another pref which would then have to be added to the Link Behavior pref panel. But I guess that's not worth it. I have no particular preference (heh) which rule to break, so why not go with your suggestion.

> (Sorry for not spotting it in the search engine code before.)

Actually this case supports your rule: The trigger in the search engine manager is a link which opens a web page. Thus it should stay open_newwindow, which is why I'll keep the loadAddSearchEngines as it is.
Comment 8 Jens Hatlak (:InvisibleSmiley) 2011-10-06 15:23:00 PDT
Comment on attachment 563858 [details] [diff] [review]
patch [Checkin: comment 8]

http://hg.mozilla.org/comm-central/rev/5d28210323c4
with pref = browser.link.open_external
Comment 9 Jens Hatlak (:InvisibleSmiley) 2011-10-06 15:24:25 PDT
I think this should bake a bit on trunk, then we can think about having it on Aurora, too.
Comment 10 Teemu Mannermaa (:wicked) 2011-10-08 01:37:51 PDT
IMO, logically this should have used the browser.link.open_newwindow as b.l.open_external is for "links from other applications" and not something coming from inside SM itself. I don't see why about:addons or about:data are not "links" in your eyes. :) Besides, both prefs are about links so that shouldn't matter. It's the link origin that determines the pref to use.
Comment 11 Jens Hatlak (:InvisibleSmiley) 2011-10-08 02:30:37 PDT
(In reply to Teemu Mannermaa (:wicked) from comment #10)
> IMO, logically this should have used the browser.link.open_newwindow as
> b.l.open_external is for "links from other applications" and not something
> coming from inside SM itself.

Please read comment 7. Thanks.
Comment 12 rsx11m 2012-02-14 14:12:23 PST
Just seeing this on a SM 2.7.1 installation where I have browser.link.open_external coincidentally set to "1" (corresponding to "The current tab/window"). However, both Data Manager and Add-ons Manager open in a /new/ window rather than in the currently active one.

Is that the intended result for this setting?

"A new tab in the current window" shows the default behavior as before.

Note You need to log in before you can comment on or make changes to this bug.