Add possibility to manage available search engines from preferences and sidebar

RESOLVED FIXED in seamonkey2.1final

Status

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: kairo, Assigned: kairo)

Tracking

Trunk
seamonkey2.1final
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

9 years ago
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.
Version: SeaMonkey 2.0 Branch → Trunk

Comment 1

9 years ago
This is duplicate to bug 599297.

Comment 2

9 years ago
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?

Updated

9 years ago
Blocks: 599297

Comment 3

9 years ago
And this bug does NOT depend on bug 410613!?

Comment 4

9 years ago
(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.
Assignee

Comment 5

9 years ago
As patches currently stand, this depends on the implementation of a fitting dialog in bug 401417.
No longer blocks: 599297
Depends on: 401417
Assignee

Updated

9 years ago
Duplicate of this bug: 599297

Comment 7

9 years ago
(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

9 years ago
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.
Assignee

Comment 9

9 years ago
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.
Assignee

Updated

9 years ago
Duplicate of this bug: 599297
Assignee

Comment 11

8 years ago
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.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #528052 - Flags: review?(neil)

Comment 12

8 years ago
What problem is the setTimeout() working around?
(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 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.)
Assignee

Comment 15

8 years ago
(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.
(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 ;-)
Assignee

Comment 17

8 years ago
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.
Attachment #528052 - Attachment is obsolete: true
Attachment #529618 - Flags: review?(neil)
Attachment #528052 - Flags: review?(neil)
Attachment #529618 - Flags: review?(neil) → review+
Assignee

Updated

8 years ago
Attachment #529618 - Flags: approval-seamonkey2.1?

Updated

8 years ago
Attachment #529618 - Flags: approval-seamonkey2.1? → approval-seamonkey2.1+

Comment 18

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

Comment 19

8 years ago
(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!
Assignee

Updated

8 years ago
Blocks: 654420
Assignee

Comment 20

8 years ago
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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1final
You need to log in before you can comment on or make changes to this bug.