r101605 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r101604‎ | r101605 | r101606 >
Date:09:34, 2 November 2011
Author:santhosh
Status:ok (Comments)
Tags:
Comment:
Remove the option to have configurable shortcut key. It wont work in all browsers/os combinations.
Ref Bug 31026
i18n #180
Modified paths:
  • /trunk/extensions/Narayam/Narayam.hooks.php (modified) (history)
  • /trunk/extensions/Narayam/Narayam.php (modified) (history)
  • /trunk/extensions/Narayam/js/ext.narayam.core.js (modified) (history)

Diff [purge]

Index: trunk/extensions/Narayam/js/ext.narayam.core.js
@@ -35,14 +35,8 @@
3636 var allImes = mw.config.get( 'wgNarayamAllSchemes' ) || {};
3737 // Currently selected scheme
3838 var currentScheme = null;
39 - // Shortcut key for turning Narayam on and off
40 - var shortcutKey = mw.config.get( 'wgNarayamShortcutKey' ) || {
41 - altKey: false,
42 - ctrlKey: false,
43 - shiftKey: false,
44 - key: null
45 - };
4639
 40+
4741 /* Private functions */
4842
4943 /**
@@ -118,10 +112,8 @@
119113 * @return bool
120114 */
121115 function isShortcutKey( e ) {
122 - return e.altKey == shortcutKey.altKey &&
123 - e.ctrlKey == shortcutKey.ctrlKey &&
124 - e.shiftKey == shortcutKey.shiftKey &&
125 - String.fromCharCode( e.which ).toLowerCase() == shortcutKey.key.toLowerCase();
 116+ return e.ctrlKey &&
 117+ String.fromCharCode( e.which ).toLowerCase() == 'm';
126118 }
127119
128120 /**
@@ -129,18 +121,8 @@
130122 * @return string
131123 */
132124 function shortcutText() {
133 - var text = '';
134 - // TODO: Localize these things (in core, too)
135 - if ( shortcutKey.ctrlKey ) {
136 - text += 'Ctrl-';
137 - }
138 - if ( shortcutKey.shiftKey ) {
139 - text += 'Shift-';
140 - }
141 - if ( shortcutKey.altKey ) {
142 - text += 'Alt-';
143 - }
144 - text += shortcutKey.key.toUpperCase();
 125+ var text = 'Ctrl-M';
 126+ // TODO: Address Bug #31026
145127 return text;
146128 }
147129
Index: trunk/extensions/Narayam/Narayam.php
@@ -32,14 +32,6 @@
3333 // Whether the input method should be active as default or not
3434 $wgNarayamEnabledByDefault = true;
3535
36 -// Shortcut key for enabling and disabling Narayam
37 -// Defaults to Ctrl+M
38 -$wgNarayamShortcutKey = array(
39 - 'altKey' => false,
40 - 'ctrlKey' => true,
41 - 'shiftKey' => false,
42 - 'key' => 'm'
43 -);
4436
4537 // Array mapping language codes and scheme names to module names
4638 // Custom schemes can be added here
Index: trunk/extensions/Narayam/Narayam.hooks.php
@@ -25,7 +25,7 @@
2626 }
2727
2828 public static function addConfig( &$vars ) {
29 - global $wgNarayamEnabledByDefault, $wgNarayamShortcutKey, $wgUser;
 29+ global $wgNarayamEnabledByDefault, $wgUser;
3030
3131 if ( $wgUser->getOption( 'narayamDisable' ) ) {
3232 // User disabled Narayam
@@ -33,7 +33,6 @@
3434 }
3535
3636 $vars['wgNarayamEnabledByDefault'] = $wgNarayamEnabledByDefault;
37 - $vars['wgNarayamShortcutKey'] = $wgNarayamShortcutKey;
3837
3938 return true;
4039 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r101790Fix the shortcut key of Narayam in OSX and Firefox, Opera browsers....santhosh05:54, 3 November 2011

Comments

#Comment by Junaidpv (talk | contribs)   08:51, 8 November 2011

In my opinion, this should not have removed.

Consider Google products like GMail which successfully set several shortcuts, suppressing browser's native ones. We should improve the code rather than removing the feature.

#Comment by Santhosh.thottingal (talk | contribs)   09:06, 8 November 2011

We did not remove the shortcut. We removed the option of "configurable shortcut". Google or gmail does not allow you to set your own keyboard shortcut. We still have shortcut, which is CONTROL+M in most of the browsers. In OSX, it is Control+g for FF and Control+Command+M for Opera.

Status & tagging log