595246 - Add possibility to manage available search engines from preferences and sidebar

Status: RESOLVED FIXED

(SeaMonkey :: Search, defect)

Description

[Robert Kaiser]

Bug 410613 allows easily installing new engines into SeaMonkey, but not a good path for managing or removing them again, we should add a solution for that. Note that bug 401417 also aims to add the engine manager from Firefox, bound to the search bar, it might be just enough to hook that into the other UI as well.

Comment 1

[Yhn]

This is duplicate to bug 599297.

Comment 2

[Yhn]

Or consider bug 599297 to be a more advanced feature to this bug? 
Or consider bug 599297 to be one of the implementation in Sidebar Search to this bug?

Comment 3

[Yhn]

And this bug does NOT depend on bug 410613!?

Comment 4

[Yhn]

(In reply to comment #0)
> Bug 410613 allows easily installing new engines into SeaMonkey, but not a good
> path for managing or removing them again, we should add a solution for that.
> Note that bug 401417 also aims to add the engine manager from Firefox, bound to
> the search bar, it might be just enough to hook that into the other UI as well.

You didn't realize that we should also manage/delete Mycroft plugins here.

Comment 5

[Robert Kaiser]

As patches currently stand, this depends on the implementation of a fitting dialog in bug 401417.

Comment 7

[Yhn]

(In reply to comment #6)
> *** Bug 599297 has been marked as a duplicate of this bug. ***

This bug does not cover all implement-requests in bug 599297.

Comment 8

[Yhn]

If you mark bug 599297 as a duplicate of this bug, you obstruct the resolvation of that bug because bug 599297 DOES NOT depeng on ANY one of bug 401417 and bug 410613.

Comment 9

[Robert Kaiser]

It does depend on the code written in those bugs as it will not be implemented without the work that is present there. If you have questions about how we are using this system, please join us in the #seamonkey IRC channel on irc.mozilla.org - thanks.

Comment 11

[Robert Kaiser]

Attachment: add calls to engine manager, make sure that one can call a browser

Here's a patch that can be taken on 2.1 still, no actual string changes. Note that I'll need to file a followup on adding accesskeys on trunk at least the one in prefs, I forgot to pre-add that for the 2.1 version.
The one tricky thing was to build something that ensures that engine manager itself can call the BrowserSearch object on a browser window to open up the page to get more search engines, but this solution seems to work reasonably well.

Comment 12

[Philip Chee]

What problem is the setTimeout() working around?

Comment 13

[[e-mail]]

(In reply to comment #12)
> What problem is the setTimeout() working around?
Probably didn't like being invoked from a menubutton; I notice that the block is copied from search.xml including the missing ; after window.focus()

Comment 14

[[e-mail]]

Comment on attachment 528052 [details] [diff] [review]
add calls to engine manager, make sure that one can call a browser

Review of attachment 528052 [details] [diff] [review]:

::: suite/common/pref/pref-search.xul
@@ +74,5 @@
+      <hbox align="start">
+        <spacer flex="1"/>
+        <button id="managerButton"
+                label="&engineManager.label;"
+                oncommand="OpenManager();"/>

You can get rid of the spacer by adding pack="end" to the hbox, and the align="start" isn't necessary (especially once you've got rid of the spacer).

An alternative is to get the button to align with the menulist, but that probably involves fiddling around with grids.

::: suite/common/search/engineManager.js
@@ +109,5 @@
   loadAddEngines: function engineManager_loadAddEngines() {
     this.onOK();
+    var win = window.opener;
+    // No browser window found as opener, look for one.
+    if (!win || !win.BrowserSearch)

[This is a modal dialog, so we should always have an opener, but I guess it looks more consistent this way.]

@@ +115,5 @@
+    // No browser open at all, launch one.
+    if (!win || !win.BrowserSearch) {
+      // Call loadAddEngines on an observer.
+      Services.obs.addObserver(function (aSubject, aTopic, aData) {
+        Services.obs.removeObserver(arguments.callee, aTopic, false);

arguments.callee is deprecated, the "strict" way is to name your observer function and remove it by name.

@@ +118,5 @@
+      Services.obs.addObserver(function (aSubject, aTopic, aData) {
+        Services.obs.removeObserver(arguments.callee, aTopic, false);
+        aSubject.BrowserSearch.loadAddEngines();
+        window.close();
+      }, "sessionstore-windows-restored", false);

I'm not sure we get this notification on a Mac, since it never restores the last closed window when you open a window, but I don't know how to check.

@@ +120,5 @@
+        aSubject.BrowserSearch.loadAddEngines();
+        window.close();
+      }, "sessionstore-windows-restored", false);
+      win = window.openDialog(Services.prefs.getCharPref("browser.chromeURL"),
+                              "_blank", "chrome,all,dialog=no");

Can't do this from a modal dialog. window.opener.openDialog should be OK though.

The irony is that loadAddEngines uses openUILinkIn which does all the heavy lifting for you if you only could write window.opener.loadAddEngines() instead. (And indeed somewhere to put OpenManager would be nice too.)

Comment 15

[Robert Kaiser]

(In reply to comment #14)
> arguments.callee is deprecated, the "strict" way is to name your observer
> function and remove it by name.

Hmm, need to play with how to get this done in a way that comes out as comact as this...

> I'm not sure we get this notification on a Mac, since it never restores the
> last closed window when you open a window, but I don't know how to check.

We're heavily relying on getting it in nsSuiteGlue, where we copied it from Firefox, so if we don't get it somewhere, there is a whole lot of things that have a problem. I was unsure myself but when I looked what nsSuiteGlue does on it, I felt good about using it.

> +        aSubject.BrowserSearch.loadAddEngines();
> +        window.close();
> +      }, "sessionstore-windows-restored", false);
> +      win = window.openDialog(Services.prefs.getCharPref("browser.chromeURL"),
> +                              "_blank", "chrome,all,dialog=no");
> 
> Can't do this from a modal dialog. window.opener.openDialog should be OK
> though.

Well, it worked fine for me where I tried it. Is Linux special or what you state not completely correct?

> The irony is that loadAddEngines uses openUILinkIn which does all the heavy
> lifting for you if you only could write window.opener.loadAddEngines() instead.
> (And indeed somewhere to put OpenManager would be nice too.)

I hope you don't mean that I should basically rewrite huge portions of what I'm doing here or make BrowserSearch be too different from what Firefox has and some add-ons might use. I'd hope I can get this in as it is and leave anything else to someone else. But yes, I would have been happier if loadAddEngines would live somewhere directly in the manager and I could just import utilityOverlay and have it do its stuff. But then, that would break compat of the BrowserSearch object, which I want to avoid. Life sucks, and I think what I did is best under the circumstances.

Comment 16

[[e-mail]]

(In reply to comment #15)
> > I'm not sure we get this notification on a Mac, since it never restores the
> > last closed window when you open a window, but I don't know how to check.
> We're heavily relying on getting it in nsSuiteGlue
Yes, but that's on startup, where we're expecting it. Restoring the previous session when (re)opening the browser window only applies on Windows and Linux and then only when the preference is set to restore previous session.

In fact due to the bug below it doesn't work anyway (and asserts) on Windows because the newly created window is modal to its modal opener and therefore gets destroyed when its opener does.

> > +        aSubject.BrowserSearch.loadAddEngines();
> > +        window.close();
> > +      }, "sessionstore-windows-restored", false);
> > +      win = window.openDialog(Services.prefs.getCharPref("browser.chromeURL"),
> > +                              "_blank", "chrome,all,dialog=no");
> > Can't do this from a modal dialog. window.opener.openDialog should be OK
> > though.
> Well, it worked fine for me where I tried it. Is Linux special or what you
> state not completely correct?
On Windows, if the preference isn't set to restore previous session, then the new window never fires the session-store-windows-restored notification and consequently remains modal, blocking access to the preference window.

> > The irony is that loadAddEngines uses openUILinkIn which does all the heavy
> > lifting for you if you only could write window.opener.loadAddEngines() instead.
> > (And indeed somewhere to put OpenManager would be nice too.)
> I hope you don't mean that I should basically rewrite huge portions of what I'm
> doing here
I was wondering how to avoid duplicating code. (Firefox doesn't have this problem because you can only manage engines from the search bar.) Still, at this point we've still got to get working code; nice code comes later ;-)

Comment 17

[Robert Kaiser]

Attachment: v2: make everything except the non-browser case work

Given that Callek wants to cut 2.1 ASAP, possibly tomorrow, I think it's best to do a patch that at least makes work what is no big discussion and just not make us do anything if there's no browser window to be found. I'll file a new bug for that and we should take that edge case there to be fixed in the next update version.

Comment 18

[Philip Chee]

I do hope that the DTD changes won't put the translators into a tizz.

Comment 19

[Robert Kaiser]

(In reply to comment #18)
> I do hope that the DTD changes won't put the translators into a tizz.

Those are OK as they don't actually change any entity or its contents, just removes comments about future use now that they're actually used. But thanks for noticing and caring about that!

Comment 20

[Robert Kaiser]

Thanks, pushed as http://hg.mozilla.org/comm-central/rev/5a3480d2f3a1 and http://hg.mozilla.org/releases/comm-2.0/rev/298197c2a6f5

(In reply to comment #17)
> I'll file a new
> bug for that and we should take that edge case there to be fixed in the next
> update version.

Filed bug 654420 on that.