Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2259183003:
Added a small widget framework to the interactive bevel SampleApp (Closed)
DescriptionAdded a small widget framework to the interactive bevel SampleApp
I made a small framework to add slider and radial controls more easily.
The Sample now has controls for light direction and color.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2259183003
Committed: https://skia.googlesource.com/skia/+/3688bfa710d4519c2d306a76bf5500481c1d1559
Patch Set 1 #Patch Set 2 : Added comments, added color display #Patch Set 3 : More comments #Patch Set 4 : Added override keywords, rearraged methods into consistent ordering #Patch Set 5 : Small comment fix #Patch Set 6 : Moved ControlPanel comments #Patch Set 7 : Made control constructors private #
Total comments: 57
Patch Set 8 : Addressed all patch 7 comments except for relativeX #Patch Set 9 : Changed relativeY to relativePos #Patch Set 10 : Correct call for integer string append #Messages
Total messages: 20 (9 generated)
Description was changed from ========== Added a small widget framework to the interactive bevel SampleApp Code is messy! I'm cleaning it up right now. I made a small framework to add slider and radial controls more easily. The Sample now has controls for light direction and color. BUG=skia: ========== to ========== Added a small widget framework to the interactive bevel SampleApp Code is messy! I'm cleaning it up right now. I made a small framework to add slider and radial controls more easily. The Sample now has controls for light direction and color. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2259183003 ==========
dvonbeck@google.com changed reviewers: + egdaniel@google.com, robertphillips@google.com
Code is messy! I'm cleaning it up right now.
Added ColorDisplay!
I think it's ready for review now!
Description was changed from ========== Added a small widget framework to the interactive bevel SampleApp Code is messy! I'm cleaning it up right now. I made a small framework to add slider and radial controls more easily. The Sample now has controls for light direction and color. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2259183003 ========== to ========== Added a small widget framework to the interactive bevel SampleApp I made a small framework to add slider and radial controls more easily. The Sample now has controls for light direction and color. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2259183003 ==========
https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:18: public: one line ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:23: // relative position const & ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:37: argumend ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:39: // position gets modulated by the component's relative position. const& ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:69: // relative height, expects CTM to have been adjusted in advance. & on string ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:104: // region, to enable mouse caputre. const SkPoint& ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:106: Why not a relativeX too ? An SkPoint ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:112: public: const SkString& ? use INHERITED ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:119: virtual SkScalar width() const = 0; private: INHERITED ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:125: ControlPanel(SkScalar width) tabbed too far over https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:160: overlength https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:161: void onClickRelease() override { // Propagate click release to selected control, deselect control add newlines and brackets https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:186: // capture const & ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:220: for (int i = 0; i < fControls.count(); i++) { \ns and brackets https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:236: public: ovveride ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:242: void onSetParent() override { one line ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:290: const & ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:297: DiscreteSliderControl(SkString name, int* output, int min, int max) use INHERITED here ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:314: static constexpr SkScalar kSliderWidth = 10.0f; private: INHERITED ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:362: */ const & ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:375: // capture const & ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:393: if (fControlSelector->isInCtrlRegion(clickPos)) return true; \ns and brackets https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:399: private: one line ? const SkString& ? use INHERITED here ? https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:629: -50.0f, 50.0f)); That's and exciting cast there. I propose you actually have an int landing point.
couple more little things https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:101: virtual bool onClick(SkPoint clickPos) { return false; }; const & https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:219: bool onIsInCtrlRegion(SkPoint clickPos) const override { const & https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:282: bool onClick(SkPoint clickPos) override { const & https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:554: fCtrlRegion = SkRect::MakeXYWH(0.0f, kLabelHeight, kRegionRadius * 2.0f, kRegionRadius * 2.0f); 100 chars. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:724: fLights = builder.finish(); Does this correctly destroy the old lights in fLights?
Addressed all patch 7 comments except for relativeX https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... File samplecode/SampleBevel.cpp (right): https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:18: public: On 2016/08/19 17:50:20, robertphillips wrote: > one line ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:23: // relative position On 2016/08/19 17:50:19, robertphillips wrote: > const & ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:37: On 2016/08/19 17:50:19, robertphillips wrote: > argumend ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:39: // position gets modulated by the component's relative position. On 2016/08/19 17:50:19, robertphillips wrote: > const& ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:69: // relative height, expects CTM to have been adjusted in advance. On 2016/08/19 17:50:19, robertphillips wrote: > & on string ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:101: virtual bool onClick(SkPoint clickPos) { return false; }; On 2016/08/19 17:56:35, egdaniel wrote: > const & Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:104: // region, to enable mouse caputre. On 2016/08/19 17:50:19, robertphillips wrote: > const SkPoint& ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:112: public: On 2016/08/19 17:50:20, robertphillips wrote: > const SkString& ? > use INHERITED ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:119: virtual SkScalar width() const = 0; On 2016/08/19 17:50:19, robertphillips wrote: > private: > INHERITED > > ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:125: ControlPanel(SkScalar width) On 2016/08/19 17:50:19, robertphillips wrote: > tabbed too far over Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:160: On 2016/08/19 17:50:20, robertphillips wrote: > overlength Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:161: void onClickRelease() override { // Propagate click release to selected control, deselect control On 2016/08/19 17:50:19, robertphillips wrote: > add newlines and brackets Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:186: // capture On 2016/08/19 17:50:19, robertphillips wrote: > const & ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:219: bool onIsInCtrlRegion(SkPoint clickPos) const override { On 2016/08/19 17:56:35, egdaniel wrote: > const & Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:220: for (int i = 0; i < fControls.count(); i++) { On 2016/08/19 17:50:20, robertphillips wrote: > \ns and brackets Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:236: public: On 2016/08/19 17:50:19, robertphillips wrote: > ovveride ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:242: void onSetParent() override { On 2016/08/19 17:50:19, robertphillips wrote: > one line ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:282: bool onClick(SkPoint clickPos) override { On 2016/08/19 17:56:35, egdaniel wrote: > const & Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:290: On 2016/08/19 17:50:19, robertphillips wrote: > const & ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:297: DiscreteSliderControl(SkString name, int* output, int min, int max) On 2016/08/19 17:50:20, robertphillips wrote: > use INHERITED here ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:314: static constexpr SkScalar kSliderWidth = 10.0f; On 2016/08/19 17:50:19, robertphillips wrote: > private: > INHERITED > > ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:362: */ On 2016/08/19 17:50:19, robertphillips wrote: > const & ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:375: // capture On 2016/08/19 17:50:19, robertphillips wrote: > const & ? Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:393: if (fControlSelector->isInCtrlRegion(clickPos)) return true; On 2016/08/19 17:50:19, robertphillips wrote: > \ns and brackets Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:399: private: On 2016/08/19 17:50:19, robertphillips wrote: > one line ? > const SkString& ? > use INHERITED here ? Doesn't fit in one line, rest are done https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:554: fCtrlRegion = SkRect::MakeXYWH(0.0f, kLabelHeight, kRegionRadius * 2.0f, kRegionRadius * 2.0f); On 2016/08/19 17:56:35, egdaniel wrote: > 100 chars. Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:629: -50.0f, 50.0f)); On 2016/08/19 17:50:19, robertphillips wrote: > That's and exciting cast there. I propose you actually have an int landing > point. Done. https://codereview.chromium.org/2259183003/diff/120001/samplecode/SampleBevel... samplecode/SampleBevel.cpp:724: fLights = builder.finish(); On 2016/08/19 17:56:35, egdaniel wrote: > Does this correctly destroy the old lights in fLights? Yes, SkLights is backed by SkTArray of lights. Destructor of SkTArray runs destructor of each light and frees the memory.
lgtm
The CQ bit was checked by dvonbeck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com Link to the patchset: https://codereview.chromium.org/2259183003/#ps160001 (title: "Changed relativeY to relativePos")
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: Build-Win-MSVC-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by dvonbeck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com Link to the patchset: https://codereview.chromium.org/2259183003/#ps180001 (title: "Correct call for integer string append")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Added a small widget framework to the interactive bevel SampleApp I made a small framework to add slider and radial controls more easily. The Sample now has controls for light direction and color. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2259183003 ========== to ========== Added a small widget framework to the interactive bevel SampleApp I made a small framework to add slider and radial controls more easily. The Sample now has controls for light direction and color. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2259183003 Committed: https://skia.googlesource.com/skia/+/3688bfa710d4519c2d306a76bf5500481c1d1559 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://skia.googlesource.com/skia/+/3688bfa710d4519c2d306a76bf5500481c1d1559 |
