Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
393023006:
Add settings for keyboard auto-repeat to options page. (Closed)
|
Created:
6 years, 5 months ago by michaelpg Modified:
6 years, 5 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, nkostylev+watch_chromium.org, yukishiino+watch_chromium.org, asvitkine+watch_chromium.org, nona+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, yusukes+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd settings for keyboard auto-repeat to options page.
BUG=219446
R=satorux@chromium.org,dbeam@chromium.org,asvitkine@chromium.org
satorux@: c/b/chromeos/preferences.cc
dbeam@: c/b/resources/options
c/b/ui/webui/options
asvitkine@: actions.xml
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285447
Patch Set 1 : #
Total comments: 35
Patch Set 2 : Use PrefRange and align sliders #
Total comments: 5
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : rebase #Messages
Total messages: 30 (0 generated)
https://codereview.chromium.org/393023006/diff/40001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/393023006/diff/40001/chrome/common/pref_names... chrome/common/pref_names.cc:572: // current values with the old synced values, so we continue to use this suffix. Would it be better to change the preference names again? :-\
https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... File chrome/browser/resources/options/chromeos/keyboard_overlay.css (right): https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.css:10: margin-left: 30px; -webkit-margin-start https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... File chrome/browser/resources/options/chromeos/keyboard_overlay.js (right): https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.js:35: * @type {Array} !Array.<number> https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.js:38: var kAutoRepeatDelays = kAutoRepeatDelays => AUTO_REPEAT_DELAYS https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.js:45: * @type {Array} !Array.<number> https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.js:48: var kAutoRepeatIntervals = AUTO_REPEAT_INTERVALS https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.js:126: options.SettingsDialog.prototype.handleConfirm.call(this); nit: \n https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.js:128: return; nit: \n https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.js:129: if (this.autoRepeatDelayChanged_) { can we just remove this.*Changed_ and send the prefs every time? https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.js:136: } nit: \n https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.js:156: section.classList.add('disabled'); section.classList.toggle('disabled', e.value.value); https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.js:169: if (index != -1) can this be an assert() instead? https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.js:181: if (index != -1) same https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.js:221: * @param {number} target The value to find. nit: s/target/value https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.js:222: * @param {Array} arr The array to search. nit: s/arr/values https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/browser/re... chrome/browser/resources/options/chromeos/keyboard_overlay.js:222: * @param {Array} arr The array to search. !Array.<number> https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/common/pre... File chrome/common/pref_names.cc (right): https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/common/pre... chrome/common/pref_names.cc:570: // preferences to not be user-configurable or sync with cloud. The prefs are sync with the cloud? https://chromiumcodereview.appspot.com/393023006/diff/40001/chrome/common/pre... chrome/common/pref_names.cc:572: // current values with the old synced values, so we continue to use this suffix. On 2014/07/16 01:51:24, Michael Giuffrida wrote: > Would it be better to change the preference names again? :-\ not sure. if we're not losing any loser data by keeping the same name, i guess it's worth considering.
https://codereview.chromium.org/393023006/diff/40001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/393023006/diff/40001/chrome/common/pref_names... chrome/common/pref_names.cc:572: // current values with the old synced values, so we continue to use this suffix. On 2014/07/16 16:59:46, Dan Beam wrote: > On 2014/07/16 01:51:24, Michael Giuffrida wrote: > > Would it be better to change the preference names again? :-\ > > not sure. if we're not losing any loser data by keeping the same name, i guess > it's worth considering. I would use the current _r2 names so that users do not have any existing settings changed unexpectedly.
In fixing these issue I've noticed some slightly buggy behavior around the sliders, since they aren't using the commit/rollback scheme that dialog-prefs do. I guess I reinvented the wheel a bit. PrefRange in pref_ui.js has the behavior I want and even lets me specify a valueMap to map indices to particular values. But it doesn't have my function to choose a nearby value if the preference is set to something not found in the map. For instance, if my map is [5000, 3000, 1000] but the pref value is 1500, the slider value will be set to -1 (which is adjusted to 0, representing 5000 in my value map). PrefRange has an optional customChangeHandler property it calls when the user updates the range instead of setting the pref directly. Would it be reasonable to add the reverse function, an optional callback to use when the preference changes instead of updating the range directly? Or would it make sense to add my "find-the-nearest-value" logic to PrefRange itself?
On 2014/07/17 01:15:57, Michael Giuffrida wrote: > In fixing these issue I've noticed some slightly buggy behavior around the > sliders, since they aren't using the commit/rollback scheme that dialog-prefs > do. I guess I reinvented the wheel a bit. PrefRange in pref_ui.js has the > behavior I want and even lets me specify a valueMap to map indices to particular > values. > > But it doesn't have my function to choose a nearby value if the preference is > set to something not found in the map. For instance, if my map is [5000, 3000, > 1000] but the pref value is 1500, the slider value will be set to -1 (which is > adjusted to 0, representing 5000 in my value map). > > PrefRange has an optional customChangeHandler property it calls when the user > updates the range instead of setting the pref directly. Would it be reasonable > to add the reverse function, an optional callback to use when the preference > changes instead of updating the range directly? > > Or would it make sense to add my "find-the-nearest-value" logic to PrefRange > itself? I think the optional callback on change makes the most sense?
Two changes: 1. Actually use PrefRange instead of re-inventing the wheel. Add a customPrefChangeHandler to allow for preference values that don't fit in the slider's buckets. 2. Align the sliders. See bug for screenshot. Thanks. https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/keyboard_overlay.css (right): https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/keyboard_overlay.css:10: margin-left: 30px; On 2014/07/16 16:59:45, Dan Beam wrote: > -webkit-margin-start Done. https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/keyboard_overlay.js (right): https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/keyboard_overlay.js:35: * @type {Array} On 2014/07/16 16:59:46, Dan Beam wrote: > !Array.<number> Done. https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/keyboard_overlay.js:38: var kAutoRepeatDelays = On 2014/07/16 16:59:46, Dan Beam wrote: > kAutoRepeatDelays => AUTO_REPEAT_DELAYS Done. https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/keyboard_overlay.js:45: * @type {Array} On 2014/07/16 16:59:46, Dan Beam wrote: > !Array.<number> Done. https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/keyboard_overlay.js:48: var kAutoRepeatIntervals = On 2014/07/16 16:59:45, Dan Beam wrote: > AUTO_REPEAT_INTERVALS Done. https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/keyboard_overlay.js:126: options.SettingsDialog.prototype.handleConfirm.call(this); On 2014/07/16 16:59:45, Dan Beam wrote: > nit: \n Removed code. https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/keyboard_overlay.js:128: return; On 2014/07/16 16:59:45, Dan Beam wrote: > nit: \n Removed code. https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/keyboard_overlay.js:129: if (this.autoRepeatDelayChanged_) { On 2014/07/16 16:59:45, Dan Beam wrote: > can we just remove this.*Changed_ and send the prefs every time? No, but now we're using PrefRange which works the same way -- the preference is only committed if the input is changed. For clarity, my answer to the original question is below: ------------- In normal use, the prefs starts out with the default values (which are valid options for these sliders), and can only be changed using these sliders, so it's safe to always update the preference. On the other hand, there are circumstances where this doesn't hold. In Dev mode, I can manually set a different repeat delay/interval that doesn't correspond to one of the slider's notches. Coming here to change function key settings would then also force a change of the auto-repeat settings, coercing them to whichever value in the const array is closest. Similarly, if we decide to change these values or introduce a new default, a user's preference -- and what options are valid here -- could be out of sync across multiple versions of Chrome. Again it makes sense to me that the values represented by the sliders should only be forced if the user actually touches the slider. Neither of those are compelling arguments for adding these *Changed_ flags, but if I get used to a particular repeat rate I wouldn't want it to change on me without my input/knowledge. https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/keyboard_overlay.js:136: } On 2014/07/16 16:59:46, Dan Beam wrote: > nit: \n Removed code. https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/keyboard_overlay.js:156: section.classList.add('disabled'); On 2014/07/16 16:59:45, Dan Beam wrote: > section.classList.toggle('disabled', e.value.value); Done. https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/keyboard_overlay.js:169: if (index != -1) On 2014/07/16 16:59:46, Dan Beam wrote: > can this be an assert() instead? Done. https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/keyboard_overlay.js:181: if (index != -1) On 2014/07/16 16:59:46, Dan Beam wrote: > same Done. https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/keyboard_overlay.js:221: * @param {number} target The value to find. On 2014/07/16 16:59:45, Dan Beam wrote: > nit: s/target/value Done. https://codereview.chromium.org/393023006/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/keyboard_overlay.js:222: * @param {Array} arr The array to search. On 2014/07/16 16:59:45, Dan Beam wrote: > !Array.<number> Done and done. https://codereview.chromium.org/393023006/diff/40001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/393023006/diff/40001/chrome/common/pref_names... chrome/common/pref_names.cc:570: // preferences to not be user-configurable or sync with cloud. The prefs are On 2014/07/16 16:59:46, Dan Beam wrote: > sync with the cloud? Done. https://codereview.chromium.org/393023006/diff/40001/chrome/common/pref_names... chrome/common/pref_names.cc:572: // current values with the old synced values, so we continue to use this suffix. On 2014/07/16 17:41:46, stevenjb wrote: > On 2014/07/16 16:59:46, Dan Beam wrote: > > On 2014/07/16 01:51:24, Michael Giuffrida wrote: > > > Would it be better to change the preference names again? :-\ > > > > not sure. if we're not losing any loser data by keeping the same name, i > guess > > it's worth considering. > > I would use the current _r2 names so that users do not have any existing > settings changed unexpectedly. > Acknowledged. > loser data ;-)
nifty, thanks for adding to the platform! lgtm https://codereview.chromium.org/393023006/diff/170001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/keyboard_overlay.css (right): https://codereview.chromium.org/393023006/diff/170001/chrome/browser/resource... chrome/browser/resources/options/chromeos/keyboard_overlay.css:25: margin-right: 5px; nit: margin: 0 5px; https://codereview.chromium.org/393023006/diff/170001/chrome/browser/resource... chrome/browser/resources/options/chromeos/keyboard_overlay.css:27: nit: remove \n
https://codereview.chromium.org/393023006/diff/170001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/keyboard_overlay.css (right): https://codereview.chromium.org/393023006/diff/170001/chrome/browser/resource... chrome/browser/resources/options/chromeos/keyboard_overlay.css:25: margin-right: 5px; On 2014/07/19 00:57:09, Dan Beam wrote: > nit: margin: 0 5px; the user-agent stylesheet has a 2px margin, will this override that? should I just explicitly use "margin: 2px 5px"?
https://codereview.chromium.org/393023006/diff/170001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/keyboard_overlay.css (right): https://codereview.chromium.org/393023006/diff/170001/chrome/browser/resource... chrome/browser/resources/options/chromeos/keyboard_overlay.css:25: margin-right: 5px; On 2014/07/19 00:58:28, Michael Giuffrida wrote: > On 2014/07/19 00:57:09, Dan Beam wrote: > > nit: margin: 0 5px; > > the user-agent stylesheet has a 2px margin, will this override that? should I > just explicitly use "margin: 2px 5px"? eh, just ignore then
The CQ bit was checked by michaelpg@chromium.org
The CQ bit was unchecked by michaelpg@chromium.org
Addressed Dan's comments. Accidentally checked "commit", but not done with review :-) https://codereview.chromium.org/393023006/diff/170001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/keyboard_overlay.css (right): https://codereview.chromium.org/393023006/diff/170001/chrome/browser/resource... chrome/browser/resources/options/chromeos/keyboard_overlay.css:27: On 2014/07/19 00:57:09, Dan Beam wrote: > nit: remove \n Done.
https://codereview.chromium.org/393023006/diff/210001/chrome/browser/resource... File chrome/browser/resources/options/pref_ui.js (right): https://codereview.chromium.org/393023006/diff/210001/chrome/browser/resource... chrome/browser/resources/options/pref_ui.js:200: if (!this.customPrefChangeHandler(event)) { nit: Here and below, I think the logic would be a tiny bit more clear using an early exit.
lgtm
https://codereview.chromium.org/393023006/diff/210001/chrome/browser/resource... File chrome/browser/resources/options/pref_ui.js (right): https://codereview.chromium.org/393023006/diff/210001/chrome/browser/resource... chrome/browser/resources/options/pref_ui.js:200: if (!this.customPrefChangeHandler(event)) { On 2014/07/22 16:42:23, stevenjb wrote: > nit: Here and below, I think the logic would be a tiny bit more clear using an > early exit. Done.
Ping (satorux, asvitkine): we'd like to get this in sooner rather than later. Thanks!
histograms lgtm
lgtm
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelpg@chromium.org/393023006/230001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by michaelpg@chromium.org
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelpg@chromium.org/393023006/250001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelpg@chromium.org/393023006/250001
Message was sent while issue was closed.
Change committed as 285447 |
