232272 - Better management of search engines for toolbar over the current add new engines system (ability to remove and order engines)

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Jennifer]

User-Agent:       
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7a) Gecko/20040102 Firebird/0.7+

There should be a GUI method implemented for the management of search engine
extensions in the toolbar. It is a straightforward process to add new ones using
the "Add New Engines" option in the drop-down menu, but there is presently no
way to use the GUI to manage installed plugins. Currently, the files have to be
removed manually from the searchplugins folder through the filesystem, which
works fine but is a hassle. Instead of an "add engines" menuitem which goes
directly to Mycroft, a GUI interface should be implemented, perhaps under "Web
Features" which could have controls for adding (which would go to Mycroft),
removing (would work like the extensions list - click on one and click remove to
get rid of it). Finally, search engines should be stored in user profile, not in
the browser root, so they persist if the browser is upgraded.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Greg Campbell]

I think the interface should still be accessible from the search bar. Instead of
"Add Engines..." it could be "Manage Engines..." which would pop up the same
part of the Options GUI you described.

Also, engines should generally be stored in the profile, but still allowing
search plugins in the browser root allows for cross-profile search plugins. It
would make sense to keep the default Google search in the browser root and let
users add new searches to the profile. It would be nice if regular plugins were
handled this way too, but that's another bug.

Comment 2

[Michael Hendy (Hendikins)]

*** Bug 235852 has been marked as a duplicate of this bug. ***

Comment 3

[Jaap A. Haitsma]

I would like to suggest to maybe not have a dialog (or have a dialog and what I
am proposing) but:
1. A context menu with the option to show the properties and the possibility to
delete the search engine
2. The ability to drag and drop the search engines just as bookmarks in order to
change their ordering

The fact that engines should be stored in the profile is also discussed in Bug
235600

Comment 4

[Prognathous]

Now that there's an easy way to manage Themes and Extensions, it just begs to
make Serach Engine list editable as well.

Nominating for v1.0

Prog.

Comment 5

[vassily "vr" ragosin]

Sort! It's only natural to sort the engines as they appear in the list according
to user prefs. If you have lots of engines -- this would be very welcome.
As an alternative, just have it listed the engines in order of last use, it's
anyway better than the order of last added.

Installing engines to profile directory is very much needed for fastupgraders, I
always forget to save the engines when I reinstall Firefox.

Comment 6

[Pierre Chanial]

-> search component

Comment 7

[Peter van der Woude [:Peter6]]

if there is going to be a context menu, please don't forget to include "New
Separator". This would make it easy to group search engines, 
like e.g. 

Google
Yahoo
---------------
ebay
---------------
Dictionary.com
Babelfish EN-NL
Babelfish NL-EN
Babelfish DU-NL
Babelfish NL-DU
---------------
Add engines....

In context menu there should also be a properties option allowing people to
change the title of the engine

Comment 8

[Walter K.]

(In reply to comment #5)
> Sort!
Search engines are sorted in alpha order. (After restart for sure) That seems OK
for now.

Also, there are bug 232638 and bug 245558 that look more critical to me. Bug
232638 Comment #8 has logical suggestions to the way search engines have to be
managed.

Comment 9

[Peter van der Woude [:Peter6]]

It would have been nice if "Firefox" would have given a reason to -1.0

"Firefox 1.0 goal" -> feature complete and stable.

minussing this leaves it feature incomplete.

I don't think a deadline date should be a reason to compromise, what is in
branch now should work by the time we reach FF1.0 Milestone.

Comment 10

[Steffen Wilberg]

*** Bug 259761 has been marked as a duplicate of this bug. ***

Comment 11

[rvalles]

With 1.0PR, after a engine is added to the search engines toolbar, there's no
way to remove it. I think it's a serious issue, and 1.0 should have at least
this much implemented.

Comment 12

[Pierre Chanial]

I have written the backend to delete search engines. To keep it simple, removing
search engines will be done with the delete key in the engine popup.
No manager for now.

Comment 13

[Peter van der Woude [:Peter6]]

Maybe we need a simple way to add the default engines in case someone
accidentally deletes one.
The "add engines" now leads to www.mozdev.org which is from my point of view the
wrong place to look.
This should really be coming from UMO, like anything else
(extensions/plugins/themes) have to WE TRUST.

The less scattered (on the web) all "plugins" are, the more professional it looks.

If needed I can file a bug for it.

Comment 14

[Mike Frysinger]

at the very least it should work the same way auto-complete lists are managed
... you highlight an entry and hit shift+del ... making the search engines list
behave slightly different from other lists seems to be the wrong direction ...

Comment 15

[Peter van der Woude [:Peter6]]

(In reply to comment #12)
> I have written the backend to delete search engines. To keep it simple, removing
> search engines will be done with the delete key in the engine popup.
> No manager for now.

Pierre, with your patch, does the user get a popup ?
|-------------------------------------------|
|                                         X |
| Do you want to delete this searchengine ? |
|                                           |
|                [yes] [no]                 |
|-------------------------------------------|

I think that would be the minimally required UI ;-)

if that is the case I'd be tempted to ?1.0 since we have a patch.

Comment 16

[Peter van der Woude [:Peter6]]

*** Bug 262958 has been marked as a duplicate of this bug. ***

Comment 17

[Phil Ringnalda (:philor)]

*** Bug 261677 has been marked as a duplicate of this bug. ***

Comment 18

[David Janes]

I think something should be done for 1.0 milestone. It's the sort of thing
reviewers will pick up on and it's so trivial.

Comment 19

[Dave]

(In reply to comment #1)
> I think the interface should still be accessible from the search bar. Instead of
> "Add Engines..." it could be "Manage Engines..." which would pop up the same
> part of the Options GUI you described.
> 
> Also, engines should generally be stored in the profile, but still allowing
> search plugins in the browser root allows for cross-profile search plugins. It
> would make sense to keep the default Google search in the browser root and let
> users add new searches to the profile. It would be nice if regular plugins were
> handled this way too, but that's another bug.

(In reply to comment #1)
> I think the interface should still be accessible from the search bar. Instead of
> "Add Engines..." it could be "Manage Engines..." which would pop up the same
> part of the Options GUI you described.
> 
> Also, engines should generally be stored in the profile, but still allowing
> search plugins in the browser root allows for cross-profile search plugins. It
> would make sense to keep the default Google search in the browser root and let
> users add new searches to the profile. It would be nice if regular plugins were
> handled this way too, but that's another bug.

I totally agree there should be engine management.  There search bar should be
able to have subfolders (like Bookmarks), several arrangement methods, and
right-click Properties (including 'Name') access.

Comment 20

[andrew.ferrier]

Hmm, I think the last proposal is perhaps going a little far. I don't see the
need for subfolders, or any method of sorting than by name, for a default Firefox
install (although the ability to remove search engines is important). I think
perhaps some of these would be better provided by extensions, in the interests
of avoiding UI and code bloat.

Comment 21

[rvalles]

(In reply to comment #20)
> Hmm, I think the last proposal is perhaps going a little far. I don't see the
> need for subfolders, or any method of sorting than by name, for a default Firefox
> install (although the ability to remove search engines is important). I think
> perhaps some of these would be better provided by extensions, in the interests
> of avoiding UI and code bloat.
I agree about folders, but something must be there for the user to be able to
sort the engines the way that pleases him. He may prefer to have most important
ones in the first entries of the list and unused ones far away.

Comment 22

[Oleg Rekutin]

(In reply to comment #21)
> I agree about folders, but something must be there for the user to be able to
> sort the engines the way that pleases him. He may prefer to have most important
> ones in the first entries of the list and unused ones far away.

IMO, it should be a simple extension of what there is now:

 - add right-click menu with a delete and rename option
 - allow the user to drag-and-drop the engines within the drop-down, the same
way that you can drag-and-drop bookmarks. This will enable re-ordering of the
engines

All I really care about is right-click to delete.

Comment 23

[David Janes]

Comment 22 seems the most sensible option - any other functionality should be
the realm of extenstions (at least for now).

Comment 24

[andrew.ferrier]

Any possibility of having this for FF 1.1?

Comment 25

[Craig Lussier]

It would be convenient if the search bar and "Quick searches" (keywords in
bookmarks) could be merged.  This would negate the need for search engine
management, as the engines would simply be bookmarks.  One idea would be to have
several other options (like "keyword") in a bookmark including:

1) "Use as search engine" on/off - Becomes one of the items in the search bar
2) "Use in <selected text> context menu" on/off - Shows up when right clicking
on selected text
3) "Use in page/link context menu" on/off - shows up when right clicking the
page or a link and uses the respective URL for the search. This would allow
things like 'show google cache of page' to be added as a context menu item.

As additional benefit (at least for me) would be that I wouldn't need a lot of
separate extensions I use now (like context menu dictionary lookup) and adding
the functionality to another computer would simply involve moving my bookmarks,
not installing a bunch of extensions a second/third/fourth... time.

Just my 2 cents

Comment 26

[Chris Blore]

(In reply to comment #24)
> Any possibility of having this for FF 1.1?

Yes I'd think so as it may not be too difficult to implement, depending on what
we decide here and it would seem logical, given as 1.1 is supposed to be for UI
changes and other more 'minor' features.

Comment 27

[[e-mail]]

I'd rather have it merged with the find bar instead of bookmarks: with a bit of
UI that would allow to easily search in the result page for the same search
terms that were submitted to the web site. 

Comment 28

[Gregor Rosenauer]

*** Bug 271207 has been marked as a duplicate of this bug. ***

Comment 29

[choi9999]

> I totally agree there should be engine management.  There search bar should be
> able to have subfolders (like Bookmarks), several arrangement methods, and
> right-click Properties (including 'Name') access.

With due respect to comments #20 and #21, I think the search bar should be able
to have folders. I currently have more than 30 search plugins for my search bar
-- I may be unusual in that regard (I'm a journalist for a living), but still,
it would be nice to be able to categorize them properly. Better to have the
capability and not need it than to need it and not have it.

Comment 30

[patrickneil]

Folders.  The only thing I can think of to better Firefox is folders in the
search bar.  After that comes organization, which is also good.

Comment 31

[Bill Mason]

*** Bug 275209 has been marked as a duplicate of this bug. ***

Comment 32

[Phil Ringnalda (:philor)]

*** Bug 275327 has been marked as a duplicate of this bug. ***

Comment 33

[Joe Llywelyn Griffith Blakesley]

I meant to submit this as a bug over a year ago and forgot.  I just submitted it
as bug 275327 (after extensive searching honestly) then found it had already
been submitted.  THe lack of deletion part of this bug is a major, obvious,
easily fixable bug that affects anyone who uses search.  A proper SE management
GUI would be nice, but that is more of a long-term priority.

Can we just have make sure that for 1.1 the basic functionality is included:
pressing [shift]+[del] while an engine on the drop-down list is selected should
result in that engine being deleted (in the same way that a
searched for phrase can be deleted).

Bug 275327 ("RFE: ability to delete search engines) could be used for the simple
delete thing if this bug is too cluttered.

(Also, possibly allow the use of the [del] key on its own as many users won't
think of using [shift]+[del].  I think, if we do that, there has to be a
confirmation dialog box though (as it is too easy to hit a single key) and I
would still allow [shift]+[del] to quickly delete.)

This is very important because (as I noted in bug 266227) many SEs are now
included by default which many people wouldn't ever use (e.g.: the top two are
exactly the same SE and two of them are for shopping sites) and it is very
difficult for users currently to replace them with SEs
that are more appropriate to themselves.

(BTW, is their a bug to discuss which SEs should be included by default?)

Comment 34

[David Janes]

I agree with Comment #33, but maybe a right-click menu would be neater (and more
visible) than [shift]+[del]? A keyboard shortcut could also be included for
accessibility.

Comment 35

[Joe Llywelyn Griffith Blakesley]

Quoting Comment #34:

>>maybe a right-click menu would be neater (and more visible) than [shift]+[del]<<

I did not suggest this because, apparently, (secondary-click) context menus
coming off drop-down menus is considered to be against people's expectations of
WIMP GUIs.

I happen to disagree but I asked for this on the back/forward buttons and it was
WONTFIXed.  For one thing the menu bar at the top of windows in most WIMP
programs can have context menus coming off it.  Why are other drop-down menus
not allowed this when the most commonly used drop-down menu is?

If the developers no longer have a hang-up about this, a context menu with
"disable", "remove", "move up" and such like would be a good way of dealing with
many of the SE management problems.

A full dialog-box-based UI would still be nice but would eventually supplement
this basic behaviour and would be needed for, say, adding or enabling engines
without downloading them from a webpage or easier re-organisation.  This would
be similar to the bookmarks context menu and manager.  In fact the SE toolbar
could just be a folder within ones bookmarks to which one could manually add SEs
using bookmarklets or replacement strings. The SE management functionality could
just be added to bookmark manager.

(BTW, is there a bring back the find "SE" to the toolbar RFE as this *is* useful
when one is searching for something on a page?  And/or, we could maybe have an
easy way to copy the phrase from the SE toolbar to the find toolbar.)

Comment 36

[David Janes]

(In reply to comment #35)
> I did not suggest this because, apparently, (secondary-click) context menus
> coming off drop-down menus is considered to be against people's expectations of
> WIMP GUIs.
> 
> I happen to disagree but I asked for this on the back/forward buttons and it was
> WONTFIXed.  For one thing the menu bar at the top of windows in most WIMP
> programs can have context menus coming off it.  Why are other drop-down menus
> not allowed this when the most commonly used drop-down menu is?

This is _very_ inconsistent - the Bookmarks menu in Firefox has a context menu,
therefore why can't the Search Engines drop down?

Comment 37

[Joe Llywelyn Griffith Blakesley]

>>This is _very_ inconsistent<<

What is very inconsistent?  I'm arguing for comment #34 .

Comment 38

[PikeUK]

*** Bug 285114 has been marked as a duplicate of this bug. ***

Comment 39

[Frank]

Sorry for the (maybe) stupid question, but what does "Status Whiteboard [asaP1]"
mean?

Comment 40

[José Jeria]

*** Bug 293999 has been marked as a duplicate of this bug. ***

Comment 41

[Adam Hauner]

Duped bug 293999 describes dataloss problem with upgrade from 1.0.3 to 1.0.4
(user's selection of search modules was lost after upgrade), so I'm adding
keyword and raise severity.

Comment 42

[bas rokers]

Another possible way of managing search engine
extensions in the toolbar is by implementing a 'right-click' menu, similar to
the 'right-click' menu used for bookmarks.

Comment 43

[Steven Ratliff]

This seems more like a "BUG OF OMISSION" than of commission. There isn't any
means of management as pointed out.So amoung many things that may eventually be
included I'd like to add a few more to the wish list:
   1.) A "SORT" function (with options for Global Sort or by Group/Cataglory or
               
       Seperator. 
   2.) SEPERATORS as also currently provided in "Bookmarks" for the browser.
       Do you think people who don't see well would like COLORED SEPERATORS?

In short, Management with the same features as in Bookmarks.

Comment 44

[Chris Ilias [:cilias]]

FWIW, there's currently an extension, that adds a context menu to the drop-down
list, with an option to delete the search plugin. Maybe the author can be
contacted about checking the code into Firefox.
http://www.extensionsmirror.nl/index.php?showtopic=3614

Also, maybe this bug should be split into 'sub-bugs'
- delete search plugin
- re-order list
- separators

If this bug is just about remove a search plugin, the summary should be changed.

I don't see much point in reordering the list, "manage engines", etc. A delete
function is all that is needed.

Comment 45

[Jennifer]

(In reply to comment #44)

> I don't see much point in reordering the list, "manage engines", etc. A delete
> function is all that is needed.

You find reinstalling all your search engines each time you upgrade fun? On Mac,
at least, I have to manually extract the searchplugins folder, upgrade, move the
folder to the new copy of Firefox, and hope the copy takes. I have nearly 20
search plugins. I shouldn't have to reinstall them every time -- my bookmarks
transfer, my extensions transfer, my passwords transfer, and my cookies
transfer. Why can't my search engines transfer, too? It is astounding that this
issue still remains unfixed, especially since the report is now over a full year
old and was reported while the browser was still called Firebird and we're now
at 1.0.6 and all other major features migrate with no trouble.

Comment 46

[Chris Ilias [:cilias]]

(In reply to comment #45)
> You find reinstalling all your search engines each time you upgrade fun? On Mac,
> at least, I have to manually extract the searchplugins folder, upgrade, move the
> folder to the new copy of Firefox, and hope the copy takes.

Using the trunk, search plugins are kept in the profile folder (on Windows here)
as per bug 123315 . I don't know about Mac.

But this is what I mean about splitting this up into sub-bugs.

Comment 47

[Thorsten 'nightrat' Fritz]

Maybe you want to have a look at the extension "Search Engine Ordering". It has
a UI to rearrange and delete the search engines.

http://maltekraus.ma.funpic.de/Firefox/Search-Engine-Ordering/index.php?lang=en

Maybe it could be ported into the code of Firefox.

Comment 48

[Calvin Huang]

I've managed to organize/sort my search plugins by putting them in
subdirectories in the "C:\Program Files\Mozilla Firefox\searchplugins" folder.
This may be useful for a lot of people as a temporary solution without using the
SEO plugin:

I wanted certain search engines on top(google, wikipedia, dictionary.com, php
doc, mysql search, imdb, etc.), and certain more seldomly used engines at the
bottom(ebay, pricewatch, hyperdictionary, ticketmaster, etc.), so I tried using
the windows file system to sort the plugins alphabetically, by date, etc. in the
default 'searchplugins' directory. This seems to work after you restart Firefox,
so I wondered, couldn't I just create subdirectories in "C:\Program
Files\Mozilla Firefox\searchplugins" and sort them this way?

Turns out, Firefox does look for search plugins in subdirectories in the order
that the file system has been configured to list them. So I just created
different tiered categories for different sets of plugins and put them in
alpha-numeratized directories like so:

C:\Program Files\Mozilla Firefox\searchplugins\1\1 - Google\
C:\Program Files\Mozilla Firefox\searchplugins\1\2 - Wikipedia\
C:\Program Files\Mozilla Firefox\searchplugins\1\3 - Dictionary.com\
C:\Program Files\Mozilla Firefox\searchplugins\1\4 - Thesaurus.com\
C:\Program Files\Mozilla Firefox\searchplugins\1\5 - Everything2\
C:\Program Files\Mozilla Firefox\searchplugins\2\1 - Answers.com\
C:\Program Files\Mozilla Firefox\searchplugins\2\2 - IMDB\
C:\Program Files\Mozilla Firefox\searchplugins\2\3 - CDDB\
C:\Program Files\Mozilla Firefox\searchplugins\2\4 - PHP\
C:\Program Files\Mozilla Firefox\searchplugins\2\5 - MySQL\
etc.

However, I've recently noticed that when Firefox updates the search plugins I've
installed, it just automatically installs them to 'C:\Program Files\Mozilla
Firefox\searchplugins\*', thus creating duplicate entries of the search plugins
I've already organized. This is very frustrating so I hope that in future
versions Firefox can just replace the existing plugins in the subdirectory
they're installed to.

Lastly, the SEO plugin seems like a very useful tool, but it doesn't come
standard with Firefox, and I'm sure a lot of people haven't heard about it.
Maybe something like it could be built into future versions of Firefox as this
is a desperately needed feature, and the subdirectories approach doesn't seem to
work well with Firefox's auto-update component.

Comment 49

[Jo Hermans]

*** Bug 253891 has been marked as a duplicate of this bug. ***

Comment 50

[Jo Hermans]

*** Bug 255543 has been marked as a duplicate of this bug. ***

Comment 51

[Marius Andreiana]

Note that I just tried Firefox 1.5 beta1 on Linux, and the Del key doesn't work
to remove searchplugins (I restarted browser too). Was this functionality
removed or it's a bug?

Comment 52

[Gids Goldberg]

Marius: When this bug is fixed it will be marked FIXED, until then don't expect 
this functionality in Firefox.

Comment 53

[Peter Lairo]

From my suggestion in related bug 205011c#89:

I think the search engines and options should be selectable via UI (like Thunderbird's newsgroup "Subscribe" UI), and not force the user to visit some (confusing) website. It would all be *right there* in "context".

+---------------+
|[G] mozilla    |
+---------------+
| Google        |
| Yahoo         |
| ...           |
|---------------|
| Options...    | <--- Click this, to get "Search Bar Options"
+---------------+

+- Search Bar Options -------------------------------+
| _//Add Search Engines\\_/Change Order\_/Settings\_ |
||+-------------------------+                       ||
||| A9               [ ]  /\|     [     Add      ]  ||
||| Acronym Finder   [x]  |||     [    Remove    ]  ||
||| Google DE        [ ]  |||     [ Refresh List ]  ||
||| Leo Eng<->Ger    [x]  |||     [     Stop     ]  ||
||| Thesaurus        [ ]  \/|                       ||
||+-------------------------+   _Search Homepage_   ||
|+--------------------------------------------------+|
|                                 [[ OK ]]  [Cancel] |
+----------------------------------------------------+

+- Search Bar Options -------------------------------+
| _/Add Search Engines\_//Change Order\\_/Settings\_ |
||+-------------------------+                       ||
||| A9                    /\|    [   Move Up   ]    ||
||| Acronym Finder        |||    [  Move Down  ]    ||
||| Google DE             |||    [ Alphabetical]    ||
||| Leo Eng<->Ger         |||                       ||
||| Thesaurus             \/|                       ||
||+-------------------------+                       ||
|+--------------------------------------------------+|
|                                 [[ OK ]]  [Cancel] |
+----------------------------------------------------+

+- Search Bar Options -------------------------------+
| _/Add Search Engines\_/Change Order\_//Settings\\_ |
||                                                  ||
|| Open search reslts in:                           ||
||  (o) Currently open tab/window                   || <-- Default!?
||  ( ) New tab                                     ||
||  ( ) New window                                  ||
||                                                  ||
|| Size of Search Bar: [    ] [pixels\/]  [Default] || <-- pixels, %, em ?
||                                                  ||
|+--------------------------------------------------+|
|                                 [[ OK ]]  [Cancel] |
+----------------------------------------------------+

What do you think?

Comment 54

[Ian Macfarlane]

I think it can all be done in one window.

Base it on the first one, except remove 'Refresh list' - the UI should do refresh itself right away, this is just UI clutter. Also remove 'Stop' - what on earth should a user expect that button to do? It's completely unescessary.

From the second page - we need 'move up' and 'move down', but we don't need/want alphabetical ordering.

From the third page - the opening in new window/tab etc options are nice but probably UI clutter. If we want those, we could have an 'advanced' button that changes them. I'm not convinced that they are needed though. We do want the size-changing (more on this next).

With the size-changing, there's no way we'd want users to enter a size in pixels, how is your mom going to understand that? There's two things I can think of.. either an option list with small, medium, large, extra-large, or a slider to change the size. We could potentially have the slider use the same small, medium etc terminology.

Anyway, there's no way that you'll get the UI guys to approve that three-tabbed suggestion you've got there. That's half as many tabs as the entire Firefox preferences section. One window, or POSSIBLY a second advanced tab (and only if we really, really see the need for it, which I doubt) would be accepted. GUI clutter is not acceptable in Firefox.

Comment 55

[Peter Lairo]

(In reply to comment #54)
> remove 'Refresh list' - the UI should do refresh itself right away

There are 1000+ search plugins. Immediate refreshing would be a nightmare, at both ends (client & server).

> Also remove 'Stop' - what on earth should a user expect that button to do? 

See above. Do you understand the way Thunderbird's newsgroup & IMAP *Subscriptions* are for? There's a reason; and it's the same reason as here.

> we don't need/want alphabetical ordering.

I think it would be useful to *quickly* undo a mess. (Agreed: optional)

BTW: Who is "we"? Are you an official representative? Are you "royal"? ;-)

> the opening in new window/tab etc options are nice but probably UI clutter. 

I've been annoyed by my current page being overwritten. I (we) would like it.

> 'advanced' button that changes them. 

How do you suggest to incorporate both reordering *and* options into one tab?

> With the size-changing, there's no way we'd want users to enter a size in
> pixels, how is your mom going to understand that?

She would never even think to change the size. :-P Users would see the default setting and easily deduct a proportional increase/decrease amount (hence the "Default" button to get out of trouble). It requires no knowledge, only cognition.

> an option list with small, medium, large, extra-large, or a slider

"We" think that would be OK too.

> That's half as many tabs as the entire Firefox preferences section. 

...and 316% more than in the File menu. So what? The user should see (only) those options that he needs, in *context*. UI clutter is, IMO, when users see more options than they need within the context they are in. Here, they are only in the search bar context, and per default see only the most-likely used and non-cluttered add/remove search engines tab. The criteria should be "clutter in context", not the sum of all UI, visible or not. All the user would ever see is the one menuitem "Options" in the search bar dropdown. That's no more than he sees now (and wouldn't go to a truly cluttered webpage).

> GUI clutter is not acceptable in Firefox.

Obviously. The real question is: What is clutter? Is *this* clutter?

Comment 56

[Peter van der Woude [:Peter6]]

(In reply to comment #55)
> Obviously. The real question is: What is clutter? Is *this* clutter?
Imagine you are Joe User who is confronted with this UI.
I'm sure it would scare you.
We (the users) wouldn't want that, would we (the users) ?

Comment 57

[Peter Lairo]

Since there are so many search engines (1,000+), there would need to be a "Search" feature (like Thunderbird's "Search Bar"):

+- Search Bar Options -------------------------------+
| _//Add Search Engines\\_/Change Order\_/Settings\_ |
|| Search for: [            ]                       || <-- add search/filter box
||+-------------------------+                       ||
||| A9               [ ]  /\|     [     Add      ]  ||
||| Acronym Finder   [x]  |||     [    Remove    ]  ||
||| Google DE        [ ]  |||     [ Refresh List ]  ||
||| Leo Eng<->Ger    [x]  |||     [     Stop     ]  ||
||| Thesaurus        [ ]  \/|                       ||
||+-------------------------+   _Search Homepage_   ||
|+--------------------------------------------------+|
|                                 [[ OK ]]  [Cancel] |
+----------------------------------------------------+

So entering "trans" would show only entries that contained "trans" (like tranlate de-en, tranlate de-fr).

The "Refresh List" and "Stop" button could be eliminated, if the list refreshes itself every time the user selects: Search bar menu / Options. But I fear that might be annoying if users go into there repeatedly, as there would always be a long delay with no/reduced UI response while it's updating. Also, the aforementioned server hits.

Comment 58

[Ng Ming Hong]

(In reply to comment #55)
> (In reply to comment #54)
> > the opening in new window/tab etc options are nice but probably UI clutter. 
> 
> I've been annoyed by my current page being overwritten. I (we) would like it.
> 

Just Alt + Enter instead of Enter. The same applies to location bar.

Comment 59

[Peter Lairo]

BTW: (to answer your question) I find the suggested UI *much* better than anything suggested so far (which is nothing), and *much* better than the current method (via truly cluttered webpage). Oh, and completely non-scary. What's scary about an existing list of items that you put checkmarks next to? It's exactly what the user wnted.

Comment 60

[Ian Macfarlane]

> There are 1000+ search plugins. Immediate refreshing would be a nightmare, at
both ends (client & server).

Oh so this is for updating them from the server rather than to make the changes to list you see refreshed. Well, ok add 'confusing' and 'badly' named to my list of complaints about it. Plus we shouldn't have to worry about updating search engines manually really, and if we do, the update checking UI should be integrated with the UI for all the other update stuff (possibly openable from the search engine UI however).

>> we don't need/want alphabetical ordering.
>I think it would be useful to *quickly* undo a mess. (Agreed: optional)

Anything 'optional' isn't going to make it past the GUI designers. I'd be happy with a hidden pref.

>BTW: Who is "we"? Are you an official representative? Are you "royal"? ;-)

No, but I'm from the UK, perhaps I speak the Queen's English? ;)

>> the opening in new window/tab etc options are nice but probably UI clutter. 
>I've been annoyed by my current page being overwritten. I (we) would like it.

I've been annoyed by this too, but I'm 50/50 as to whether it's worthwhile to have a UI for it. Don't forget that alt+enter opens it in a new tab.
Perhaps we could by default synch this to the options in Options>Tabs

>> 'advanced' button that changes them. 
>How do you suggest to incorporate both reordering *and* options into one tab?

Reordering is easy, just move up/down buttons which are quite standard. Advanced options could be another tab or a button, but as you can tell I wouldn't be very happy with either, as they're both fairly ugly.

>> With the size-changing, there's no way we'd want users to enter a size in
>> pixels, how is your mom going to understand that?
>She would never even think to change the size. :-P Users would see the default
>setting and easily deduct a proportional increase/decrease amount (hence the
>"Default" button to get out of trouble). It requires no knowledge, only
>cognition.

Personally, I don't believe the resizing belongs here anyway, I think (as I've mentioned in bug 205011) there should simply be a resizer added automatically between resizable entries (like location bar and find bar).

>> That's half as many tabs as the entire Firefox preferences section.
>...and 316% more than in the File menu. So what? The user should see (only)
>those options that he needs, in *context*. UI clutter is, IMO, when users see
>more options than they need within the context they are in. Here, they are only
>in the search bar context, and per default see only the most-likely used and
>non-cluttered add/remove search engines tab. The criteria should be "clutter in
>context", not the sum of all UI, visible or not. All the user would ever see is
>the one menuitem "Options" in the search bar dropdown. That's no more than he
>sees now (and wouldn't go to a truly cluttered webpage).
>> GUI clutter is not acceptable in Firefox.
>Obviously. The real question is: What is clutter? Is *this* clutter?

I'm afraid it is, and rather bad clutter at that. To me, it looks like a design by comittee, of trying to put everything in there that a user could possibly want to do. That UI design belongs in Seamonkey, but there is 0% chance that you'll get it past the people who have the final say on the UI in Firefox.


Someone should change severity to 'enhancement', version to 'trunk' and target milestone to 'future' (though I feel this is a must-have for Firefox 2). Also I'm dubious that bug 293999 is a dup of this bug, surely it's a dup of bug 123315 instead, in which case we should remove the dataloss keyword from this bug also.

Comment 61

[Peter van der Woude [:Peter6]]

How possible/impossible would it be to simply use the Extension Manager window for this (calling it Search Engine manager offcourse) ?
The devs agreed that the current Opions UI sux and has to be changed (again), so adding something similar , as your current proposal, will never happen.
(ps: talk to Mike Beltzner if you want to know more about it.)

Comment 62

[Patrik Spieß]

I liked the idea of Peter very much. However, I also think 3 tabs is too much. 
How about this window? (Also reachable via "Options..." below search plugin list as a dropdown from search field).

+- Search Bar Options -------------------------------------+
|                                                          |
|  +-Plugin Repository-----+    +- Installed plugins ---+  |
|  | + Translations      /\|    | Google              /\|  |
|  | - Dictionaries      |||    | Dictionaries        |||  |
|  |   + Britannica      |||    | Britannica          |||  |
|  |   + Merriam-webster |||    | Merriam-webster     |||  |
|  | + ABC               |||    | ABC                 |||  |
|  | ...                 \/|    |                     \/|  |
|  +-----------------------+    +-----------------------+  |
|             [ Install => ]    [Remove] [Up] [Down] [Sort]|
|                                                          |
|                                            [Advanced]    |
+----------------------------------------------------------+
|                                       [[ OK ]]  [Cancel] |
+----------------------------------------------------------+

On the left: Tree selection with hierarchical view of all serach plugins (categories as already on the site). You would only need to refresh the category that the user expands (delay too long? => prefetch of 1 level?).

On the right: Regular list of installed plugins.

Both lists could be multi-select lists to install/delete more than one plugin at once. Up/down makes no sense and should be greyed out if more than one entries are selected. all buttons should be disabled if nothing is selected.

Consider [sort] and [advanced] as optional.

Opinions?

Comment 63

[Stefan Breunig]

In believe that even the "reduced" variant would be quite complicated. Using those add/remove, up/down buttons is so damn annoying for the advanced users.
So, to solve this it should be possible to use drag & drop reordering, and that could be done as well in the select dialog directly (http://maltekraus.de/Firefox/Search-Engine-Ordering/drag-drop.png).

I wouldn't try to add all possible search engines to the available ones, I'd say: Just keep the ones that are currently included by default anyway, and allow adding them similar to "Add a Keyword for this Search". (Maybe have some hand selected ones in a popup dialog on "add engines" and explain how adding not listed ones works). This would be much more intuitive than searching a list of about 2000 plugins. As advanced user I rather tend to find out a search query myself and create it manually rather than searching that endless list.

Like "inline drag & drop" reordering removing could be done via hovering an item and pressing the DEL key. It isn't obvious, but better than adding a bloat dialog.

Comment 64

[Worcester12345]

(In reply to comment #61)
> How possible/impossible would it be to simply use the Extension Manager window
> for this (calling it Search Engine manager offcourse) ?

Makes sense. That way, the same existing code can be reused again and again. Of course, it would be nice if THAT code allowed sorting by column, for example.

;-)

Comment 65

[Mike Connor [:mconnor]]

These are now stored the in the profile.  Apparently Ben has some search bits he's already working on.

Comment 66

[:Gavin Sharp [email: [e-mail]]]

*** Bug 318010 has been marked as a duplicate of this bug. ***

Comment 67

[Adam Guthrie]

*** Bug 325673 has been marked as a duplicate of this bug. ***

Comment 68

[:Gavin Sharp [email: [e-mail]]]

Attachment: search engine manager, v1

This is the base work for the new engine manager dialog. It's not 100% complete, but I want some initial feedback on it before I continue. Supports reordering, removing, editing of aliases. Some known issues:

- the alias is unused - setting it has no effect
- extension installed search plugins need to be handled better (disabling and then reenabling an extension that provides a removed plugin causes the plugin to reappear)
- drag and drop should be implemented in the treeview
- removing all engines leads to non-optimal UI behavior
- missing icon padding in the tree view

And undoubtedly other bugs too. I'd love it if people could give this patch a spin and see if they can shake any bugs out of it. I will try to post linux and windows test builds tomorrow that include this patch, for people who want to test it out.

Comment 69

[:Gavin Sharp [email: [e-mail]]]

I've posted test Windows and Linux builds that include this patch at http://gavinsharp.com/ff/ .

Comment 70

[Frank]

Just tested it and it looks nice. I like the context menu integration.

- I assume the alias will be used as display name. If not, a rename is missing.

- Found one problem. Reordering doesn't always work when more then one window is opened.

- Will there be seperators and/or sub menus?

- Will the results be opened in a new tab when started from the search bar, just like the context menu search?

Comment 71

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #70)
> - I assume the alias will be used as display name. If not, a rename is missing.

No, the alias is meant to be used as a "keyword" in the location bar, much like bookmark keywords today. The unique identifier for a search engine is currently it's display name, so having a "rename" is not trivial. This can (and hopefully will) change in the future, but won't be part of the initial implementation.

> - Found one problem. Reordering doesn't always work when more then one window
> is opened.

I did test this case - can you consistently reproduce the issue? Some steps to reproduce would be useful, but I'll look into it and see if I see anything.

> - Will there be seperators and/or sub menus?

Possibly, but I don't see this as something that would be useful to most people.

> - Will the results be opened in a new tab when started from the search bar,
> just like the context menu search?

There is a pref for always openeing the results in a new tab (browser.search.openintab), or you can use Alt+Enter to open a specific search in a new tab. This patch doesn't affect this behavior.

Comment 72

[Malte Kraus]

Are there plans to add drag & drop just as with the bookmarks? I find it much more useful to move an engine just from the menu than having to open a dialog. When using (and writing) the extension 'Search Engine Ordering' I noticed that I used it much more often after I implemented Drag 'n' Drop.

Same thing for a context menu to remove an engine like in the extension 'SearchPluginHacks' and 'Search Engine Ordering'.

Another minor thing: when the dialog is closed, the menu reappears and is hidden shortly after that. Maybe a hidePopup() could be called before the dialog is opened to prevent that flickering?

Comment 73

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #72)
> Are there plans to add drag & drop just as with the bookmarks? I find it much
> more useful to move an engine just from the menu than having to open a dialog.
> When using (and writing) the extension 'Search Engine Ordering' I noticed that
> I used it much more often after I implemented Drag 'n' Drop.

Yes, see comment 68.

> Another minor thing: when the dialog is closed, the menu reappears and is
> hidden shortly after that. Maybe a hidePopup() could be called before the
> dialog is opened to prevent that flickering?

That's an issue with the modal dialog - a workaround for that is indeed another bug that needs fixing.

Comment 74

[Frank]

(In reply to comment #71)
> (In reply to comment #70)
> > - Found one problem. Reordering doesn't always work when more then one window
> > is opened.
> 
> I did test this case - can you consistently reproduce the issue? Some steps to
> reproduce would be useful, but I'll look into it and see if I see anything.

Strange, when I first time opened your version it went wrong quite often, but once I restarted "Bon Echo" it didn't happen anymore.

> There is a pref for always openeing the results in a new tab
> (browser.search.openintab), or you can use Alt+Enter to open a specific search
> in a new tab. This patch doesn't affect this behavior.

Well, one would at least expect them to behave similar: both in same window of both in new tab. Anyway, found that it is bug 227710.

Comment 75

[Frank]

After adding a new search engine I had a strange refresh problem. I will attach a screendump. The top window is opened by the underlining manage dailog. I added a search engine from mycroft and after that the dropdown list from the underlying window opened and didn't go away until I clicked on it.

Comment 76

[Frank]

Attachment: Refresh problem after adding, see comment 75

Comment 77

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #76)
> Created an attachment (id=219141) [edit]
> Refresh problem after adding, see comment 75

Thanks Frank, I'd noticed that too, and I've fixed this locally. It's a pretty minor change, so I'll wait until there are more changes before posting an updated patch and new test builds.

Comment 78

[Mike Connor [:mconnor]]

Comment on attachment 219110 [details] [diff] [review]
search engine manager, v1

       var foundStart = false;
       var startLine, numberOfLines;
       // Find the beginning and end of the section
-      for (var i=0; i<lines.length; i++) {
+      for (var i = 0; i<lines.length; i++) {

I don't get why you fixed spacing around one operator, but not the other :P

       var section = {};
-      for (var i=0; i<lines.length; i++) {
+      for (var i = 0; i<lines.length; i++) {

same as previous comment!

       // NO                 NON-EMPTY   &           <name>=<value> TEMPLATE?<n1>=<v1>&<n2>=<v2>
-      for (var i=0; i<inputs.length; i++) {
+      for (var i = 0; i<inputs.length; i++) {

and again?  (just fix all of these, and I'll stop yelling at you)


+    // Now that all engines are loaded, build the sorted engine list
+    this._buildSortedEngineList();
+
+    // Remove prefs for non-existent engines
+    this._clearOldPrefs();

Can this happen after we select the engine?  or even async to startup?  I don't know how expensive this is or if its worth it, but doing cleanup that doesn't affect functionality shouldn't happen as part of startup

+  _saveSortedEngineList: function SRCH_SVC_saveSortedEngineList() {
<snip>
+    // Save the pref file explicitly, since we're called at XPCOM shutdown
+    preferences.savePrefFile(null);
+  },

discussed this on IRC, prefs are an implementation detail right now, so this is ok for a2, but need to clean this up when we switch away from prefs

+  _buildSortedEngineList: function SRCH_SVC_buildSortedEngineList() {

+    // Array for the remaining engines, alphabetically sorted

the alpha engines thing shouldn't really happen in most cases, I'd think that we would add things to the sort order when they're being added, or detected in loadEngines.  If we want to allow alpha sorting, expose that in the management interface

 

       notifyAction(engineToRemove, SEARCH_ENGINE_REMOVED);
     }
   },
 
+  moveEngine: function SRCH_SVC_moveEngine(aEngine, aDir) {
+    ENSURE_ARG(Math.abs(aDir) < this._sortedEngines.length,
+               "Invalid offset passed to moveEngine!");
+    ENSURE_ARG(aEngine instanceof Ci.nsISearchEngine,
+               "Invalid engine passed to moveEngine!");
+
+    var engine = aEngine.wrappedJSObject;
+
+    var index = this._sortedEngines.indexOf(engine);
+    ENSURE(index != -1, "moveEngine: Can't find engine to move!",
+           Cr.NS_ERROR_UNEXPECTED);
+
+    var newIndex = index - aDir;
+    ENSURE_ARG((newIndex < this._sortedEngines.length) && (newIndex >= 0),
+               "Index out of bounds!");
+
+    // Swap the two engines
+    this._sortedEngines[index] = this._sortedEngines[newIndex];
+    this._sortedEngines[newIndex] = engine;
+
+    notifyAction(engine, SEARCH_ENGINE_CHANGED);
+  },

Why does this only move one step at a time?  can we not use splice() here instead?

   get defaultEngine() {
     const defPref = BROWSER_SEARCH_PREF + "defaultenginename";
     // Get the default engine - this pref should always exist, but the engine
     // might be hidden
     this._defaultEngine = this.getEngineByName(getLocalizedPref(defPref, ""));
     if (!this._defaultEngine || this._defaultEngine.hidden)
-      this._defaultEngine = this.getVisibleEngines({})[0] || null;
+      this._defaultEngine = this._getSortedEngines(false)[0] || null;

why change this?  net result is the same?  what's the aCount useful for in getVisibleEngines anyway?


@@ -2029,16 +2139,17 @@ SearchService.prototype = {
         if (aVerb == SEARCH_ENGINE_LOADED) {
           var engine = aEngine.QueryInterface(Ci.nsISearchEngine);
           LOG("nsISearchEngine::observe: Done installation of " + engine.name
               + ".");
           this._addEngineToStore(engine.wrappedJSObject);
         }
         break;
       case XPCOM_SHUTDOWN_TOPIC:
+        this._saveSortedEngineList();

I'd rather save stuff earlier than XPCOM shutdown, can't we do this on quit-application-requested or something a little earlier, aside from the "drop prefs as impl detail" bit.


+    var os = Cc["@mozilla.org/observer-service;1"].
+             getService(Ci.nsIObserverService);
+    os.addObserver(this, "browser-search-engine-modified",
+                   false);

why the linebreak for this?

+  /**
+   * Moves the selected engine either up or down in the engine list
+   * @param aDir
+   *        -1 to move the selected engine down, +1 to move it up.
+   */
+  move: function engineManager_move(aDir) {
+    var selectedEngine = gEngineView.selectedEngine;
+    if (!selectedEngine)
+      return;
+
+    if (Math.abs(aDir) != 1)
+      throw new Error("Invalid direction!");
+
+    var newIndex = gEngineView.selectedIndex - aDir;
+
+    if (newIndex < 0 || newIndex > gEngineView.lastIndex)
+      throw new Error("Invalid aDir!");
+
+    gEngineView._engineStore.moveEngine(selectedEngine, aDir);
+
+    gEngineView.invalidate();
+    gEngineView.selection.select(newIndex);
+  },
+
+  onSelect: function engineManager_onSelect() {
+    var noEngineSelected = (gEngineView.selectedIndex == -1);
+    document.getElementById("remove").disabled = noEngineSelected;
+    // document.getElementById("rename").disabled = noEngineSelected;
+    document.getElementById("alias").disabled  = noEngineSelected;
+
+    document.getElementById("up").disabled = noEngineSelected ||
+                                             (gEngineView.selectedIndex == 0);
+
+    var lastSelected = (gEngineView.selectedIndex == gEngineView.lastIndex);
+    document.getElementById("dn").disabled = noEngineSelected ||
+                                             lastSelected;
+  }
+};

I think this would be nicer if it supported move to top/move to bottom and dnd (i.e. more than one step at a time)

Index: browser/components/search/content/engineManager.xul
===================================================================
RCS file: browser/components/search/content/engineManager.xul
diff -N browser/components/search/content/engineManager.xul
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ browser/components/search/content/engineManager.xul 20 Apr 2006 05:21:43 -0000
@@ -0,0 +1,60 @@
+<?xml version="1.0"?>

no boilerplate!

+<?xml-stylesheet href="chrome://global/skin/"?>
+
+<!DOCTYPE dialog SYSTEM "chrome://browser/locale/engineManager.dtd">
+
+<dialog id="engineManager"
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
+        buttons="accept,cancel"
+        onload="gEngineManagerDialog.init();"
+        ondialogaccept="gEngineManagerDialog.onOK();"
+        title="&engineManager.title;"
+        style="min-width: 40em;"
+        windowtype="Browser:SearchManager">

localize the minwidth!

+  <label>&engineManager.intro;</label>

<description> please, so this wraps nicely if and when the string is more verbose


+  <hbox flex="1">
+    <tree id="engineList" flex="1" rows="10" hidecolumnpicker="true"
+          seltype="single" onselect="gEngineManagerDialog.onSelect();">
+      <treechildren id="engineChildren" flex="1"/>
+      <treecols>
+        <treecol id="engineName" class="sortDirectionIndicator" flex="3"
+                 persist="width" label="&nameColumn.label;"/>
+        <splitter class="tree-splitter"/>
+        <treecol id="engineAlias" class="sortDirectionIndicator" flex="2"
+                 persist="width" label="&aliasColumn.label;"/>
+      </treecols>
+    </tree>
+    <vbox>
+      <spacer flex="1"/>
+      <button id="up" label="&up.label;" accesskey="&up.accesskey;"
+              oncommand="gEngineManagerDialog.move(1);"/>
+      <button id="dn" label="&dn.label;" accesskey="&dn.accesskey;"
+              oncommand="gEngineManagerDialog.move(-1);"/>
+      <spacer flex="1"/>
+      <button id="alias" label="&editAlias.label;"
+              accesskey="&editAlias.accesskey;"
+              oncommand="gEngineManagerDialog.editAlias();"/>
+      <button id="remove" label="&delete.label;"
+              accesskey="&delete.accesskey;"
+              oncommand="gEngineManagerDialog.remove();"/>

These should be commands instead of oncommand, so we can skip sanity checks in the commands in favour of disabling the command when appropriate (also, better/expected UI to do this)

Not sure about the layout, will play with it next iteration



Index: browser/locales/en-US/chrome/browser/searchbar.dtd
===================================================================
RCS file: browser/locales/en-US/chrome/browser/searchbar.dtdpff
diff -N browser/locales/en-US/chrome/browser/searchbar.dtd
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ browser/locales/en-US/chrome/browser/searchbar.dtd  20 Apr 2006 05:21:45 -0000
@@ -0,0 +1,2 @@
+<!ENTITY cmd_engineManager.label        "Manage search engines...">
+<!ENTITY cmd_engineManager.accesskey    "M">

Should be Manage Search Engines


not there yet, but not that far off

Comment 79

[:Gavin Sharp [email: [e-mail]]]

> (In reply to comment #70)
> > - I assume the alias will be used as display name. If not, a rename is missing.
> 
> No, the alias is meant to be used as a "keyword" in the location bar, much like
> bookmark keywords today. The unique identifier for a search engine is currently
> it's display name, so having a "rename" is not trivial. This can (and hopefully
> will) change in the future, but won't be part of the initial implementation.

Filed bug 335102 for that.

Comment 80

[:Gavin Sharp [email: [e-mail]]]

Attachment: search engine manager, v1.1

(In reply to comment #78)
> (From update of attachment 219110 [details] [diff] [review] [edit])
> -      for (var i=0; i<inputs.length; i++) {
> +      for (var i = 0; i<inputs.length; i++) {
> 
> and again?  (just fix all of these, and I'll stop yelling at you)

fixed all of these

> +    // Now that all engines are loaded, build the sorted engine list
> +    this._buildSortedEngineList();
> +
> +    // Remove prefs for non-existent engines
> +    this._clearOldPrefs();
> 
> Can this happen after we select the engine?  or even async to startup?  I don't
> know how expensive this is or if its worth it, but doing cleanup that doesn't
> affect functionality shouldn't happen as part of startup

Actually, I think I'll just remove that method. Having extra prefs around for now won't really hurt, and clearing them causes problems with temporarily disabled extensions that ship search engines. It won't be relevant when we move to non-prefs for storage, which I want to do as soon as this is landed.

> +  _saveSortedEngineList: function SRCH_SVC_saveSortedEngineList() {
> <snip>
> +    // Save the pref file explicitly, since we're called at XPCOM shutdown
> +    preferences.savePrefFile(null);
> +  },
> 
> discussed this on IRC, prefs are an implementation detail right now, so this is
> ok for a2, but need to clean this up when we switch away from prefs
> 
> +  _buildSortedEngineList: function SRCH_SVC_buildSortedEngineList() {
> 
> +    // Array for the remaining engines, alphabetically sorted
> 
> the alpha engines thing shouldn't really happen in most cases, I'd think that
> we would add things to the sort order when they're being added, or detected in
> loadEngines.  If we want to allow alpha sorting, expose that in the management
> interface

This code just maintains the current behavior for unordered engines. Added engines are added to the end of the list. Do you want me to remove this? Guess it doesn't really matter.

> +  moveEngine: function SRCH_SVC_moveEngine(aEngine, aDir) {
> +    ENSURE_ARG(Math.abs(aDir) < this._sortedEngines.length,
> +               "Invalid offset passed to moveEngine!");
> +    ENSURE_ARG(aEngine instanceof Ci.nsISearchEngine,
> +               "Invalid engine passed to moveEngine!");
> +
> +    var engine = aEngine.wrappedJSObject;
> +
> +    var index = this._sortedEngines.indexOf(engine);
> +    ENSURE(index != -1, "moveEngine: Can't find engine to move!",
> +           Cr.NS_ERROR_UNEXPECTED);
> +
> +    var newIndex = index - aDir;
> +    ENSURE_ARG((newIndex < this._sortedEngines.length) && (newIndex >= 0),
> +               "Index out of bounds!");
> +
> +    // Swap the two engines
> +    this._sortedEngines[index] = this._sortedEngines[newIndex];
> +    this._sortedEngines[newIndex] = engine;
> +
> +    notifyAction(engine, SEARCH_ENGINE_CHANGED);
> +  },
> 
> Why does this only move one step at a time?  can we not use splice() here
> instead?

This method isn't limited to one at a time, I just forgot to update the interface docs.

>    get defaultEngine() {
>      const defPref = BROWSER_SEARCH_PREF + "defaultenginename";
>      // Get the default engine - this pref should always exist, but the engine
>      // might be hidden
>      this._defaultEngine = this.getEngineByName(getLocalizedPref(defPref, ""));
>      if (!this._defaultEngine || this._defaultEngine.hidden)
> -      this._defaultEngine = this.getVisibleEngines({})[0] || null;
> +      this._defaultEngine = this._getSortedEngines(false)[0] || null;
> 
> why change this?  net result is the same?  what's the aCount useful for in
> getVisibleEngines anyway?

Just to make it clearer what's actually being called. aCount is required for returning arrays through XPCOM (see method signature in nsIBrowserSearchService).

> @@ -2029,16 +2139,17 @@ SearchService.prototype = {
>          if (aVerb == SEARCH_ENGINE_LOADED) {
>            var engine = aEngine.QueryInterface(Ci.nsISearchEngine);
>            LOG("nsISearchEngine::observe: Done installation of " + engine.name
>                + ".");
>            this._addEngineToStore(engine.wrappedJSObject);
>          }
>          break;
>        case XPCOM_SHUTDOWN_TOPIC:
> +        this._saveSortedEngineList();
> 
> I'd rather save stuff earlier than XPCOM shutdown, can't we do this on
> quit-application-requested or something a little earlier, aside from the "drop
> prefs as impl detail" bit.

I'm going to address that in bug 335101, which I also want to fix as soon as this lands.

> +    var os = Cc["@mozilla.org/observer-service;1"].
> +             getService(Ci.nsIObserverService);
> +    os.addObserver(this, "browser-search-engine-modified",
> +                   false);
> 
> why the linebreak for this?

Because it used to be somewhere else that required it :). fixed that.

> +  /**
> +   * Moves the selected engine either up or down in the engine list
> +   * @param aDir
> +   *        -1 to move the selected engine down, +1 to move it up.
> +   */
> +  move: function engineManager_move(aDir) {
> +    var selectedEngine = gEngineView.selectedEngine;
> +    if (!selectedEngine)
> +      return;
> +
> +    if (Math.abs(aDir) != 1)
> +      throw new Error("Invalid direction!");
> +
> +    var newIndex = gEngineView.selectedIndex - aDir;
> +
> +    if (newIndex < 0 || newIndex > gEngineView.lastIndex)
> +      throw new Error("Invalid aDir!");
> +
> +    gEngineView._engineStore.moveEngine(selectedEngine, aDir);
> +
> +    gEngineView.invalidate();
> +    gEngineView.selection.select(newIndex);
> +  },
> +
> +  onSelect: function engineManager_onSelect() {
> +    var noEngineSelected = (gEngineView.selectedIndex == -1);
> +    document.getElementById("remove").disabled = noEngineSelected;
> +    // document.getElementById("rename").disabled = noEngineSelected;
> +    document.getElementById("alias").disabled  = noEngineSelected;
> +
> +    document.getElementById("up").disabled = noEngineSelected ||
> +                                             (gEngineView.selectedIndex == 0);
> +
> +    var lastSelected = (gEngineView.selectedIndex == gEngineView.lastIndex);
> +    document.getElementById("dn").disabled = noEngineSelected ||
> +                                             lastSelected;
> +  }
> +};
> 
> I think this would be nicer if it supported move to top/move to bottom and dnd
> (i.e. more than one step at a time)

Filed bug 335086 for drag&drop, and created moveToIndex on the dialog. Do you think the dialog should have top/bottom buttons?

> > Index: browser/components/search/content/engineManager.xul
> no boilerplate!

fixed

> +<?xml-stylesheet href="chrome://global/skin/"?>
> +
> +<!DOCTYPE dialog SYSTEM "chrome://browser/locale/engineManager.dtd">
> +
> +<dialog id="engineManager"
> +        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +        buttons="accept,cancel"
> +        onload="gEngineManagerDialog.init();"
> +        ondialogaccept="gEngineManagerDialog.onOK();"
> +        title="&engineManager.title;"
> +        style="min-width: 40em;"
> +        windowtype="Browser:SearchManager">
> 
> localize the minwidth!

fixed.

> +  <label>&engineManager.intro;</label>
> 
> <description> please, so this wraps nicely if and when the string is more
> verbose

fixed

> +  <hbox flex="1">
> +    <tree id="engineList" flex="1" rows="10" hidecolumnpicker="true"
> +          seltype="single" onselect="gEngineManagerDialog.onSelect();">
> +      <treechildren id="engineChildren" flex="1"/>
> +      <treecols>
> +        <treecol id="engineName" class="sortDirectionIndicator" flex="3"
> +                 persist="width" label="&nameColumn.label;"/>
> +        <splitter class="tree-splitter"/>
> +        <treecol id="engineAlias" class="sortDirectionIndicator" flex="2"
> +                 persist="width" label="&aliasColumn.label;"/>
> +      </treecols>
> +    </tree>
> +    <vbox>
> +      <spacer flex="1"/>
> +      <button id="up" label="&up.label;" accesskey="&up.accesskey;"
> +              oncommand="gEngineManagerDialog.move(1);"/>
> +      <button id="dn" label="&dn.label;" accesskey="&dn.accesskey;"
> +              oncommand="gEngineManagerDialog.move(-1);"/>
> +      <spacer flex="1"/>
> +      <button id="alias" label="&editAlias.label;"
> +              accesskey="&editAlias.accesskey;"
> +              oncommand="gEngineManagerDialog.editAlias();"/>
> +      <button id="remove" label="&delete.label;"
> +              accesskey="&delete.accesskey;"
> +              oncommand="gEngineManagerDialog.remove();"/>
> 
> These should be commands instead of oncommand, so we can skip sanity checks in
> the commands in favour of disabling the command when appropriate (also,
> better/expected UI to do this)

Not sure what you mean by "also, better/expected UI to do this". I added <command>s and fixed onSelect accordingly.

> Index: browser/locales/en-US/chrome/browser/searchbar.dtd

> +<!ENTITY cmd_engineManager.label        "Manage search engines...">
> +<!ENTITY cmd_engineManager.accesskey    "M">
> 
> Should be Manage Search Engines

fixed


also fixed:
- made dialog non-resizable
- made dialog position persistent
- fixed focus outline of "Add engines" by adding a spacer
- open the dialog on a timeout to avoid modality weirdness (dropdown opening when making changes)

Let me know if you think any of the dependent bugs (bug 335086, bug 335101, bug 335102) need to be fixed before this lands and I can roll them in to this patch.

Comment 81

[:Gavin Sharp [email: [e-mail]]]

Hmm, Bugzilla's interdiff isn't all that useful. I made http://gavinsharp.com/tmp/232272-v1v2.html if that helps!

Comment 82

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 219466 [details] [diff] [review]
search engine manager, v1.1

Good work, Gavin. Some comments that can be addressed in polish and shine bugs for B1:

- the whole "alias" thing seems really power-userish and confuses what should be a simple UI; I'd recommend just removing it until there are some clear use cases behind it. This should also result in making the dialog narrower.

- s/Add Search Engines .../Get more search engines.../; also, the label should be moved down a little, it looks too tight up against the textbox like that.

- the "Get more search engines" button should close the dialog and load the new page as per the user's new window prefs (ie: either in a new tab or a new window). I've been meaning to file this against the EM as well, fwiw. :)

- s/The following Search Engines are available/You have the following search engines installed:

- I think you have it in another bug, but a "Restore Default Set" button is needed; just wanted to get that on record

- I also notice that you're planning on adding drag and drop support; nice, nice

Comment 83

[:Gavin Sharp [email: [e-mail]]]

Attachment: search engine manager, v1.2

(In reply to comment #82)
> (From update of attachment 219466 [details] [diff] [review] [edit])
> - the whole "alias" thing seems really power-userish and confuses what should
> be a simple UI; I'd recommend just removing it until there are some clear use
> cases behind it. This should also result in making the dialog narrower.

removed the alias stuff (at least for now).

> - s/Add Search Engines .../Get more search engines.../; also, the label should
> be moved down a little, it looks too tight up against the textbox like that.

Made the text change, but I can't really align the link with the buttons, given how the dialog binding works. I'll try and see if I can fix this some other way.

> - the "Get more search engines" button should close the dialog and load the new
> page as per the user's new window prefs (ie: either in a new tab or a new
> window). I've been meaning to file this against the EM as well, fwiw. :)

Fixed that (EM is bug 262575, we need a more generic solution to this problem).

> - s/The following Search Engines are available/You have the following search
> engines installed:

fixed

> - I think you have it in another bug, but a "Restore Default Set" button is
> needed; just wanted to get that on record

Yep, bug 334486.

> - I also notice that you're planning on adding drag and drop support; nice,
> nice

(yep, bug 335086)

Comment 84

[:Gavin Sharp [email: [e-mail]]]

http://gavinsharp.com/tmp/232272-v2v3.html - interdiff between v2 and v3 (UI review comments)
http://gavinsharp.com/tmp/232272-v1v3.html - interdiff between v1 and v3

Comment 85

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 219919 [details] [diff] [review]
search engine manager, v1.2

>Index: browser/components/search/content/engineManager.xul

>+  <script type="application/x-javascript"
>+          src="chrome://browser/content/search/engineManager.js"/>
>+  <stringbundle id="engineManagerBundle"
>+                src="chrome://browser/locale/search.properties"/>

This stringbundle isn't used, I've removed it locally.

Comment 86

[Mike Connor [:mconnor]]

Comment on attachment 219919 [details] [diff] [review]
search engine manager, v1.2

Ok, looks good to go!

Comment 87

[:Gavin Sharp [email: [e-mail]]]

Checked in, 1.8 branch and trunk.

Comment 88

[Robert Strong (they/them - no direct email)]

(In reply to comment #82)
> - the "Get more search engines" button should close the dialog and load the new
> page as per the user's new window prefs (ie: either in a new tab or a new
> window). I've been meaning to file this against the EM as well, fwiw. :)
Bug 262575 will first have to be fixed to do this for the EM since the EM is a toolkit app and the app isn't always a browser. Basically, we have widgets in toolkit that the EM uses (e.g. label class text-link) that don't know how to handle this and toolkit components in general have no way to accomplish this today generically.

Comment 89

[Jonathan Haas]

Currently there is no padding between the text and the icon in the manager. Could this be fixed?

Comment 90

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #89)
> Currently there is no padding between the text and the icon in the manager.
> Could this be fixed?

Yeah, I noticed that earlier, see comment 68. I filed bug 335820 for that, and bug 335822 for improving the UI in the "no engines installed" case.

Comment 91

[Thomas Schwecherl]

Why not add the search engine manager to the new Tools > "Add-ons" menu?

Comment 92

[Peter van der Woude [:Peter6]]

(In reply to comment #91)
> Why not add the search engine manager to the new Tools > "Add-ons" menu?
> 

-> Bug 335781

Comment 93

[Mycroft Project (CC)]

*** Bug 318126 has been marked as a duplicate of this bug. ***