Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2600213002:
Adds animated touch point and the hint box for touch calibration UX (Closed)
|
Created:
3 years, 11 months ago by malaykeshav Modified:
3 years, 11 months ago CC:
chromium-reviews, davemoore+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds animated touch point and the hint box for touch calibration UX
- Adds the circular throbbing touch point view that the user interacts
with during touch calibration.
- Adds the hint box view that contains the hint message as given in the
UX specs.
(https://drive.google.com/file/d/0B_2Uyb2Rhx2OU0FIbXUyMkhMZlE/view)
- Adds required resource strings for hint box messages.
Working protottpe(The video is the entire prototype. This change only
involves adding the touch point and hint box.):
https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWdUVFNSM0F5WDF4YlBn/view
Screenshot of how it looks on different DPI screens:
https://screenshot.googleplex.com/ND5d9v0799y.png
Screenshot for RTL:
https://screenshot.googleplex.com/Xd3PyAYbDTV.png
BUG=634166
COMPONENT=Touch Calibration, ChromeOS, UI Strings
Review-Url: https://codereview.chromium.org/2600213002
Cr-Commit-Position: refs/heads/master@{#443000}
Committed: https://chromium.googlesource.com/chromium/src/+/2f631d1860f7dfcf100b5a6bef46263cd7a4e698
Patch Set 1 : Adds animated touch point and the hint box for touch calibration UX #
Total comments: 22
Patch Set 2 : Resolving comments #Patch Set 3 : Hint box now updates size based on the label and sublabel #
Total comments: 2
Patch Set 4 : Resolving comments #
Messages
Total messages: 40 (22 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by malaykeshav@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
malaykeshav@chromium.org changed reviewers: + stevenjb@chromium.org
Addition of animated touch point and hint box to the touch calibration UX. PTAL
https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:71: } while (width <= bounds.width() && height <= bounds.height()); Is this guaranteed to exit? It seems like if font_size were to reach the maximum font size it might not? We might want to ensure that width and height change. https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:217: int left_offset = width() * 0.08f; Does this work as expected with RTL?
Also, it's been a long time now since I have worked in Views, it would be good to add oshima@ or someone with more recent Views experience as a reviewer on this.
malaykeshav@chromium.org changed reviewers: + oshima@chromium.org
Adding @oshima PTAL
https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:62: int GetFontSizeForRect(gfx::Rect bounds, base::string16 text) { a few questions. 1) Why do we want to use fixed size, instead of adjusting size for target language? 2) You need to consider the case that it didn't fit for certain language. (basically same as steven's comment) https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:107: const int max_outer_radius_; It's probably better to say "smallest/largest size of the animated growing circle. https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:126: min_outer_radius_(3.f * width / 8.f), can you define a constant for this factor against max? https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:180: explicit HintBox(const gfx::Rect& bounds, int border_radius); no explicit https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:222: height() * 0.11f); Can you use Inset()? https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h (right): https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:127: HintBox* hint_box_view_; looks like these views can also be just views::View*
The CQ bit was checked by malaykeshav@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...
Resolved comments. The text now has DIP size and works with RTL. Screenshot of how it looks on different DPI screens: https://screenshot.googleplex.com/ND5d9v0799y.png Screenshot for RTL: https://screenshot.googleplex.com/Xd3PyAYbDTV.png PTAL https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:62: int GetFontSizeForRect(gfx::Rect bounds, base::string16 text) { On 2017/01/05 23:29:04, oshima wrote: > a few questions. > 1) Why do we want to use fixed size, instead of adjusting size for target > language? > 2) You need to consider the case that it didn't fit for certain language. > (basically same as steven's comment) Font size is now fixed along with the bounds which is now DIP constant. https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:71: } while (width <= bounds.width() && height <= bounds.height()); On 2016/12/28 01:04:50, stevenjb wrote: > Is this guaranteed to exit? It seems like if font_size were to reach the maximum > font size it might not? We might want to ensure that width and height change. Removed. No longer required. https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:107: const int max_outer_radius_; On 2017/01/05 23:29:04, oshima wrote: > It's probably better to say "smallest/largest size of the animated growing > circle. Done https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:126: min_outer_radius_(3.f * width / 8.f), On 2017/01/05 23:29:04, oshima wrote: > can you define a constant for this factor against max? Done https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:180: explicit HintBox(const gfx::Rect& bounds, int border_radius); On 2017/01/05 23:29:04, oshima wrote: > no explicit Done https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:217: int left_offset = width() * 0.08f; On 2016/12/28 01:04:50, stevenjb wrote: > Does this work as expected with RTL? The updated code works with RTL. https://screenshot.googleplex.com/Xd3PyAYbDTV.png https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:222: height() * 0.11f); On 2017/01/05 23:29:04, oshima wrote: > Can you use Inset()? Done. https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h (right): https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:127: HintBox* hint_box_view_; On 2017/01/05 23:29:04, oshima wrote: > looks like these views can also be just views::View* I need a rounded rectangle for the hint box. Would that be possible with just a views::View* ?
Description was changed from ========== Adds animated touch point and the hint box for touch calibration UX - Adds the circular throbbing touch point view that the user interacts with during touch calibration. - Adds the hint box view that contains the hint message as given in the UX specs. (https://drive.google.com/file/d/0B_2Uyb2Rhx2OU0FIbXUyMkhMZlE/view) - Adds required resource strings for hint box messages. Working protottpe(The video is the entire prototype. This change only involves adding the touch point and hint box.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... BUG=634166 COMPONENT=Touch Calibration, ChromeOS, UI Strings ========== to ========== Adds animated touch point and the hint box for touch calibration UX - Adds the circular throbbing touch point view that the user interacts with during touch calibration. - Adds the hint box view that contains the hint message as given in the UX specs. (https://drive.google.com/file/d/0B_2Uyb2Rhx2OU0FIbXUyMkhMZlE/view) - Adds required resource strings for hint box messages. Working protottpe(The video is the entire prototype. This change only involves adding the touch point and hint box.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... Screenshot of how it looks on different DPI screens: https://screenshot.googleplex.com/ND5d9v0799y.png Screenshot for RTL: https://screenshot.googleplex.com/Xd3PyAYbDTV.png BUG=634166 COMPONENT=Touch Calibration, ChromeOS, UI Strings ==========
https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:222: height() * 0.11f); On 2017/01/09 18:59:40, malaykeshav wrote: > On 2017/01/05 23:29:04, oshima wrote: > > Can you use Inset()? > > Done. My apologies, I misread the code. SetRect looks ok but keep label_height. I guess 4* is actually 2*? https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h (right): https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:127: HintBox* hint_box_view_; On 2017/01/09 18:59:41, malaykeshav wrote: > On 2017/01/05 23:29:04, oshima wrote: > > looks like these views can also be just views::View* > > I need a rounded rectangle for the hint box. Would that be possible with just a > views::View* ? You can do that when you create the view, not later right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:40001) has been deleted
PTAL https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:222: height() * 0.11f); On 2017/01/09 19:44:16, oshima wrote: > On 2017/01/09 18:59:40, malaykeshav wrote: > > On 2017/01/05 23:29:04, oshima wrote: > > > Can you use Inset()? > > > > Done. > > My apologies, I misread the code. SetRect looks ok but keep label_height. > I guess 4* is actually 2*? > > Done. https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h (right): https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:127: HintBox* hint_box_view_; On 2017/01/09 at 19:44:16, oshima wrote: > On 2017/01/09 18:59:41, malaykeshav wrote: > > On 2017/01/05 23:29:04, oshima wrote: > > > looks like these views can also be just views::View* > > > > I need a rounded rectangle for the hint box. Would that be possible with just a > > views::View* ? > > You can do that when you create the view, not later right? Done
The CQ bit was checked by malaykeshav@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:62: int GetFontSizeForRect(gfx::Rect bounds, base::string16 text) { On 2017/01/09 18:59:41, malaykeshav wrote: > On 2017/01/05 23:29:04, oshima wrote: > > a few questions. > > 1) Why do we want to use fixed size, instead of adjusting size for target > > language? > > 2) You need to consider the case that it didn't fit for certain language. > > (basically same as steven's comment) > > Font size is now fixed along with the bounds which is now DIP constant. I thought we're going to adjust the bounds using GetPreferredSize() ?
On 2017/01/10 at 17:11:18, oshima wrote: > https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): > > https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:62: int GetFontSizeForRect(gfx::Rect bounds, base::string16 text) { > On 2017/01/09 18:59:41, malaykeshav wrote: > > On 2017/01/05 23:29:04, oshima wrote: > > > a few questions. > > > 1) Why do we want to use fixed size, instead of adjusting size for target > > > language? > > > 2) You need to consider the case that it didn't fit for certain language. > > > (basically same as steven's comment) > > > > Font size is now fixed along with the bounds which is now DIP constant. > > I thought we're going to adjust the bounds using GetPreferredSize() ? Done
Hint box now updates size based on the label and sublabel. PTAL https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2600213002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:62: int GetFontSizeForRect(gfx::Rect bounds, base::string16 text) { On 2017/01/10 at 17:11:18, oshima wrote: > On 2017/01/09 18:59:41, malaykeshav wrote: > > On 2017/01/05 23:29:04, oshima wrote: > > > a few questions. > > > 1) Why do we want to use fixed size, instead of adjusting size for target > > > language? > > > 2) You need to consider the case that it didn't fit for certain language. > > > (basically same as steven's comment) > > > > Font size is now fixed along with the bounds which is now DIP constant. > > I thought we're going to adjust the bounds using GetPreferredSize() ? Done
Patchset #3 (id:80001) has been deleted
lgtm for this change. I think the calibration process should be canceled when the display metrics changes. Is it handled? https://codereview.chromium.org/2600213002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2600213002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:74: const gfx::Size GetSizeForString(base::string16 text, gfx::FontList font_list) { const for return value doesn't make much sense. Please use const & for the arguments.
> I think the calibration process should be canceled when the display metrics changes. Is it handled? Yes this is being handled already in https://cs.chromium.org/chromium/src/chrome/browser/chromeos/display/touch_ca...
https://codereview.chromium.org/2600213002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2600213002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:74: const gfx::Size GetSizeForString(base::string16 text, gfx::FontList font_list) { On 2017/01/11 18:17:14, oshima wrote: > const for return value doesn't make much sense. > > Please use const & for the arguments. Done.
The CQ bit was checked by malaykeshav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2600213002/#ps120001 (title: "Resolving comments")
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": 120001, "attempt_start_ts": 1484160455298790,
"parent_rev": "8586921e08baafc1209e66223d0642d6246094b9", "commit_rev":
"2f631d1860f7dfcf100b5a6bef46263cd7a4e698"}
Message was sent while issue was closed.
Description was changed from ========== Adds animated touch point and the hint box for touch calibration UX - Adds the circular throbbing touch point view that the user interacts with during touch calibration. - Adds the hint box view that contains the hint message as given in the UX specs. (https://drive.google.com/file/d/0B_2Uyb2Rhx2OU0FIbXUyMkhMZlE/view) - Adds required resource strings for hint box messages. Working protottpe(The video is the entire prototype. This change only involves adding the touch point and hint box.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... Screenshot of how it looks on different DPI screens: https://screenshot.googleplex.com/ND5d9v0799y.png Screenshot for RTL: https://screenshot.googleplex.com/Xd3PyAYbDTV.png BUG=634166 COMPONENT=Touch Calibration, ChromeOS, UI Strings ========== to ========== Adds animated touch point and the hint box for touch calibration UX - Adds the circular throbbing touch point view that the user interacts with during touch calibration. - Adds the hint box view that contains the hint message as given in the UX specs. (https://drive.google.com/file/d/0B_2Uyb2Rhx2OU0FIbXUyMkhMZlE/view) - Adds required resource strings for hint box messages. Working protottpe(The video is the entire prototype. This change only involves adding the touch point and hint box.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... Screenshot of how it looks on different DPI screens: https://screenshot.googleplex.com/ND5d9v0799y.png Screenshot for RTL: https://screenshot.googleplex.com/Xd3PyAYbDTV.png BUG=634166 COMPONENT=Touch Calibration, ChromeOS, UI Strings Review-Url: https://codereview.chromium.org/2600213002 Cr-Commit-Position: refs/heads/master@{#443000} Committed: https://chromium.googlesource.com/chromium/src/+/2f631d1860f7dfcf100b5a6bef46... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/chromium/src/+/2f631d1860f7dfcf100b5a6bef46...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:120001) has been created in https://codereview.chromium.org/2629003003/ by dschuyler@chromium.org. The reason for reverting is: Hoping to clear up issue on builder here: https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... -- sheriff. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
