Closed Bug 436709 Opened 12 years ago Closed 12 years ago

Migrate SeaMonkey's Internet Search preferences to new pref window

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a1

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch for pref-search v0.1 (obsolete) — Splinter Review
No description provided.
Attachment #323238 - Flags: superreview?(neil)
Attachment #323238 - Flags: review?(mnyromyr)
Comment on attachment 323238 [details] [diff] [review]
Patch for pref-search v0.1

>+  // build the engine list, we do this by hand instead of using
>+  // the xul template builder because of bug #285076
>+  CreateEngineList();
So, it turns out there's a better workaround than creating the menu by hand: set the ref of the engineList after the overlay has loaded. Note that there's a bug in the internet search service (caused by a performance feature) in that the very first time you set the ref it will only build one menuitem. I can attach the patch that works around that in the search service or you can work around it here by calling engineList.builder.rebuild().

>+  // nothing is selected
>+  if (!engineList.label)
My preference would be for if (!engineList.selectedItem)
I didn't do detailed look, since I assume you'll tackle Neil's comment first. ;-)

But while you're fixing the panel you should also fix the trunk regression (as compared to branch) of not showing the search icon in the closed search engine menulist (it's missing for both Classic and Modern, although Modern shows them in the dropdown, which we usually(?) don't do for Classic)...
Attachment #323238 - Flags: superreview?(neil)
Attachment #323238 - Flags: review?(mnyromyr)
Status: NEW → ASSIGNED
Attached patch Patch for pref-search v0.2 (obsolete) — Splinter Review
Changes since v0.1:
* Went back to using a template and setting ref/rebuilding as suggested by Neil
* Fixed issue with missing icons for search engines - in Gecko 1.9 the attribute for an icon on a menuitem has changed from "src" to "image" - behaviour is back as it was for classic/modern under Gecko 1.8.x
Attachment #323238 - Attachment is obsolete: true
Attachment #325199 - Flags: superreview?(neil)
Attachment #325199 - Flags: review?(mnyromyr)
Comment on attachment 325199 [details] [diff] [review]
Patch for pref-search v0.2

>+  // Need to set ref attribute once overlay has loaded
>+  engineList.setAttribute("ref", "NC:SearchEngineRoot");
>+  // Due to a bug in the internet search service, the first time
>+  // ref is set only one menuitem is built, so force a rebuild
>+  engineList.builder.rebuild();
>+
>   //nothing is selected
>+  if (!engineList.selectedItem)
Whoops, this doesn't quite work - since the menulist starts off empty it has no selected item - you need to try to set it to the preference value (engineList.value's setter will try to set selectedItem, so you don't need to use getElementsByAttribute yourself in this case). sr=me with this fixed.

>     try
>     {
>+      name = document.getElementById("browser.search.defaultenginename").value;
>+      selectedItem = engineList.getElementsByAttribute("label", name).item(0);
>     }
>+    catch(e) {}
I don't think this needs try/catch any more.

>-        //select the first listed search engine
>-        engineList.selectedIndex = 1;
Heh, I can't believe we only just fixed this ;-)

>+      // select a locale-dependent predefined search engine
>+      // in absence of a user default
>+      engineList.selectedItem = selectedItem;
>+    }
>+    else
>+    {
>+      // select the first listed search engine
>+      engineList.selectedIndex = 0;
>     }
> 
>+    var pref = document.getElementById(engineList.getAttribute("preference"));
>+    pref.value = engineList.selectedItem.value;
Setting the selected item/index updates the value, so you just need
pref.value = engineList.value;
Or you could be sneaky and use selectedItem.doCommand(); as that updates both the menulist and the preference ;-)
Attachment #325199 - Flags: superreview?(neil) → superreview+
Attachment #325199 - Flags: review?(mnyromyr)
Changes since v0.2:
* Removed try/catch
* Added attempt to set menulist from preference value

Carrying forward sr= and requesting r=
Attachment #325199 - Attachment is obsolete: true
Attachment #325313 - Flags: superreview+
Attachment #325313 - Flags: review?(mnyromyr)
Comment on attachment 325313 [details] [diff] [review]
Patch for pref-search v0.2a (Checked in)

>Index: pref-search.js
>===================================================================
>+function Startup()
>+{
>+  CreateEngineList();
>+}
...
>+function CreateEngineList()
> {

AFAICT, this function is only called once, so you could just put the code into Startup.

>   //nothing is selected

And spend a space here ;-)

r=me with those.


(My Classic Linux build crops the label of the selected search engine in the menulist, but this may just be either toolkit or my KDE theme.)
Attachment #325313 - Flags: review?(mnyromyr) → review+
Comment on attachment 325313 [details] [diff] [review]
Patch for pref-search v0.2a (Checked in)

Checking into trunk
pref-search.xul
new revision: 1.39; previous revision: 1.38
pref-search.js
new revision: 1.15; previous revision: 1.14
preferences.xul
new revision: 1.20; previous revision: 1.19
preftree.xul
new revision: 1.122; previous revision: 1.121
done
Attachment #325313 - Attachment description: Patch for pref-search v0.2a → Patch for pref-search v0.2a (Checked in)
(In reply to comment #6)
> (From update of attachment 325313 [details] [diff] [review])
> (My Classic Linux build crops the label of the selected search engine in the
> menulist, but this may just be either toolkit or my KDE theme.) 
> 
This is due to display: none css in toolkit themes gnomestripe, winstripe and pmstripe (but not pinstripe):
http://lxr.mozilla.org/seamonkey/source/toolkit/themes/gnomestripe/global/menu.css#179
http://lxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/menu.css#196
http://lxr.mozilla.org/seamonkey/source/toolkit/themes/pmstripe/global/menu.css#189

So would have to be raised as a bug against them (Bug 443516)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.