Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2877173002:
Add scale to settings-slider (Closed)
|
Created:
3 years, 7 months ago by stevenjb Modified:
3 years, 7 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd scale to settings-slider
We need to be able to represent a floating point preference with a
slider, but paper-slider only provides integer values, so we need to
provide a scale property that will scale the slider values but still
set the pref value correctly.
BUG=705816
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2877173002
Cr-Commit-Position: refs/heads/master@{#472147}
Committed: https://chromium.googlesource.com/chromium/src/+/38a267a5ba3177fd5b3913c3e90ab6c61fe272da
Patch Set 1 #Patch Set 2 : . #
Total comments: 16
Patch Set 3 : Feedback #Patch Set 4 : Change comment into code. #Patch Set 5 : Less code in the |scale| comment. #
Messages
Total messages: 30 (11 generated)
Description was changed from ========== Add scale to settings-slider We need to be able to represent a floating point preference with a slider, but paper-slider only provides integer values, so we need to provide a scale property that will scale the slider values but still set the pref value correctly. BUG=705816 ========== to ========== Add scale to settings-slider We need to be able to represent a floating point preference with a slider, but paper-slider only provides integer values, so we need to provide a scale property that will scale the slider values but still set the pref value correctly. BUG=705816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #2 (id:20001) has been deleted
stevenjb@chromium.org changed reviewers: + afakhry@chromium.org, jamescook@chromium.org, michaelpg@chromium.org
michaelpg@, please review afakhry@, jamescook@ FYI https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_private/prefs_util.cc (left): https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/prefs_util.cc:546: DCHECK_EQ(pref->GetType(), value->GetType()); I ran into this when testing. This DCHECK is bogus since we call value->GetAsDouble() for pref->GetType() == base::Value::Type::INTEGER on line 558.
lgtm. Thanks Steven! https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_private/prefs_util.cc (left): https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/prefs_util.cc:546: DCHECK_EQ(pref->GetType(), value->GetType()); On 2017/05/12 18:49:32, stevenjb wrote: > I ran into this when testing. This DCHECK is bogus since we call > value->GetAsDouble() for pref->GetType() == base::Value::Type::INTEGER on line > 558. Yes, that's the DCHECK that was causing me grief. https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_slider.js (right): https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_slider.js:68: newValue = sliderValue / this.scale; Is it possible to give |scale| a default value of 1 and do this scaling all the time?
https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_slider.js (right): https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_slider.js:68: newValue = sliderValue / this.scale; On 2017/05/12 19:12:13, afakhry wrote: > Is it possible to give |scale| a default value of 1 and do this scaling all the > time? It is, and I considered that, but this seemed slightly better to me since we do not apply it when tickValues is defined. That said, I could be convinced either way.
https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_slider.js (right): https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_slider.js:68: newValue = sliderValue / this.scale; On 2017/05/12 20:14:13, stevenjb wrote: > On 2017/05/12 19:12:13, afakhry wrote: > > Is it possible to give |scale| a default value of 1 and do this scaling all > the > > time? > > It is, and I considered that, but this seemed slightly better to me since we do > not apply it when tickValues is defined. That said, I could be convinced either > way. I'm fine with either way.
On 2017/05/12 23:56:11, afakhry wrote: > https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/controls/settings_slider.js (right): > > https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/controls/settings_slider.js:68: newValue = > sliderValue / this.scale; > On 2017/05/12 20:14:13, stevenjb wrote: > > On 2017/05/12 19:12:13, afakhry wrote: > > > Is it possible to give |scale| a default value of 1 and do this scaling all > > the > > > time? > > > > It is, and I considered that, but this seemed slightly better to me since we > do > > not apply it when tickValues is defined. That said, I could be convinced > either > > way. > > I'm fine with either way. Hi Steven, could you please land this CL, so that I can go ahead and rebase on top of it? Thanks!
On 2017/05/15 19:19:12, afakhry wrote: > On 2017/05/12 23:56:11, afakhry wrote: > > > https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... > > File chrome/browser/resources/settings/controls/settings_slider.js (right): > > > > > https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... > > chrome/browser/resources/settings/controls/settings_slider.js:68: newValue = > > sliderValue / this.scale; > > On 2017/05/12 20:14:13, stevenjb wrote: > > > On 2017/05/12 19:12:13, afakhry wrote: > > > > Is it possible to give |scale| a default value of 1 and do this scaling > all > > > the > > > > time? > > > > > > It is, and I considered that, but this seemed slightly better to me since we > > do > > > not apply it when tickValues is defined. That said, I could be convinced > > either > > > way. > > > > I'm fine with either way. > > Hi Steven, could you please land this CL, so that I can go ahead and rebase on > top of it? Thanks! I was planning to have Michael take a look at it first, he should be in today. michaelpg@ - ping :) In the meanwhile you can download this CL to a branch, run 'git cl issue 2877173002' in that branch, and run 'git cl upstream slider_branch' in your working branch and it should set this CL as a dependency for your CL so you can test it with these changes and we can review it.
I'm back today, so I can re-review whenever you're ready. https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_slider.js (right): https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_slider.js:27: * |scale| is applied to the slider value if |tickValues| is empty, the pref value is being scaled, not the slider value. |scale| is applied to the pref value, the inverse of |scale| is applied to the slider value :-\ https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_slider.js:68: newValue = sliderValue / this.scale; On 2017/05/12 20:14:13, stevenjb wrote: > On 2017/05/12 19:12:13, afakhry wrote: > > Is it possible to give |scale| a default value of 1 and do this scaling all > the > > time? > > It is, and I considered that, but this seemed slightly better to me since we do > not apply it when tickValues is defined. That said, I could be convinced either > way. Either way, I'd suggest an assert so it breaks if it's used incorrectly (e.g. if scale is set check that tickValues is unset or vice versa) https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_slider.js:89: if (this.scale) { in the same vein as afakhry's chomment, this may be easier to follow/maintain if |scale| is applied as an operation instead of setting the slider inside each branch. var val = /** {number} */(this.pref.value); if (this.scale) val *= this.scale; this.$.slider.value = val; https://codereview.chromium.org/2877173002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_slider_tests.js (right): https://codereview.chromium.org/2877173002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_slider_tests.js:116: slider.set('pref.value', 1); please add tests for: * tickValues being used, causing scale to be ignored or expecting an assert * a pref value other than 1. this test would pass if the element just sets the slider value to |this.scale|!
PTAL https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_slider.js (right): https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_slider.js:27: * |scale| is applied to the slider value if |tickValues| is empty, On 2017/05/15 19:53:18, michaelpg (back) wrote: > the pref value is being scaled, not the slider value. |scale| is applied to the > pref value, the inverse of |scale| is applied to the slider value :-\ No, it's the other way around, but I will clarify the comment :) https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_slider.js:68: newValue = sliderValue / this.scale; On 2017/05/15 19:53:18, michaelpg (back) wrote: > On 2017/05/12 20:14:13, stevenjb wrote: > > On 2017/05/12 19:12:13, afakhry wrote: > > > Is it possible to give |scale| a default value of 1 and do this scaling all > > the > > > time? > > > > It is, and I considered that, but this seemed slightly better to me since we > do > > not apply it when tickValues is defined. That said, I could be convinced > either > > way. > > Either way, I'd suggest an assert so it breaks if it's used incorrectly (e.g. if > scale is set check that tickValues is unset or vice versa) Done (assert placed in valueChanged where it is easier to add and called less frequently). https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_slider.js:89: if (this.scale) { On 2017/05/15 19:53:18, michaelpg (back) wrote: > in the same vein as afakhry's chomment, this may be easier to follow/maintain if > |scale| is applied as an operation instead of setting the slider inside each > branch. > > var val = /** {number} */(this.pref.value); > if (this.scale) val *= this.scale; > this.$.slider.value = val; > Set default scale to 1. https://codereview.chromium.org/2877173002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_slider_tests.js (right): https://codereview.chromium.org/2877173002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_slider_tests.js:116: slider.set('pref.value', 1); On 2017/05/15 19:53:18, michaelpg (back) wrote: > please add tests for: > > * tickValues being used, causing scale to be ignored or expecting an assert > * a pref value other than 1. this test would pass if the element just sets the > slider value to |this.scale|! 1. I'm not a fan of testing asserts (now that we have one). 2. Done.
https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_slider.js (right): https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_slider.js:27: * |scale| is applied to the slider value if |tickValues| is empty, On 2017/05/15 21:45:09, stevenjb wrote: > On 2017/05/15 19:53:18, michaelpg (back) wrote: > > the pref value is being scaled, not the slider value. |scale| is applied to > the > > pref value, the inverse of |scale| is applied to the slider value :-\ > > No, it's the other way around, but I will clarify the comment :) > it's still confusing -- I think we're reading it in opposite ways which are both valid. I'm thinking in terms of "The slider value is divided" (to calculate the new pref value), or "The pref value will be multiplied by |scale|" (to determine the slider value). You're thinking in terms of "The slider value will be the result of the pref value multiplied by |scale|." Can I suggest avoiding the confusing English bits? * The slider UI will reflect |pref.value| * |scale|. * When the slider changes, the pref will be set to |slider.value| / |scale|.
Did you have any non comment (i.e nit) concerns? PTAL. https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_slider.js (right): https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_slider.js:27: * |scale| is applied to the slider value if |tickValues| is empty, On 2017/05/15 22:14:32, michaelpg (back) wrote: > On 2017/05/15 21:45:09, stevenjb wrote: > > On 2017/05/15 19:53:18, michaelpg (back) wrote: > > > the pref value is being scaled, not the slider value. |scale| is applied to > > the > > > pref value, the inverse of |scale| is applied to the slider value :-\ > > > > No, it's the other way around, but I will clarify the comment :) > > > > it's still confusing -- I think we're reading it in opposite ways which are both > valid. > > I'm thinking in terms of "The slider value is divided" (to calculate the new > pref value), or "The pref value will be multiplied by |scale|" (to determine the > slider value). You're thinking in terms of "The slider value will be the result > of the pref value multiplied by |scale|." > > Can I suggest avoiding the confusing English bits? > > * The slider UI will reflect |pref.value| * |scale|. > * When the slider changes, the pref will be set to |slider.value| / |scale|. Dude, the code says: slider.value = pref.value * scale. I don't know how that can be more clear - the slider value, i.e. slider.value, is multiplied by scale. I'll change the comment to use slider.value I guess.
The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/15 22:24:00, stevenjb wrote: > Did you have any non comment (i.e nit) concerns? > PTAL. The comment documents part of the element's public interface, so it's worth more than a nit to me. Unless there's a rule that comment fixes are always nits. The slider value is not multiplied by scale. The comment says that it is. It's wrong and it suggests usage that doesn't work. I suggested a fix but I'm fine with any wording that isn't contradicted by code. https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_slider.js (right): https://codereview.chromium.org/2877173002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_slider.js:27: * |scale| is applied to the slider value if |tickValues| is empty, On 2017/05/15 22:24:00, stevenjb wrote: > On 2017/05/15 22:14:32, michaelpg (back) wrote: > > On 2017/05/15 21:45:09, stevenjb wrote: > > > On 2017/05/15 19:53:18, michaelpg (back) wrote: > > > > the pref value is being scaled, not the slider value. |scale| is applied > to > > > the > > > > pref value, the inverse of |scale| is applied to the slider value :-\ > > > > > > No, it's the other way around, but I will clarify the comment :) > > > > > > > it's still confusing -- I think we're reading it in opposite ways which are > both > > valid. > > > > I'm thinking in terms of "The slider value is divided" (to calculate the new > > pref value), or "The pref value will be multiplied by |scale|" (to determine > the > > slider value). You're thinking in terms of "The slider value will be the > result > > of the pref value multiplied by |scale|." > > > > Can I suggest avoiding the confusing English bits? > > > > * The slider UI will reflect |pref.value| * |scale|. > > * When the slider changes, the pref will be set to |slider.value| / |scale|. > > Dude, the code says: > > slider.value = pref.value * scale. What term is multiplied by scale in this equation? > > I don't know how that can be more clear - the slider value, i.e. slider.value, > is multiplied by scale. I'll change the comment to use slider.value I guess. > The slider value is not multiplied by scale. Not being pedantic here, it's just not. If you have a pref value of 2 and a scale of 10, 2 is multiplied by 10 to create the slider value, 20. If you were to multiply the slider value, i.e. 20, by scale, you'd get 200. That's not what the code is doing.
On Mon, May 15, 2017 at 4:55 PM, <michaelpg@chromium.org> wrote: > On 2017/05/15 22:24:00, stevenjb wrote: > > Did you have any non comment (i.e nit) concerns? > > PTAL. > > The comment documents part of the element's public interface, so it's > worth more > than a nit to me. Unless there's a rule that comment fixes are always nits. > > Sure but... I apparently underestimated the level of confusion here (and Ahmed wasn't confused). Not saying your perspective isn't valid, just... there is no such thing as a perfect comment. I'm thinking of replacing it with "see code"... > The slider value is not multiplied by scale. The comment says that it is. > It's > wrong and it suggests usage that doesn't work. I suggested a fix but I'm > fine > with any wording that isn't contradicted by code. > > I clearly was not explicit enough. I had the assumption "slider.value = f(pref.value)" mapped a bit to concretely in my head I guess, so when I say "is mulitpilied by", the "pref.valuye" part seemed clear to me, but apparently it is not :) > > https://codereview.chromium.org/2877173002/diff/40001/ > chrome/browser/resources/settings/controls/settings_slider.js > File chrome/browser/resources/settings/controls/settings_slider.js > (right): > > https://codereview.chromium.org/2877173002/diff/40001/ > chrome/browser/resources/settings/controls/settings_slider.js#newcode27 > chrome/browser/resources/settings/controls/settings_slider.js:27: * > |scale| is applied to the slider value if |tickValues| is empty, > On 2017/05/15 22:24:00, stevenjb wrote: > > On 2017/05/15 22:14:32, michaelpg (back) wrote: > > > On 2017/05/15 21:45:09, stevenjb wrote: > > > > On 2017/05/15 19:53:18, michaelpg (back) wrote: > > > > > the pref value is being scaled, not the slider value. |scale| is > applied > > to > > > > the > > > > > pref value, the inverse of |scale| is applied to the slider > value :-\ > > > > > > > > No, it's the other way around, but I will clarify the comment :) > > > > > > > > > > it's still confusing -- I think we're reading it in opposite ways > which are > > both > > > valid. > > > > > > I'm thinking in terms of "The slider value is divided" (to calculate > the new > > > pref value), or "The pref value will be multiplied by |scale|" (to > determine > > the > > > slider value). You're thinking in terms of "The slider value will be > the > > result > > > of the pref value multiplied by |scale|." > > > > > > Can I suggest avoiding the confusing English bits? > > > > > > * The slider UI will reflect |pref.value| * |scale|. > > > * When the slider changes, the pref will be set to |slider.value| / > |scale|. > > > > Dude, the code says: > > > > slider.value = pref.value * scale. > > What term is multiplied by scale in this equation? > > The *result* is slider.value. Again, different assumptions. > > > > I don't know how that can be more clear - the slider value, i.e. > slider.value, > > is multiplied by scale. I'll change the comment to use slider.value I > guess. > > > > The slider value is not multiplied by scale. Not being pedantic here, > it's just not. > > The result really really is :) But you're right, it's really 'the slider value = the pref value multiplied by scale'.. > If you have a pref value of 2 and a scale of 10, 2 is multiplied by 10 > to create the slider value, 20. If you were to multiply the slider > value, i.e. 20, by scale, you'd get 200. That's not what the code is > doing. > > The slider value is (the pref value) mulitplied by scale :) -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Back to first principles: Don't explain the code in the comment. PTAL
On 2017/05/15 23:23:23, stevenjb wrote: > Back to first principles: Don't explain the code in the comment. > PTAL * Don't *describe* the code in the comment.
lgtm
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2877173002/#ps100001 (title: "Less code in the |scale| comment.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494950701647740,
"parent_rev": "60b0f2c0bbf2fbb96aa09a781f5adac7981832df", "commit_rev":
"38a267a5ba3177fd5b3913c3e90ab6c61fe272da"}
Message was sent while issue was closed.
Description was changed from ========== Add scale to settings-slider We need to be able to represent a floating point preference with a slider, but paper-slider only provides integer values, so we need to provide a scale property that will scale the slider values but still set the pref value correctly. BUG=705816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add scale to settings-slider We need to be able to represent a floating point preference with a slider, but paper-slider only provides integer values, so we need to provide a scale property that will scale the slider values but still set the pref value correctly. BUG=705816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2877173002 Cr-Commit-Position: refs/heads/master@{#472147} Committed: https://chromium.googlesource.com/chromium/src/+/38a267a5ba3177fd5b3913c3e90a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/38a267a5ba3177fd5b3913c3e90a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
