XWIKI-16216: Allow users to rebind the shortcuts from their user profile UI #2981
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -155,8 +155,9 @@ | |
| ## Override keyboard shortcut (if any) | ||
| ## | ||
| #if ($keyboardShortcutsEnabled && "$!extraShortcut" != "") | ||
| #set ($extraShortcutWithUserPreferences = "#getShortcutValue($extraShortcut)") | ||
|
There was a problem hiding this comment. Are you sure this is correct? From what I understand, There was a problem hiding this comment. Thank you for noticing this 👍 I tested this new version locally and it worked properly with unset shortcuts (empty) and regular edited values |
||
| shortcut.remove("$extraShortcutWithUserPreferences"); | ||
| shortcut.add("$extraShortcutWithUserPreferences", function() { XWiki.displayDocExtra("${extraAnchor}", "${extraTemplate}", true); }, { 'disable_in_input':true }); | ||
Sereza7 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| #end | ||
| #end | ||
| document.observe("dom:loaded", extraInit, false); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,6 +65,21 @@ | |
| #macro(submitButton $name $shortcut $value $class) | ||
| <input class="btn $!class" type="submit" name="$name"#if($keyboardShortcutsEnabled) title="$shortcut"#end value="$value"/> | ||
| #end | ||
| #** | ||
| * Returns the escaped value of the shortcut. This value can be set in two ways: via user preferences and wiki level translation | ||
Sereza7 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * keys. If there are both, the user preference will take priority. | ||
| * @param userPreferenceName The id of the user preference that is listed in XWikiUsersDocumentInitializer.java | ||
| * e.g. `shortcut_developer_usertype` | ||
| * @param translationKey The key of the translation containing the wiki level defined shortcut. | ||
| * e.g. `core.shortcuts.developer.user.type` | ||
| * @since 16.9.0-rc-1 | ||
| *# | ||
| #macro(getShortcutValue $translationKey) | ||
| #set ($shortcutPreferenceObject = $xwiki.getDocument($xcontext.getUser()).getObject('XWiki.UserPreferenceShortcutsClass', 'translationKey', $translationKey)) | ||
|
There was a problem hiding this comment. This call is quite expensive. This gets the document from the cache and performs a full clone of it. This seems particularly dangerous as this macro is likely called in a loop with all shortcuts and this might happen in every request (not sure if there is further caching). I wonder if it wouldn't make sense to introduce a script service for it. This could help in two ways:
An alternative to an extra cache could also be a way to simply get all shortcuts in the script service to avoid repeatedly calling the method when a larger number of shortcut values is needed. There was a problem hiding this comment.
Isn't the main reason for it to be cheaper that we avoid cloning the doc? AFAIK 1. is not an issue except for the performance gain described in 2.
👍 If we set up a script service, might as well make the most of it and make ourselves some shortcuts :) There was a problem hiding this comment. Talking about performance without having actual numbers (from profiling) is hard. It's a feeling of me that this could impact the performance, but it would be best to perform some actual profiling to see how bad it really is. I assume that adding a script service to get all shortcuts should already be a huge win. Adding a cache then adds quite some complexity that might not be warranted. A possibility could also be to cache shortcuts just for the current request in the context. There was a problem hiding this comment. When looking on making a script service, I found EDIT With manual testing, this seemed to work properly /EDIT This'd mean that there's still one There was a problem hiding this comment. Proposed the improvement in bbc63a0 . There was a problem hiding this comment. While a script service could certainly make things a bit cleaner, the performance should indeed be okay that way. The clone happens only once this way the first time an object is accessed. |
||
| #if("$!shortcutPreferenceObject"!='')$escapetool.javascript($escapetool.xml($shortcutPreferenceObject.value))## | ||
| #{else}$escapetool.javascript($escapetool.xml($services.localization.render($translationKey)))## | ||
Sereza7 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| #end | ||
| #end | ||
| #** | ||
| * Displays a submit button for the editor. This macro calls submitButton, | ||
| * composing all its parameters based on the action's identifier and the | ||
|
|
@@ -74,7 +89,9 @@ | |
| * @param class The class to use. | ||
| *# | ||
| #macro(editActionButton $action $resourceIdentifier $class) | ||
| #set ($translationKey = "core.shortcuts.edit.${resourceIdentifier}") | ||
| #set ($shortcut = "#getShortcutValue($translationKey)") | ||
| #submitButton("action_${action}", $shortcut, $services.localization.render($resourceIdentifier), $class) | ||
Sereza7 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| <input type="hidden" name="xaction" value="$escapetool.xml($action)" /> | ||
| #end | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,16 +131,16 @@ | |
| req.send(null); | ||
| }; | ||
|
|
||
| // Append developer shortcuts for toggling userType and hiddenDocuments in the current user profile | ||
| shortcut.add("#getShortcutValue('core.shortcuts.developer.user.type')", function() { | ||
| developerShortcutsRestCall("${request.contextPath}/rest/currentuser/properties/usertype/next", | ||
| "$escapetool.javascript($services.localization.render('core.shortcuts.developer.user.type.error'))"); | ||
| }, {'type': #if("$!xwiki.getDocument($xcontext.getUser()).getObject('XWiki.UserPreferenceShortcutsClass', 'translationKey', 'core.shortcuts.developer.user.type')"!='')shortcut.type.SIMPLE#{else}shortcut.type.SEQUENCE#end, 'disable_in_input': true }); | ||
|
There was a problem hiding this comment. This call seems quite expensive. See also my comment about performance above. There was a problem hiding this comment. Addressed partially in bbc63a0 There was a problem hiding this comment. Yes, as written above, this looks okay from a performance point of view. |
||
|
|
||
| shortcut.add("#getShortcutValue('core.shortcuts.developer.user.displayHiddenDocs')", function () { | ||
| developerShortcutsRestCall("${request.contextPath}/rest/currentuser/properties/displayHiddenDocuments/next", | ||
| "$escapetool.javascript($services.localization.render('core.shortcuts.developer.user.displayHiddenDocs.error'))"); | ||
| }, {'type': #if("$!xwiki.getDocument($xcontext.getUser()).getObject('XWiki.UserPreferenceShortcutsClass', 'translationKey', 'core.shortcuts.developer.user.displayHiddenDocs')"!='')shortcut.type.SIMPLE#{else}shortcut.type.SEQUENCE#end, 'disable_in_input': true }); | ||
| //]]> | ||
| #end | ||
| </script> | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this condition still correct? It isn't possible to set a shortcut value if
$extraShortcutis empty?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this in ac1e5f6 👍
Thank you for noticing, the logic here was off.