Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made extension separator configurable #232

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Made extension separator configurable #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions extension/chrome/content/nightly.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,10 +391,18 @@ insertExtensions: function() {
if (element) {
var type = element.localName.toLowerCase();
if ((type == "input") || (type == "textarea")) {
if (type == "input") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this block of magic decision. If the user wants a new line it has to be respected everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = ", ";
} else {
var extensionSeparator = "\n";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whimboo If input use comma, else use new line.

}
if (nightly.preferences.prefHasUserValue("extensionSeparator")) {
extensionSeparator = nightly.preferences.getCharPref("extensionSeparator");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whimboo If a customized extension separator is set, it will be used instead of what's defined as default behaviour above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats a bad news.

A few tips, I'm unsure about success:

  • \\n
  • a physical new line

Copy link
Contributor Author

@UtiluMark UtiluMark Nov 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xabolcs "\n" also gets escaped, and it seems impossible to enter a physical new line in the .js file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about creating a private getSeparator() and let the parameter be newline, space, tab, comma, semicolon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@UtiluMark, do you have a commit up on github which I could use to check this problem with \n?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

about:config?

}
nightly.getExtensionList(function(text) {
var newpos = element.selectionStart + text.length;
var value = element.value;
element.value = value.substring(0, element.selectionStart) + text.join(", ") +
element.value = value.substring(0, element.selectionStart) + text.join(extensionSeparator) +
value.substring(element.selectionEnd);
element.selectionStart = newpos;
element.selectionEnd = newpos;
Expand All @@ -406,9 +414,13 @@ insertExtensions: function() {
},

copyExtensions: function() {
var extensionSeparator = "\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaults are set in preferences/nightlytools.js but not in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default extension separator is set in the code, the extension separator can optionally be overwritten by customizing the new hidden preference nightly.extensionSeparator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (nightly.preferences.prefHasUserValue("extensionSeparator")) {
extensionSeparator = nightly.preferences.getCharPref("extensionSeparator");
}
nightly.getExtensionList(function(text) {
if (text)
nightly.copyText(text.join(", "));
nightly.copyText(text.join(extensionSeparator));
});
},

Expand Down
2 changes: 2 additions & 0 deletions extension/defaults/preferences/nightlytools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,6 @@ pref("nightly.disableCheckCompatibility", false);
pref("nightly.currChangeset", "");
pref("nightly.prevChangeset", "");

pref("nightly.extensionSeparator", "");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see a default value set here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


pref("extensions.{8620c15f-30dc-4dba-a131-7c5d20cf4a29}.description", "chrome://nightly/locale/nightly.properties");