Made extension separator configurable #232
Conversation
* Reverted default extension separator from comma to new line, except for input fields because they don't support newlines * Added a hidden preference nightly.extensionSeparator to optionally customize the extension separator
| @@ -9,4 +9,6 @@ pref("nightly.disableCheckCompatibility", false); | |||
| pref("nightly.currChangeset", ""); | |||
| pref("nightly.prevChangeset", ""); | |||
|
|
|||
| pref("nightly.extensionSeparator", ""); | |||
There was a problem hiding this comment.
I do not see a default value set here.
There was a problem hiding this comment.
That's because there is not one default. The default behaviour is a new line, except for input fields because they don't support newlines.
If a customized extension separator is set it will be used everywhere.
There was a problem hiding this comment.
Another advantage of this would be that in the future we'll be able to detect if a user customized the extension separator or not, so we can make changes to the default behaviour only for users who didn't customize the extension separator.
There was a problem hiding this comment.
Ok, so we should check how we can handle input fields correctly. I will have to think about this.
But generally please define the default separator really in the prefs file. AFAIK this will put this as default value, and whenever a user changes the pref value it will be marked as user set which we can read out / detect if necessary.
| @@ -406,9 +414,13 @@ insertExtensions: function() { | |||
| }, | |||
|
|
|||
| copyExtensions: function() { | |||
| var extensionSeparator = "\n"; | |||
There was a problem hiding this comment.
Defaults are set in preferences/nightlytools.js but not in the code.
There was a problem hiding this comment.
The default extension separator is set in the code, the extension separator can optionally be overwritten by customizing the new hidden preference nightly.extensionSeparator.
There was a problem hiding this comment.
It has to be used the other way around. Means read the pref first and then check if it has to be changed eg. for input fields.
| @@ -391,10 +391,18 @@ insertExtensions: function() { | |||
| if (element) { | |||
| var type = element.localName.toLowerCase(); | |||
| if ((type == "input") || (type == "textarea")) { | |||
| if (type == "input") { | |||
There was a problem hiding this comment.
Please remove this block of magic decision. If the user wants a new line it has to be respected everywhere.
There was a problem hiding this comment.
That's not possible, because input fields don't support newlines.
Without this, all extensions would be glued together on a single line without any separation in the input field, because the newlines just disappear when used inside input fields.
There was a problem hiding this comment.
So you talk about input fields here but what about textarea? It should support new lines just fine. So we should only special-case input here.
There was a problem hiding this comment.
Indeed, that's how it is in this PR: by default only in case of input fields a comma is used, in all other cases (textarea, copy to clipboard) new lines are used.
| var extensionSeparator = "\n"; | ||
| } | ||
| if (nightly.preferences.prefHasUserValue("extensionSeparator")) { | ||
| extensionSeparator = nightly.preferences.getCharPref("extensionSeparator"); |
There was a problem hiding this comment.
@whimboo If a customized extension separator is set, it will be used instead of what's defined as default behaviour above.
There was a problem hiding this comment.
As said earlier. Define the default in the .js file, load it first and then in case of an input field update it if needed. The code above is in the wrong order.
There was a problem hiding this comment.
@whimboo I don't know how to define the default new line in the .js file, since entering \n there will not result in a new line, instead it gets escaped, resulting in all the extensions on a single line literally separated by "\n".
What should I set as default in the .js file?
There was a problem hiding this comment.
Thats a bad news.
A few tips, I'm unsure about success:
\\n- a physical new line
There was a problem hiding this comment.
@xabolcs "\n" also gets escaped, and it seems impossible to enter a physical new line in the .js file.
There was a problem hiding this comment.
How about creating a private getSeparator() and let the parameter be newline, space, tab, comma, semicolon.
There was a problem hiding this comment.
@UtiluMark, do you have a commit up on github which I could use to check this problem with \n?
There was a problem hiding this comment.
@whimboo Yes, you can use this separator branch and set for example \n as value of nightly.extensionSeparator in about:config to see the problem of \n getting escaped.
| if (type == "input") { | ||
| var extensionSeparator = ", "; | ||
| } else { | ||
| var extensionSeparator = "\n"; |
There was a problem hiding this comment.
@whimboo If input use comma, else use new line.
|
@UtiluMark, may I close this PR or do you want to rebase it? It's already referenced in shared.js#L91. |
|
@xabolcs I'm working on it. |
No hurry! :) |
for input fields because they don't support newlines
customize the extension separator
This should fix the issues mentioned in #195, #230 and #231.