Closed
Bug 107418
Opened 23 years ago
Closed 22 years ago
about:config todo list
Categories
(SeaMonkey :: Preferences, defect)
SeaMonkey
Preferences
Tracking
(Not tracked)
VERIFIED
FIXED
Future
People
(Reporter: sspitzer, Assigned: sspitzer)
References
(Depends on 1 open bug, )
Details
(Keywords: polish)
Attachments
(4 files, 9 obsolete files)
4.87 KB,
patch
|
Details | Diff | Splinter Review | |
20.89 KB,
text/plain
|
Details | |
24.45 KB,
patch
|
caillon
:
review+
sspitzer
:
superreview+
|
Details | Diff | Splinter Review |
18.02 KB,
image/png
|
Details |
1) get sort indicators to show up 2) support column reordering (lxr on "ordinal") 3) persist the sort type 4) persist the sorted column 5) bonus, restore selection after sorting 6) bonus, ensure selected index is visible after sorting. 7) big bonus, support inline editing. (#17199) I don't think we should waste a lot of time on about:config it's most valuable for debugging, and for sample code on how to implement nsIOutlinerView from js.
Updated•23 years ago
|
Component: Browser-General → Preferences
OS: Windows 2000 → All
QA Contact: doronr → sairuh
Hardware: PC → All
Comment 1•23 years ago
|
||
about:config also used to be color coded in NS4.x.
Updated•23 years ago
|
Depends on: advancedprefs
Comment 2•23 years ago
|
||
Comment 3•23 years ago
|
||
I removed the initial sort of the array, added functions to handle the update of one or all of the sort indicators added to the xul file the rdf to support this Sorry - I should have done this earlier - when Seth made his fix for the sort
Status: NEW → ASSIGNED
Comment 4•23 years ago
|
||
kerz - can you review the code (and give me an r= if appropriate)?
Comment 5•23 years ago
|
||
Seems ok at first glance, but i know not of outliner so you'll want to find someone who does to review it properly.
Updated•23 years ago
|
Attachment #56747 -
Attachment is obsolete: true
Comment 6•23 years ago
|
||
Comment 7•23 years ago
|
||
another item to add: context menus here need cleanup. most items don't do anything, and might not even be relevant. let me know if i should file this as a seperate bug. Back - works Forward - works Reload - works Stop - [actually, it never takes long enough to appear enabled] ---- View Page Source - shows source for window, but no content View Page Info - sparse: info displayed undefined, unknown ---- Bookmark this Page - works ---- Save Page As - saving as an .html page results in the same content seen in view source [saved page appears blank when loaded in browser] ---- Edit Page in Composer - launches blank composer window Send Page - nothing happens ---- Select All - no selection occurs Copy - nothing copied to clipboard [nothing to paste]
Assignee | ||
Comment 8•23 years ago
|
||
taking about:config bugs from chipc.
Assignee: chipc → sspitzer
Status: ASSIGNED → NEW
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Comment 9•22 years ago
|
||
CCing some likely reviewers.
Comment 10•22 years ago
|
||
1) got sort indicators to show up 2) supported column reordering (lxr on "ordinal") 3) persisted the sort type 4) persisted the sorted column 5) restored selection after sorting 6) ensured selected index is visible after sorting. 7) added context menu with four options: a) copy pref name b) copy pref value c) edit pref (if not locked) using prompt dialog d) reset pref (if not locked or default)
Comment 11•22 years ago
|
||
Attachment #82181 -
Attachment is obsolete: true
Updated•22 years ago
|
URL: about:config
Comment 12•22 years ago
|
||
This now makes about:config track pref changes and update its internal array.
Attachment #82191 -
Attachment is obsolete: true
Comment 13•22 years ago
|
||
While you're at it, can you add an "Delete" to the context menu?
Comment 14•22 years ago
|
||
Applied the patch, works well. Was thinking of doing this myself just yesterday... should have figured there was a bug on it. ('Reset' is quite nice!) Only a few small nits: 1) Do we really need the ability to add new prefs? 2) I wasn't able to add a pref without a '.' in it. This may be correct. 3) When prefs were sorted by name, and I added a pref named 'aaa.bbb', it was added at the bottom, under xul prefs, rather then the top. 4) As cbiesinger pointed out, there was no way for me to delete 'aaa.bbb' after creating it. 5) Probably not easy, but just in case it is... when values are changed, or prefs added/removed, might be nice to have undo/redo in the edit menu work. 6) How about edit->copy to copy an entire line (name, status, type, value). 7) Again, if it's easy enough, ability to select multiple prefs at once with ctrl and shift, to copy them would be nice. 8) Can Copy Name and Copy Value also copy the info to the X cutbuffer? The current 'copying' as relatively useless to me.
Comment 15•22 years ago
|
||
You mean PRIMARY, not CUT_BUFFER, right?
Comment 16•22 years ago
|
||
I have no idea what it's called. Just make it so middle click into my mail and IRC apps running in an xterm actually paste. :) There's a bug on JS console to do this too. Currently I go to a bugzilla page, ctrl-v paste into the additional comments text box, select it, THEN middle click my xterm. Fun.
Comment 17•22 years ago
|
||
> Do we really need the ability to add new prefs? It's easier to remove it then add it in later... > I wasn't able to add a pref without a '.' in it. This may be correct. Where there any JS console errors? > When prefs were sorted by name, and I added a pref named 'aaa.bbb', it was added at the bottom, under xul prefs, rather then the top. Well, I didn't want to force a sort, but it could be done. > As cbiesinger pointed out, there was no way for me to delete 'aaa.bbb' after creating it. You'll need to file a bug under preferences back end to fix that. > Probably not easy, but just in case it is... when values are changed, or prefs added/removed, might be nice to have undo/redo in the edit menu work. I'm not sure how easy it is to access the edit menu at all, actually. > How about edit->copy to copy an entire line (name, status, type, value). See above. > Again, if it's easy enough, ability to select multiple prefs at once with ctrl and shift, to copy them would be nice. There's a problem with line delimiters (it also affects the JS console) > Can Copy Name and Copy Value also copy the info to the X cutbuffer? The current 'copying' as relatively useless to me. Sure, Pike told me what the fix is.
Comment 18•22 years ago
|
||
> > As cbiesinger pointed out, there was no way for me to delete 'aaa.bbb' after
> > creating it.
> You'll need to file a bug under preferences back end to fix that.
See clearUserPref()
Comment 19•22 years ago
|
||
> > > As cbiesinger pointed out, there was no way for me to delete 'aaa.bbb'
> > > after creating it.
> > You'll need to file a bug under preferences back end to fix that.
> See clearUserPref()
clear != delete. After using clearUserPref getPrefType still works (and
getPrefValue also works if the pref has a default value).
Comment 20•22 years ago
|
||
The copy functions set both clipboards and new prefs get sorted into place for name or type sort order (it would be too slow for other sort orders).
Attachment #86393 -
Attachment is obsolete: true
Comment 21•22 years ago
|
||
Attachment #90290 -
Attachment is obsolete: true
Comment 22•22 years ago
|
||
Comment on attachment 90338 [details] [diff] [review] Fix bug in previous patch >+ <treechildren id="configTreeBody" ondblclick="Modify();"/> Nit: This should read <treechildren id="configTreeBody" ondblclick="if (event.button == 0) Modify();"/>
Comment 23•22 years ago
|
||
Attachment #90338 -
Attachment is obsolete: true
Comment 25•22 years ago
|
||
Attachment #95384 -
Attachment is obsolete: true
Updated•22 years ago
|
Attachment #107913 -
Flags: review?(glazman)
Updated•22 years ago
|
Attachment #107913 -
Flags: review?(glazman) → review?(caillon)
Comment 26•22 years ago
|
||
Comment on attachment 107913 [details] [diff] [review] Addressed comments I just did a quick run through of this patch, and found some issues I'd like to see addressed before I do a more thorough review. >Index: content/config.js >@@ -20,245 +20,377 @@ >+const nsISupportsString = Components.interfaces.nsISupportsString; Use nsIPrefLocalizedString instead. >+const nsIPromptService = Components.interfaces.nsIPromptService; >+const nsIPrefService = Components.interfaces.nsIPrefService; >+const nsIClipboardHelper = Components.interfaces.nsIClipboardHelper; > const nsIAtomService = Components.interfaces.nsIAtomService; >+const nsPrompt_CONTRACTID = "@mozilla.org/embedcomp/prompt-service;1"; >+const nsPref_CONTRACTID = "@mozilla.org/preferences-service;1"; Please use nsPrefService_CONTRACTID as the variable name. Otherwise it is too close to nsIPref which is deprecated. > // XXX get these from a string bundle >-var gLockStrs = ["default", "user set","locked"]; >+const gLockStrs = ["default", "user set", "locked"]; >+const gTypeStrs = ["string", "integer", "boolean"]; >+ >+const kDefault = 0; >+const kUserSet = 1; >+const kLocked = 2; > > const kStrType = 0; > const kIntType = 1; > const kBoolType = 2; Can these consts be removed? Why not use nsIPrefBranch.PREF_STRING and friends? Your array can then be changed to a map between them as such. Further, it would make getting them from a string bundle fit right in if you wish to do something like: var gTypeStrs = []; gTypeStrs[nsIPrefBranch.PREF_BOOL] = "bool"; // you can do this for now I guess if you aren't doing bundles... gTypeStrs[nsIPrefBranch.PREF_STRING] = strBundle.getStringFromName("about-config-pref_string"); >+function fetchPref(prefName, prefIndex) >+{ ... >+ try { >+ switch (prefBranch.getPrefType(prefName)) { >+ case prefBranch.PREF_BOOL: This would be cleaner if you had a const nsIPrefBranch = Components.interfaces.nsIPrefBranch at the top of the file, and used that here for resolving your pref type constants. >+ pref.typeCol = kBoolType; >+ // convert to a string >+ pref.valueCol = prefBranch.getBoolPref(prefName) ? "true" : "false"; pref.valueCol = prefBranch.getBoolPref(prefName).toString(); >+ break; >+ case prefBranch.PREF_INT: >+ pref.typeCol = kIntType; >+ // convert to a string >+ pref.valueCol = "" + prefBranch.getIntPref(prefName); pref.valueCol = prefBranch.getIntPref(prefName).toString(); >+ break; >+ //case prefBranch.PREF_STRING: Uncomment the previous line? It makes the code more readable. >+ default: >+ pref.valueCol = prefBranch.getComplexValue(prefName, nsISupportsString).data; >+ break; >+ } >+ } I hate to see exceptions like this get eaten, especially on a mainly power user feature. A dump may be nice here, although I don't care too much. In any case, if you ARE going to eat the exception, at least comment that you are doing so intentionally. >+ finally { >+ return pref; >+ } >+} >+function ModifyPref(entry) >+{ ... >+ default: >+ var SupportsString = { >+ data: result.value, >+ QueryInterface: function(iid) { return this; } >+ }; Eek! No. Don't do that. you want to .createInstance() here. If a method expects an XPCOM object, make sure you give it one and not a JS object which looks like one. If the c++ implementation of the methods you call ever change, it is possible that you can crash if it wants things on the object which you don't define in JS. (And make sure to use nsIPrefLocalizedString like I asked earlier, btw) >+ prefBranch.setComplexValue(entry.prefCol, nsISupportsString, SupportsString); >+ break; >+ } >+ return true; >+}
Attachment #107913 -
Flags: review?(caillon) → review-
Comment 27•22 years ago
|
||
> Eek! No. Don't do that. you want to .createInstance() here. If a method
> expects an XPCOM object, make sure you give it one and not a JS object which
> looks like one.
Since when is passing JS objects through XPConnect verboten?
Comment 28•22 years ago
|
||
Read the contract. http://lxr.mozilla.org/mozilla/source/modules/libpref/public/nsIPrefBranch.idl#162 174 * @param aValue The XPCOM object into which to the complex preference 175 * value should be retrieved. It does not say JS Object, it says XPCOM object. If you break the contract, then the method can do whatever it wants to spite you. If that means crashing, or hanging, or throwing exceptions, or any other thing like giving you something unexpected back, well you broke the contract so you should not expect the method to follow the contract either.
Comment 29•22 years ago
|
||
For the record, caillon provided a more thorough argument on IRC.
Comment 30•22 years ago
|
||
Attachment #107913 -
Attachment is obsolete: true
Updated•22 years ago
|
Attachment #108752 -
Flags: review?(caillon)
Updated•22 years ago
|
Attachment #108752 -
Flags: review?(caillon)
Updated•22 years ago
|
Attachment #108752 -
Flags: review?(evang-obsolete)
Comment 31•22 years ago
|
||
Comment on attachment 108752 [details] [diff] [review] Addressed caillon's comments Sorry for the delay in getting to this! >Index: content/config.js >+const nsIPrefLocalizedString = Components.interfaces.nsIPrefLocalizedString; >+const nsIPromptService = Components.interfaces.nsIPromptService; >+const nsIPrefService = Components.interfaces.nsIPrefService; >+const nsIPrefBranch = Components.interfaces.nsIPrefBranch; >+const nsIClipboardHelper = Components.interfaces.nsIClipboardHelper; > const nsIAtomService = Components.interfaces.nsIAtomService; Could you add a new line here, to make it easier to see that the types of values you're getting change in between here? >+const nsPrefLocalizedString_CONTRACTID = "@mozilla.org/pref-localizedstring;1"; >+const nsPrompt_CONTRACTID = "@mozilla.org/embedcomp/prompt-service;1"; >+const nsPrefService_CONTRACTID = "@mozilla.org/preferences-service;1"; >+const nsClipboardHelper_CONTRACTID = "@mozilla.org/widget/clipboardhelper;1"; > const nsAtomService_CONTRACTID = "@mozilla.org/atom-service;1"; > >-var atomService = Components.classes[nsAtomService_CONTRACTID].getService(nsIAtomService); >-var gLockAtoms = [atomService.getAtom("default"), atomService.getAtom("user"), atomService.getAtom("locked")]; >+const promptService = Components.classes[nsPrompt_CONTRACTID].getService(nsIPromptService); >+const prefService = Components.classes[nsPrefService_CONTRACTID].getService(nsIPrefService); >+const prefBranch = prefService.getBranch(null).QueryInterface(Components.interfaces.nsIPrefBranchInternal); >+const clipboardHelper = Components.classes[nsClipboardHelper_CONTRACTID].getService(nsIClipboardHelper); >+const atomService = Components.classes[nsAtomService_CONTRACTID].getService(nsIAtomService); prefer names prefixed with 'g' for the above instances. >+const gLockAtoms = [atomService.getAtom("default"), atomService.getAtom("user"), atomService.getAtom("locked")]; ... >+const gLockStrs = ["default", "user set", "locked"]; >+const gTypeStrs = []; Just use |var| rather than |const| for the three above declarations. |const| here is useless, since it does not prevent an array from being written to (as you yourself demonstrate just below) >+gTypeStrs[nsIPrefBranch.PREF_STRING] = "string"; >+gTypeStrs[nsIPrefBranch.PREF_INT] = "integer"; >+gTypeStrs[nsIPrefBranch.PREF_BOOL] = "boolean"; ... >+const kDefault = 0; >+const kUserSet = 1; >+const kLocked = 2; Also, if you're using consts, use uppercase, and maybe more descriptive names. PREF_IS_DEFAULT_VALUE, PREF_IS_USER_SET, PREF_IS_LOCKED maybe? ... >+ cycleHeader: function(colID, elt) { >+ var index = this.selection.currentIndex; >+ if (colID == gSortedColumn) >+ gSortDirection = -gSortDirection; Hmm, don't you want to switch the direction only if you're going to be doing the reverse() below? Shouldn't they be in the same block? >+ if (colID == gSortedColumn && gFastIndex == gPrefArray.length) { >+ gPrefArray.reverse(); >+ if (index >= 0) >+ index = gPrefArray.length - index - 1; >+ } ... >+function getIndexOfPref(pref) >+{ >+ var low = -1, high = gFastIndex; >+ var index = (low + high) >> 1; Why not use division (and subtracting maybe?) here rather than bitshifting? That is the standard way to write searches like this. If nothing else, there should definitely be a comment, explaining it, but I'd prefer the division. >+ while (index > low) { >+ var mid = gPrefArray[index]; >+ if (mid == pref) >+ return index; >+ if (gSortFunction(mid, pref) < 0) >+ low = index; >+ else >+ high = index; >+ index = (low + high) >> 1; >+ } New line here? Right below, would it be faster to cache the length in a var rather than look it up every iteration? (if nothing else, we probably will save the time to go through XPConnect each iteration) >+ for (index = gFastIndex; index < gPrefArray.length; ++index) >+ if (gPrefArray[index] == pref) >+ break; >+ return index; >+} ... >+function fetchPref(prefName, prefIndex) >+{ >+ var pref = { >+ prefCol: prefName, >+ lockCol: kDefault, >+ typeCol: nsIPrefBranch.PREF_STRING, >+ valueCol: "" >+ }; You end up returning the above object type, though it is local to this function. I know it works in JS, but "it works" is no reason to do it. I'd prefer you declare it in the global scope, with a more descriptive name and a comment as to its intended usage and users. Then when you want to use a new local pref object, just do var prefObj = new PrefDataObject(prefName, prefIdx); or similar. >+ >+ gPrefHash[prefName] = pref; >+ gPrefArray[prefIndex] = pref; >+ >+ if (prefBranch.prefIsLocked(prefName)) >+ pref.lockCol = kLocked; >+ else if (prefBranch.prefHasUserValue(prefName)) >+ pref.lockCol = kUserSet; >+ >+ try { >+ switch (prefBranch.getPrefType(prefName)) { >+ case prefBranch.PREF_BOOL: >+ pref.typeCol = prefBranch.PREF_BOOL; >+ // convert to a string >+ pref.valueCol = prefBranch.getBoolPref(prefName).toString(); >+ break; >+ case prefBranch.PREF_INT: >+ pref.typeCol = prefBranch.PREF_INT; >+ // convert to a string >+ pref.valueCol = prefBranch.getIntPref(prefName).toString(); >+ break; >+ default: >+ case prefBranch.PREF_STRING: >+ pref.valueCol = prefBranch.getComplexValue(prefName, nsIPrefLocalizedString).data; >+ break; >+ } >+ } When I asked you to comment this, I meant to put it into an empty catch. That way we know that you are "handling" exceptions per the comment. Move the return to the function scope, rather than a finally. >+ // There are obscure cases in which you can't tell in advance >+ // that the pref exists but has no user or default value... >+ finally { >+ return pref; >+ } >+} ... >+function onConfigLoad() >+{ ... Should you cache the prefCount.value? >+ for (var i = 0; i < prefCount.value; ++i) >+ { >+ var prefName = prefArray[i]; >+ if (/capability/.test(prefName)) // avoid displaying "private" preferences >+ continue; > >- prefArray[i] = htmlEscape(prefArray[i]); >- baseArray[k] = {indexCol:prefIndex, prefCol:prefArray[i], lockCol:prefLockState, typeCol:prefType, valueCol:prefValue}; >- } >+ fetchPref(prefName, gPrefArray.length); Hmm, what are you doing with the pref object you fetch? Seems like this is a wasted call? .... >+var gSortFunctions = { prefCol: prefColSortFunction, >+ lockCol: lockColSortFunction, >+ typeCol: typeColSortFunction, >+ valueCol: valueColSortFunction}; var foo = { bar: val1, bat: val2, baz: val3 }; > >-function lockColSortFunction(x,y) >+function copyValue() > { >- return stringSortFunction(x.lockCol, y.lockCol); >+ clipboardHelper.copyString(gPrefArray[view.selection.currentIndex].valueCol); > } > >-function typeColSortFunction(x,y) >+function Modify() > { >- return intSortFunction(x.typeCol, y.typeCol); >+ ModifyPref(gPrefArray[view.selection.currentIndex]); > } > >-function valueColSortFunction(x,y) >+function Reset() > { >- return stringSortFunction(x.valueCol, y.valueCol); >+ var entry = gPrefArray[view.selection.currentIndex]; >+ prefBranch.clearUserPref(entry.prefCol); > } > >-var gSortFunctions = {indexCol:indexColSortFunction, >- prefCol:prefColSortFunction, >- lockCol:lockColSortFunction, >- typeCol:typeColSortFunction, >- valueCol:valueColSortFunction}; >- >-function HandleColumnClick(id) >+function New(element) Can you use a different name besides |New|? It is too close to the reserved keyword |new|, and not very descriptive of what it does. > { >- if (id == gSortedColumn) { >- gSortAscending = !gSortAscending; >- baseArray.reverse(); >+ var type = parseInt(element.getAttribute("value")); >+ var result = {value: ""}; >+ var dummy = {value: 0}; Space between the braces and the literals, e.g. |{ foo: bar };| rather than |{foo: bar};| >+ // XXX get these from a string bundle Since you're already doing extensive changes, a string bundle isn't really that much work. But I'll let this slide... >+ if (promptService.prompt(window, >+ "New " + gTypeStrs[type] + " value", >+ "Enter the preference name", >+ result, >+ null, >+ dummy)) { >+ var pref; >+ if (result.value in gPrefHash) >+ pref = gPrefHash[result.value]; >+ else >+ pref = {prefCol: result.value, lockCol: kDefault, typeCol: type, valueCol: ""}; Brace space. >+ if (ModifyPref(pref)) >+ setTimeout(gotoPref, 0, result.value); > } >- else { >- baseArray.sort(gSortFunctions[id]); >- gSortedColumn = id; >- } >- >- gTree.treeBoxObject.invalidate(); > } ... >+function ModifyPref(entry) >+{ >+ if (entry.lockCol == kLocked) >+ return false; >+ var result = {value: entry.valueCol}; >+ var dummy = {value: 0}; >+ // XXX get this from a string bundle >+ if (!promptService.prompt(window, >+ "Enter " + gTypeStrs[entry.typeCol] + " value", >+ entry.prefCol, >+ result, >+ null, >+ dummy)) >+ return false; >+ switch (entry.typeCol) { >+ case nsIPrefBranch.PREF_BOOL: >+ prefBranch.setBoolPref(entry.prefCol, eval(result.value)); >+ break; >+ case nsIPrefBranch.PREF_INT: >+ prefBranch.setIntPref(entry.prefCol, eval(result.value)); >+ break; >+ default: >+ case nsIPrefBranch.PREF_STRING: >+ var prefLocalizedString = Components.classes[nsPrefLocalizedString_CONTRACTID].createInstance(nsIPrefLocalizedString); >+ prefLocalizedString.data = result.value; >+ prefBranch.setComplexValue(entry.prefCol, nsIPrefLocalizedString, prefLocalizedString); >+ break; >+ } Setting a pref could fail if the prefs file was somehow not able to be written to, couldn't it? (disk full, someone accidentally chmod'd it, etc). Or possibly you're OOM and can't create a new prefLocalizedString? Rare edge cases surely, but they probably should be expected and dealt with. Otherwise, everything looks fine and I'm about ready to r= this but I want to see another patch before doing so.
Attachment #108752 -
Flags: review?(caillon) → review-
Comment 32•22 years ago
|
||
>>+ cycleHeader: function(colID, elt) { >>+ var index = this.selection.currentIndex; >>+ if (colID == gSortedColumn) >>+ gSortDirection = -gSortDirection; > >Hmm, don't you want to switch the direction only if you're going to be doing >the reverse() below? Shouldn't they be in the same block? I can only use reverse() if I know that I can do a fast reverse(). Otherwise I always have to sort. >>+function getIndexOfPref(pref) >>+{ >>+ var low = -1, high = gFastIndex; >>+ var index = (low + high) >> 1; > >Why not use division (and subtracting maybe?) here rather than bitshifting? Difficult when I don't have an integer division operator. >Right below, would it be faster to cache the length in a var rather than look >it up every iteration? (if nothing else, we probably will save the time to go >through XPConnect each iteration) > >>+ for (index = gFastIndex; index < gPrefArray.length; ++index) >>+ if (gPrefArray[index] == pref) >>+ break; >>+ return index; >>+} I didn't think .length would be slow on a JS array. Also I don't expect many iterations here. >>+function fetchPref(prefName, prefIndex) >>+{ >>+ var pref = { >>+ prefCol: prefName, >>+ lockCol: kDefault, >>+ typeCol: nsIPrefBranch.PREF_STRING, >>+ valueCol: "" >>+ }; > >You end up returning the above object type, though it is local to this >function. I know it works in JS, but "it works" is no reason to do it. I'd >prefer you declare it in the global scope, with a more descriptive name and a >comment as to its intended usage and users. Then when you want to use a new >local pref object, just do var prefObj = new PrefDataObject(prefName, prefIdx); >or similar. Well I almost never need to use the object - there's only one case were I need the object itself after creating it and adding it to the array and hash. So perhaps I should figure out a way to avoid returning it instead. >>+function onConfigLoad() >>+{ >... >Should you cache the prefCount.value? It's a JS property, should be reasonably fast? >Setting a pref could fail if the prefs file was somehow not able to be written >to, couldn't it? (disk full, someone accidentally chmod'd it, etc). Or >possibly you're OOM and can't create a new prefLocalizedString? Rare edge >cases surely, but they probably should be expected and dealt with. How?
Comment 33•22 years ago
|
||
> I know it works in JS, but "it works" is no reason to do it.
No. And using functions "just works", but that's not a reason to do it. For
that matter, using classes shouldn't be done just because it's doable...
Is there a real reason for declaring a constructor for that object if it's only
ever constructed in one place? Apart from that, your suggested code is
identical to Neil's (and also just returns a local)...
Comment 34•22 years ago
|
||
Attachment #108752 -
Attachment is obsolete: true
Updated•22 years ago
|
Attachment #111164 -
Flags: review?(caillon)
Comment 35•22 years ago
|
||
Comment on attachment 111164 [details] [diff] [review] Addressed some of caillon's comments >+function getIndexOfPref(pref) > { >+ var low = -1, high = gFastIndex; >+ var index = (low + high) >> 1; >+ while (index > low) { >+ var mid = gPrefArray[index]; >+ if (mid == pref) >+ return index; >+ if (gSortFunction(mid, pref) < 0) >+ low = index; >+ else >+ high = index; >+ index = (low + high) >> 1; >+ } >+ for (index = gFastIndex; index < gPrefArray.length; ++index) >+ if (gPrefArray[index] == pref) >+ break; >+ return index; >+} It is very unlikely to ever happen, but if the pref does not exist, you will be returned an index that is equal to your array length, and that may break callers. You might want to check that you have a valid index before returning, and return -1 otherwise. And your callers probably should check for a value != -1. >+function onConfigLoad() >+{ >+ document.title = "about:config"; Wanna add an XXX comment about a stringbundle here? I can see how someone may want to change this title. >Index: content/config.xul >=================================================================== >- <treechildren id="configTreeBody"/> >+ <treechildren id="configTreeBody" ondblclick="if (event.button == 0) Modify();"/> This should call ModifySelected(), no? r=caillon with the comments addressed.
Attachment #111164 -
Flags: review?(caillon) → review+
Comment 36•22 years ago
|
||
caillon: not true, getIndexOfPref is only passed an existing pref object. Also, the document title is the url, so I don't see the point of changing it... But thanks for pointing out my typo :-/
Comment 37•22 years ago
|
||
Comment on attachment 111164 [details] [diff] [review] Addressed some of caillon's comments OK, I've changed Modify() to ModifySelected() in my copy of the patch.
Attachment #111164 -
Flags: superreview?(sspitzer)
Comment 38•22 years ago
|
||
If it's only passed real prefs, fine. But at the very least, add a comment stating that it's a precondition for the pref to exist. (Don't assume future callers will do that unless you tell them to with a comment) Why should the title be the URL? That's already in the urlbar. It is redundant to have it as the title, IMO. It would be cooler to name it "My preferences" or something, so that someone who has no idea what about:config is and stumbles on it by accident would have a quicker idea. Plus, about:config only really has a meaning in english. In other languages, it is just a random URL. IMO, all chrome urls should have descriptive titles. However, that is a different bug and I don't think you should change it here.
Assignee | ||
Comment 39•22 years ago
|
||
Comment on attachment 111164 [details] [diff] [review] Addressed some of caillon's comments automatic rs=sspitzer, since callion has given a thorough review. about:config is such a power user thing, I'm ok with letting neil do what ever he sees fit.
Attachment #111164 -
Flags: superreview?(sspitzer) → superreview+
Comment 40•22 years ago
|
||
The CVS checkin comment mentions bug 107148 which is almost, but not exactly, this bug... may be confusing...
Comment 41•22 years ago
|
||
Oops, looks like I mistyped the checkin comment for bug 107418 as bug 107148 :-(
Comment 42•22 years ago
|
||
Any new enchancements can be filed separately. Marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 43•22 years ago
|
||
*** Bug 155179 has been marked as a duplicate of this bug. ***
Comment 44•22 years ago
|
||
is this supposed to be working under Mac OS X? (build 2003012008)
Comment 45•22 years ago
|
||
With trunk build 2003012008 on w2k I only get empty list, see attachment.
Comment 46•22 years ago
|
||
With 2003012005 on Win2k it works for me.
Comment 47•22 years ago
|
||
Does *not* work for me. WinXP Build 2003012008 Looks like we narrowed it down. :-)
Comment 48•22 years ago
|
||
Bug 189452? Checked in at 01/20/2003 05:39 (not marked resolved yet).
Comment 49•22 years ago
|
||
See bug 189452 comment #4 (or your JS console...) for more info. That bug caused the problem, not this bug here.
Comment 50•22 years ago
|
||
Does not work for me, Linux CVS HEAD. I get the headers, no content.
Comment 51•22 years ago
|
||
Adam, that is bug 189835. *This* bug here is fixed.
Comment 52•21 years ago
|
||
This does not work properly in MFCEmbed. Using the latest nightly (1/27/2003), entering "about:config" shows the headers and all of the preferences. You can even edit the preferences and they appear as changed in the preferences displayed. However, the changes don't get saved and don't appear changed when the browser is restarted. Let me know if this needs to be filed as a separate bug.
Comment 53•21 years ago
|
||
vrfy'd fixed with comm trunk (2003.02.12) builds on linux rh8.0, win2k and mac 10.2.3. tested the items in comment 10 and comment 11. neil, thanks so much for doing this!
Status: RESOLVED → VERIFIED
Comment 54•21 years ago
|
||
See also bug 195845 about a description for prefs.
Comment 55•21 years ago
|
||
Comment on attachment 107902 [details] comments about attachment 95384 [details] [diff] [review] >>+ if (colID == gSortedColumn && gFastIndex == gPrefArray.length) { >>+ gPrefArray.reverse(); >>+ if (index >= 0) >>+ index = gPrefArray.length - index - 1; >>+ } else { > >else on its own line Is that prevailing style? Not in lots of Mozilla code. >>+ isEditable: function(row, colID) {return false}, > >missing ; Why care? >>+ for (index = gFastIndex; index < gPrefArray.length; index++) > >++index No. There's no good reason for such style mongering here, in C, or in C++ with scalar types. One size does not fit all. Pedantry is bad if there's no payoff. Pedagogy is good for kids. We're all adults here, right? I'm not sure what else to say. /be
Comment 56•21 years ago
|
||
I'm also wondering (late to this bug, sorry) why JS code is being reviewed as if it were written in C++ ((low + high) >> 1 is *necessary*; object literals are good; etc.). Nuff said. Grump. /be
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•