Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Updated AddonManager userscripts to match other addon-types #1861

Closed
wants to merge 13 commits into from

5 participants

@jerone

It always bothered me that the add-on manager for userscripts is a little bit different then the other add-on types.

So today I updated the:

  • scripts listing - Added the following:
    • More - Added the More link, linking to the scripts detail page.
  • details page - Added the following meta:
    • Creator - Shows the author when // @author is defined in the userscript. When homepage (see below) is defined, creator will be linkable.
    • Homepage - Adds link to scripts homepage when // @homepageURL is defined in the userscript.
  • context-menu - Right-click on an userscript. Added menuitems for:
    • Show More Information - Opens the details page. This item is default behavior, so it's bold.
    • About - Opens the about dialog.
    • Execute sooner & later - Disable second & second to last execution order menuitem. Fixes greasemonkey#1220

No localization updates, as all are from Fx own localization.

Maybe TODO's:

  • Add more metadata's that have same attributes in Fx (like // @supportURL && // @contributionURL and more).
  • Add list of all remaining metadata's (e.g. // @grant).
  • Minimize menuitems Execute (first|sooner|later|last) (maybe a sub-menu) -> see comment #1861 (comment) & #1861 (comment)
@jerone

Test script can be found here: http://userscripts.org/scripts/show/292712

@jerone

#1706 is a nice addition to this.

@jerone

For the contextmenu in the userscripts listing I was thinking about this for the execution order:
execution_order
Makes the list smaller and groups connecting actions together.
Any opinions...

Ref: #1837

content/addons4-overlay.js
@@ -135,6 +135,18 @@ function focus() {
function init() {
GM_util.getService().config.addObserver(observer);
+ // http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#771
+ gViewController.commands.cmd_userscript_showItemDetails = {
+ isEnabled: function cmd_userscript_showItemDetails_isEnabled(aAddon) {
@Ventero
Ventero added a note

Nit: Trailing whitespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
content/addons4-overlay.js
@@ -135,6 +135,18 @@ function focus() {
function init() {
GM_util.getService().config.addObserver(observer);
+ // http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#771
+ gViewController.commands.cmd_userscript_showItemDetails = {
+ isEnabled: function cmd_userscript_showItemDetails_isEnabled(aAddon) {
+ return addonIsInstalledScript(aAddon) && (gViewController.currentViewObj != gDetailView);
+ },
+ doCommand: function cmd_userscript_showItemDetails_doCommand(aAddon, aScrollToPreferences) {
@Ventero
Ventero added a note

Nit: Trailing whitespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
content/addons4-overlay.js
@@ -149,11 +161,17 @@ function init() {
doCommand: function(aAddon) { reorderScriptExecution(aAddon, -9999); }
};
gViewController.commands.cmd_userscript_execute_sooner = {
- isEnabled: addonExecutesNonFirst,
+ isEnabled: function cmd_userscript_execute_sooner_isEnabled(aAddon) {
@Ventero
Ventero added a note

Nit: Trailing whitespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
content/addons4-overlay.js
((8 lines not shown))
doCommand: function(aAddon) { reorderScriptExecution(aAddon, -1); }
};
gViewController.commands.cmd_userscript_execute_later = {
- isEnabled: addonExecutesNonLast,
+ isEnabled: function cmd_userscript_execute_later_isEnabled(aAddon) {
@Ventero
Ventero added a note

Nit: Trailing whitespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
modules/addons4.js
@@ -118,6 +119,24 @@ ScriptAddon.prototype.description = null;
// Private and custom attributes.
ScriptAddon.prototype._script = null;
+ScriptAddon.prototype.__defineGetter__('creator',
+function ScriptAddon_getCreator() {
+ var value = parseMetaById(this._script.textContent, 'author');
+ if(value) {
+ return new ScriptAddonAuthor(value);
+ }
+ return null;
+});
+
+ScriptAddon.prototype.__defineGetter__('homepageURL',
+function ScriptAddon_getHomepageURL() {
+ var value = parseMetaById(this._script.textContent, 'homepageURL');
@Ventero
Ventero added a note

This can be simplified to return parseMetaById(...);.

@jerone
jerone added a note

Thnx for the review.
parseMetaById will return the value or null. A value can also be an empty string. All ScriptAddon attributes defaults are actual explicitly set to null (don't know if that's intended). Don't know if an empty string will also ignore the value or cause problems with empty urls...

@Ventero
Ventero added a note

I'm not sure I follow. In parseMetaById, you have something like if (aId == match[1] && match[2]) return match[2]; return null - thus, it will return null if 1) the given aId could not be found or 2) contains an empty value. Thus, in the homepageURL getter, the only way the if (value) branch isn't taken is if value is null. Thus, if (value) return value; return null; is effectively the same as return value;.

@jerone
jerone added a note

You're so correct! Missed the && match[2].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
content/addons4-overlay.xul
((5 lines not shown))
</commandset>
<menupopup id="addonitem-popup">
+ <menuitem command="cmd_userscript_showItemDetails" class="greasemonkey"
@Ventero
Ventero added a note

Nit: trailing whitespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
content/addons4-overlay.xul
((49 lines not shown))
label="&ForcedFindUpdate;"
/>
+
@Ventero
Ventero added a note

Nit: trailing whitespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ventero

In general, wouldn't it make sense to fall back as much as possible to the existing implementations of the addon commands etc.? For example, instead of copy/pasting the code from gViewController.commands.showItemDetails.doCommand into gViewController.commands.cmd_userscript_showItemDetails.doCommand, just make the latter point to the former. The same goes for ScriptAddonAuthor, just use the existing AddonAuthor. Especially because the copied code is licensed under MPL2.0, which would have to be mentioned somewhere (currently all MPL licensed code is in a separate third-party/ folder as described in the CREDITS file).

I also think that in the detail view, the execution order commands in the context menu should be hidden/disabled. When opening the detail view of an extension, all disabled links are hidden, maybe we should adopt this for userscripts as well (e.g. by adding a new CSS rule to hide disabled menuitems in the detail view, which would also allow you to drop the greasemonkeyNoHide class)?

It seems a bit unnecessary to re-parse the metadata of all scripts just to get the author/homepage URL. Maybe just turn these into "official" metadata keys recognized by parse() and store them in the script object?

I've also left some comments/nitpicks on the actual diff.

@arantius
Collaborator

-0 on changing the context menu

If you want to use it, a nested context menu is much harder. Some people are already complaining that it's too many clicks to edit execution order. If you don't want to use it, a seven vs. ten item menu doesn't seem like a big difference to me.

@Ventero

Would adding drag/drop support to change the execution order make sense? Probably not all that easy to implement, but definitely a much simpler/easier user interface.

@jerone

The same goes for ScriptAddonAuthor, just use the existing AddonAuthor. Especially because the copied code is licensed under MPL2.0, which would have to be mentioned somewhere (currently all MPL licensed code is in a separate third-party/ folder as described in the CREDITS file).

Fixed that, used AddonManagerPrivate.AddonAuthor now.

In general, wouldn't it make sense to fall back as much as possible to the existing implementations of the addon commands etc.? For example, instead of copy/pasting the code from gViewController.commands.showItemDetails.doCommand into gViewController.commands.cmd_userscript_showItemDetails.doCommand,

I don't know about this one. The existing code did this already and means an complete change for all menu-items.

I've also left some comments/nitpicks on the actual diff.

Fixed.

@Ventero

I don't know about this one. The existing code did this already and means an complete change for all menu-items.

The existing commands are all completely custom, so there's nothing in the addon manager that could be re-used for them, whereas your new commands are all verbatim copies of existing commands.

@Ventero

I should probably clarify this: I'm only talking about the doCommand functions that are exact copies of the addon manager's functions. The actual command objects are of course necessary.

@jerone

I should probably clarify this: I'm only talking about the doCommand functions that are exact copies of the addon manager's functions. The actual command objects are of course necessary.

The code from show-details & about is now executing the doCommand from Mozilla.

@jerone

It seems a bit unnecessary to re-parse the metadata of all scripts just to get the author/homepage URL. Maybe just turn these into "official" metadata keys recognized by parse() and store them in the script object?

I was hoping this was the situation, but it's not. Now is this information only requested when an user access the details/about page. Which (I think) is not often.

@jerone

Would adding drag/drop support to change the execution order make sense? Probably not all that easy to implement, but definitely a much simpler/easier user interface.

+1 With the knowledge I've got now, I'm not the one who can build that.

@arantius
Collaborator

I'd resist adding drag'n'drop into the AOM. It would be difficult, and very difficult to support as time goes forward.

@jerone

I'd resist adding drag'n'drop into the AOM. It would be difficult, and very difficult to support as time goes forward.

Understand.

Is it an idea to show execution context-items only when sorted on execution order?

@arantius
Collaborator

We have little to no hard data on when/why/how/how often this feature is used. So my design is based off of speculation.

If a user knows what they're doing, all you ever need to do is pick the script that must run before others (i.e. needs an unchanged page, or it breaks), pick "run first" on it, and go on their way. The current UI solves this perfectly. If you've got really special needs, you can tweak things even further. But in general, one should never even need to switch to the sort-by-execution view.

Hiding the ability to change execution order behind that view would make it harder to use. It would make it harder to know where the script one needs to change is. (I know its name, and I know the alphabet, but I don't know where it already is in execution order.) It would be harder to discover that it's even an option.

I'd like to see a concrete explanation for what the use case is (and why it is common) before we go changing the UI around.

@jerone

I'll leave the execution order alone. If anyone is interested in combining into menu, here's the patch: https://gist.github.com/jerone/8658483 as seen in #1861 (comment)

I'm marking this PR feature complete, unless someone has any other suggestion.

@jerone

Confirmed working up till Firefox Aurora 29.0a2 (2014-02-09).

@arantius arantius added this to the 1.17 milestone
@sizzlemctwizzle

Personally, in all my years of using GM I've never once changed the execution order of a script, so I really have nothing to say except that'd love to have it out of the way. But I realize users apparently need it for some reason (yeah I know why, it just never happens to me) and it needs to be easily accessible/discoverable.

Anyway, the more information the better so I'm +1 on this PR.

@arantius arantius modified the milestone: 2.2, 2.1
@jerone jerone closed this
@arantius arantius removed this from the 2.2 milestone
@Ede123

Hmm, why is this closed? It was never merged, right?

Actually I think this would be a really helpful pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
24 content/addons4-overlay.js
@@ -135,6 +135,14 @@ function focus() {
function init() {
GM_util.getService().config.addObserver(observer);
+ // http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#771
+ gViewController.commands.cmd_userscript_showItemDetails = {
+ isEnabled: function cmd_userscript_showItemDetails_isEnabled(aAddon) {
+ return addonIsInstalledScript(aAddon) && (gViewController.currentViewObj != gDetailView);
+ },
+ doCommand: gViewController.commands.cmd_showItemDetails.doCommand
+ };
+
gViewController.commands.cmd_userscript_edit = {
isEnabled: addonIsInstalledScript,
doCommand: function(aAddon) { GM_util.openInEditor(aAddon._script); }
@@ -149,11 +157,17 @@ function init() {
doCommand: function(aAddon) { reorderScriptExecution(aAddon, -9999); }
};
gViewController.commands.cmd_userscript_execute_sooner = {
- isEnabled: addonExecutesNonFirst,
+ isEnabled: function cmd_userscript_execute_sooner_isEnabled(aAddon) {
+ return addonExecutesNonFirst(aAddon) && 1 != aAddon.executionIndex;
+ },
doCommand: function(aAddon) { reorderScriptExecution(aAddon, -1); }
};
gViewController.commands.cmd_userscript_execute_later = {
- isEnabled: addonExecutesNonLast,
+ isEnabled: function cmd_userscript_execute_later_isEnabled(aAddon) {
+ return addonExecutesNonLast(aAddon) &&
+ (GM_util.getService().config.scripts.length - 2)
+ != aAddon.executionIndex;
+ },
doCommand: function(aAddon) { reorderScriptExecution(aAddon, 1); }
};
gViewController.commands.cmd_userscript_execute_last = {
@@ -174,6 +188,12 @@ function init() {
}
};
+ // http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#949
+ gViewController.commands.cmd_userscript_showItemAbout = {
+ isEnabled: addonIsInstalledScript,
+ doCommand: gViewController.commands.cmd_showItemAbout.doCommand
+ };
+
window.addEventListener('ViewChanged', onViewChanged, false);
onViewChanged(); // initialize on load as well as when it changes later
View
27 content/addons4-overlay.xul
@@ -20,6 +20,7 @@
<script type="application/x-javascript" src="chrome://greasemonkey/content/third-party/mpl-utils.js" />
<commandset id="viewCommandSet">
+ <command id="cmd_userscript_showItemDetails" />
<command id="cmd_userscript_edit" />
<command id="cmd_userscript_show" />
<command id="cmd_userscript_execute_first" />
@@ -28,39 +29,49 @@
<command id="cmd_userscript_execute_last" />
<command id="cmd_userscript_showItemPreferences" />
<command id="cmd_userscript_forcedFindItemUpdates" />
+ <command id="cmd_userscript_showItemAbout"/>
</commandset>
<menupopup id="addonitem-popup">
+ <menuitem command="cmd_userscript_showItemDetails" class="greasemonkey"
+ label="&cmd.showDetails.label;" accesskey="&cmd.showDetails.accesskey;"
+ default="true"
+ />
+
<menuitem command="cmd_userscript_edit"
- label="&Edit;" accesskey="&Edit.accesskey;" class="greasemonkey"
+ label="&Edit;" accesskey="&Edit.accesskey;" class="greasemonkey greasemonkeyNoHide"
/>
<menuitem command="cmd_userscript_show"
- label="&Show;" accesskey="&Show.accesskey;" class="greasemonkey"
+ label="&Show;" accesskey="&Show.accesskey;" class="greasemonkey greasemonkeyNoHide"
/>
<menuseparator class="greasemonkey"/>
<menuitem command="cmd_userscript_execute_first"
- label="&ExecuteFirst;" class="greasemonkey"
+ label="&ExecuteFirst;" class="greasemonkey greasemonkeyNoHide"
/>
<menuitem command="cmd_userscript_execute_sooner"
- label="&ExecuteSooner;" class="greasemonkey"
+ label="&ExecuteSooner;" class="greasemonkey greasemonkeyNoHide"
/>
<menuitem command="cmd_userscript_execute_later"
- label="&ExecuteLater;" class="greasemonkey"
+ label="&ExecuteLater;" class="greasemonkey greasemonkeyNoHide"
/>
<menuitem command="cmd_userscript_execute_last"
- label="&ExecuteLast;" class="greasemonkey"
+ label="&ExecuteLast;" class="greasemonkey greasemonkeyNoHide"
/>
<menuseparator class="greasemonkey"/>
<!-- Default find updates item. -->
- <menuitem command="cmd_findItemUpdates" class="greasemonkey"
+ <menuitem command="cmd_findItemUpdates" class="greasemonkey greasemonkeyNoHide"
label="&cmd.findUpdates.label;" accesskey="&cmd.findUpdates.accesskey;"
/>
<!-- Custom forced find updates item. -->
- <menuitem command="cmd_userscript_forcedFindItemUpdates" class="greasemonkey"
+ <menuitem command="cmd_userscript_forcedFindItemUpdates" class="greasemonkey greasemonkeyNoHide"
label="&ForcedFindUpdate;"
/>
+
+ <menuitem command="cmd_userscript_showItemAbout" class="greasemonkey greasemonkeyNoHide"
+ label="&cmd.about.label;" accesskey="&cmd.about.accesskey;"
+ />
</menupopup>
<vbox id="list-view">
View
15 modules/addons4.js
@@ -21,6 +21,7 @@ Components.utils.import('resource://gre/modules/XPCOMUtils.jsm');
Components.utils.import('resource://greasemonkey/prefmanager.js');
Components.utils.import('resource://greasemonkey/remoteScript.js');
Components.utils.import('resource://greasemonkey/util.js');
+Components.utils.import("resource://greasemonkey/parseScript.js");
var Cc = Components.classes;
var Ci = Components.interfaces;
@@ -118,6 +119,20 @@ ScriptAddon.prototype.description = null;
// Private and custom attributes.
ScriptAddon.prototype._script = null;
+ScriptAddon.prototype.__defineGetter__('creator',
+function ScriptAddon_getCreator() {
+ var value = parseMetaById(this._script.textContent, 'author');
+ if(value) {
+ return new AddonManagerPrivate.AddonAuthor(value);
+ }
+ return null;
+});
+
+ScriptAddon.prototype.__defineGetter__('homepageURL',
+function ScriptAddon_getHomepageURL() {
+ return parseMetaById(this._script.textContent, 'homepageURL');
+});
+
ScriptAddon.prototype.__defineGetter__('applyBackgroundUpdates',
function ScriptAddon_getApplyBackgroundUpdates() {
return this._script.checkRemoteUpdates;
View
16 modules/parseScript.js
@@ -1,5 +1,5 @@
var EXPORTED_SYMBOLS = [
- 'extractMeta', 'parse', 'gLineSplitRegexp', 'gMetaLineRegexp'];
+ 'extractMeta', 'parse', 'parseMetaById', 'gLineSplitRegexp', 'gMetaLineRegexp'];
Components.utils.import('resource://greasemonkey/script.js');
Components.utils.import('resource://greasemonkey/scriptIcon.js');
@@ -26,6 +26,20 @@ function extractMeta(aSource) {
return '';
}
+/** Gets meta value by meta id from source of a script. **/
+function parseMetaById(aSource, aId) {
+ var metaLines = extractMeta(aSource).match(gLineSplitRegexp);
+ for (var j = 0, metaLine = null; metaLine = metaLines[j]; j++) {
+ metaLine = metaLine.replace(/\s+$/, '');
+ var match = metaLine.match(gMetaLineRegexp);
+ if (!match) continue;
+ if (aId == match[1] && match[2]) {
+ return match[2];
+ }
+ }
+ return null;
+}
+
/** Parse the source of a script; produce Script object. */
function parse(aSource, aUri, aFailWhenMissing, aNoMetaOk) {
var meta = extractMeta(aSource).match(gLineSplitRegexp);
View
10 skin/addons4.css
@@ -5,10 +5,6 @@
list-style-image: url(chrome://greasemonkey/skin/icon32.png);
}
-.addon.addon-view[type="greasemonkey-user-script"] .details.button-link {
- display: none !important;
-}
-
#greasemonkey-sort-bar {
display: none;
}
@@ -31,10 +27,6 @@ page.greasemonkey #user-script-list-empty {
display: none;
}
-.addon.addon-view[type="greasemonkey-user-script"] .details.button-link {
- display: none !important;
-}
-
/* Do not show native items when view is user scripts. */
#addonitem-popup[addontype="greasemonkey-user-script"] menuitem:not(.greasemonkey),
#addonitem-popup[addontype="greasemonkey-user-script"] menuseparator:not(.greasemonkey),
@@ -45,7 +37,7 @@ page.greasemonkey #user-script-list-empty {
}
/* Do show all items (even disabled ones!) when view is user scripts. */
-#addonitem-popup[addontype="greasemonkey-user-script"] .greasemonkey
+#addonitem-popup[addontype="greasemonkey-user-script"] .greasemonkey.greasemonkeyNoHide
{
display: -moz-box !important;
}
Something went wrong with that request. Please try again.