Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 2836313002: BoxLayout now suports per-view margins.

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 6 days ago by kylix_rd
Modified:
2 days, 17 hours ago
Reviewers:
Peter Kasting, sky
CC:
chromium-reviews, tfarina, robliao
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

BoxLayout now suports per-view margins. Call BoxLayout::SetViewMargins(view, margins) to set the amount of space for each side of the control.

Patch Set 1 #

Total comments: 11

Patch Set 2 : Clarifications, comments, and logic changes #

Patch Set 3 : BoxLayout now uses Margins property on the view #

Total comments: 1

Patch Set 4 : Added margin collapsing and a BoxLayout example #

Total comments: 45

Patch Set 5 : Refactor Combobox and Textfield creation into common functions #

Patch Set 6 : BoxLayoutExample complete #

Patch Set 7 : Comments, constant identifiers, removed ViewListener and ExampleModel #

Patch Set 8 : Adjust cross-axis margin to be the max of all margins or insets #

Patch Set 9 : Added unit tests. Fixed GetPreferredSize() calculations. Refactored some code. #

Total comments: 11

Patch Set 10 : Added example comment. Addressed other feedback. #

Patch Set 11 : Moved view properties to separate file. #

Patch Set 12 : Fixed crash in example and added ability to control size of child panels. #

Patch Set 13 : Uncollapsed margins in the cross-axis now align the views #

Patch Set 14 : Fixed BoxLayout for unit tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1009 lines, -50 lines) Patch
M ui/views/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M ui/views/examples/BUILD.gn View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
A ui/views/examples/box_layout_example.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +107 lines, -0 lines 0 comments Download
A ui/views/examples/box_layout_example.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +439 lines, -0 lines 0 comments Download
M ui/views/examples/examples_window.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -0 lines 0 comments Download
M ui/views/layout/box_layout.h View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +127 lines, -5 lines 0 comments Download
M ui/views/layout/box_layout.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 11 chunks +224 lines, -44 lines 0 comments Download
M ui/views/layout/box_layout_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +69 lines, -0 lines 0 comments Download
M ui/views/view.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -1 line 0 comments Download
A ui/views/view_properties.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +21 lines, -0 lines 0 comments Download
A ui/views/view_properties.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +13 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 95 (46 generated)
kylix_rd
Here's the initial cut at adding per-view margins to BoxLayout. If this passes muster, I'll ...
3 weeks, 6 days ago (2017-04-25 14:24:23 UTC) #3
sky
https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc#newcode15 ui/views/layout/box_layout.cc:15: : view_(nullptr), layout_(nullptr), margins_(0, 0, 0, 0){}; (0,0,0,0) is ...
3 weeks, 5 days ago (2017-04-25 17:28:59 UTC) #4
Peter Kasting
My needs are minimal -- I won't have cases with adjacent margins, so collapsing behavior ...
3 weeks, 5 days ago (2017-04-25 21:43:34 UTC) #5
sky
https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc#newcode306 ui/views/layout/box_layout.cc:306: gfx::Size BoxLayout::GetPreferredSizeForChildWidth(const View* host, It seems like what you're ...
3 weeks, 5 days ago (2017-04-25 22:46:07 UTC) #6
kylix_rd
https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc#newcode15 ui/views/layout/box_layout.cc:15: : view_(nullptr), layout_(nullptr), margins_(0, 0, 0, 0){}; On 2017/04/25 ...
3 weeks, 4 days ago (2017-04-26 17:41:33 UTC) #8
sky
https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc#newcode34 ui/views/layout/box_layout.cc:34: const_cast<View*>(view_)->SetBounds( On 2017/04/26 17:41:33, kylix_rd wrote: > On 2017/04/25 ...
3 weeks, 4 days ago (2017-04-26 19:57:07 UTC) #12
kylix_rd
This patch addresses more feedback and also moves to using a class property on View ...
3 weeks, 3 days ago (2017-04-27 18:58:01 UTC) #15
sky
Could you please clarify what 'more feedback'? Specifically are you still working on the patch ...
3 weeks, 3 days ago (2017-04-27 22:21:45 UTC) #18
kylix_rd
On 2017/04/27 22:21:45, sky wrote: > Could you please clarify what 'more feedback'? Specifically are ...
3 weeks, 3 days ago (2017-04-28 13:26:21 UTC) #19
sky
On 2017/04/26 17:41:33, kylix_rd wrote: > https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc > File ui/views/layout/box_layout.cc (right): > > https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc#newcode15 > ...
3 weeks, 2 days ago (2017-04-28 19:20:59 UTC) #20
kylix_rd
On 2017/04/28 19:20:59, sky wrote: > On 2017/04/26 17:41:33, kylix_rd wrote: > > > https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc ...
2 weeks, 5 days ago (2017-05-02 20:37:39 UTC) #21
sky
Glad to hear it! Don't hesitate to ask questions if I wasn't clear in my ...
2 weeks, 5 days ago (2017-05-03 00:03:09 UTC) #22
kylix_rd
On 2017/05/03 00:03:09, sky wrote: > Glad to hear it! > > Don't hesitate to ...
2 weeks, 4 days ago (2017-05-03 18:29:22 UTC) #23
sky
I believe Peter is out of the office right now, so I'm not sure he ...
2 weeks, 4 days ago (2017-05-04 03:51:09 UTC) #24
kylix_rd
Here's BoxLayout with full margin collapsing. I've also got most of the work done on ...
1 week, 5 days ago (2017-05-09 16:24:16 UTC) #26
sky
https://codereview.chromium.org/2836313002/diff/40001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2836313002/diff/40001/ui/views/view.h#newcode1705 ui/views/view.h:1705: VIEWS_EXPORT extern const ui::ClassProperty<gfx::Insets*>* const kMarginsKey; Please move this ...
1 week, 5 days ago (2017-05-09 19:26:04 UTC) #29
kylix_rd
Some things were already addressed in a subsequent patch prior to the comments. Others are ...
1 week, 5 days ago (2017-05-09 20:07:06 UTC) #30
sky
https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_layout.cc#newcode350 ui/views/layout/box_layout.cc:350: void BoxLayout::AdjustCrossAxisForMargin(const ViewWrapper& child, On 2017/05/09 20:07:06, kylix_rd wrote: ...
1 week, 5 days ago (2017-05-09 23:55:29 UTC) #31
kylix_rd
As for the unification of the cross-axis margins, I'll think about that some more. My ...
1 week, 5 days ago (2017-05-10 15:30:37 UTC) #32
sky
Peter should weigh in on this, but again my thinking is that we will end ...
1 week, 4 days ago (2017-05-10 16:12:18 UTC) #37
Peter Kasting
It seems like there are generally two routes to go with most of the cross-axis ...
1 week, 4 days ago (2017-05-10 18:07:36 UTC) #41
kylix_rd
Based on Peter's highly detailed and epic ASCII art illustration, this patch implements (1). It ...
1 week, 4 days ago (2017-05-10 20:35:19 UTC) #42
kylix_rd
This includes some unit tests which will ensure margins are taken into account and/or collapsed ...
1 week, 3 days ago (2017-05-11 19:59:53 UTC) #49
sky
Peter, I stared at your ASCII art for a good ten minutes and couldn't make ...
1 week, 3 days ago (2017-05-12 14:03:10 UTC) #52
kylix_rd
On 2017/05/12 14:03:10, sky wrote: > Peter, > > I stared at your ASCII art ...
1 week, 3 days ago (2017-05-12 15:25:53 UTC) #53
sky
On 2017/05/12 15:25:53, kylix_rd wrote: > On 2017/05/12 14:03:10, sky wrote: > > Peter, > ...
1 week, 2 days ago (2017-05-12 19:41:43 UTC) #54
Peter Kasting
On 2017/05/12 19:41:43, sky wrote: > I'm advocating for something like 1, but not quite. ...
1 week, 2 days ago (2017-05-12 19:59:16 UTC) #55
sky
In earlier email threads I thought we wanted to head toward a place where views ...
1 week, 1 day ago (2017-05-14 14:04:25 UTC) #56
Peter Kasting
On 2017/05/14 14:04:25, sky wrote: > In earlier email threads I thought we wanted to ...
6 days, 22 hours ago (2017-05-15 17:06:29 UTC) #57
sky
(3) it is. Will review shortly. On Mon, May 15, 2017 at 10:06 AM, <pkasting@chromium.org> ...
6 days, 21 hours ago (2017-05-15 18:18:17 UTC) #58
sky
GAH! Sorry, that should be (1) it is. Will review shortly. On Mon, May 15, ...
6 days, 21 hours ago (2017-05-15 18:18:34 UTC) #59
sky
On 2017/05/15 18:18:34, sky wrote: > GAH! Sorry, that should be (1) it is. Will ...
6 days, 19 hours ago (2017-05-15 20:28:24 UTC) #60
sky
I got part way through this before realizing what you implemented isn't what Peter wants. ...
6 days, 19 hours ago (2017-05-15 20:29:10 UTC) #61
Peter Kasting
On 2017/05/15 20:28:24, sky wrote: > On 2017/05/15 18:18:34, sky wrote: > > GAH! Sorry, ...
6 days, 18 hours ago (2017-05-15 21:15:33 UTC) #62
sky
On 2017/05/15 21:15:33, Peter Kasting wrote: > On 2017/05/15 20:28:24, sky wrote: > > On ...
6 days, 17 hours ago (2017-05-15 21:44:53 UTC) #63
Peter Kasting
On 2017/05/15 21:44:53, sky wrote: > On 2017/05/15 21:15:33, Peter Kasting wrote: > > Then ...
6 days, 17 hours ago (2017-05-15 21:53:03 UTC) #64
kylix_rd
On 2017/05/15 21:53:03, Peter Kasting wrote: > On 2017/05/15 21:44:53, sky wrote: > > On ...
6 days, 1 hour ago (2017-05-16 13:34:01 UTC) #65
kylix_rd
On 2017/05/16 13:34:01, kylix_rd wrote: > On 2017/05/15 21:53:03, Peter Kasting wrote: > > On ...
5 days, 23 hours ago (2017-05-16 16:21:35 UTC) #66
kylix_rd
On 2017/05/16 16:21:35, kylix_rd wrote: > On 2017/05/16 13:34:01, kylix_rd wrote: > > On 2017/05/15 ...
5 days, 21 hours ago (2017-05-16 17:37:08 UTC) #67
kylix_rd
Still need to separate the view property into a separate file, and maybe add more ...
5 days, 20 hours ago (2017-05-16 18:45:48 UTC) #68
kylix_rd
New patches. Moved view properties to separate files. Fixed crash reported by pkasting@ in the ...
4 days, 21 hours ago (2017-05-17 18:14:03 UTC) #75
kylix_rd
OK, this patch *should* get the functionality nailed down. In the non-collapsed margin mode, the ...
3 days, 22 hours ago (2017-05-18 17:06:14 UTC) #80
Peter Kasting
On 2017/05/18 17:06:14, kylix_rd wrote: > OK, this patch *should* get the functionality nailed down. ...
3 days, 22 hours ago (2017-05-18 17:10:46 UTC) #83
kylix_rd
On 2017/05/18 17:10:46, Peter Kasting wrote: > On 2017/05/18 17:06:14, kylix_rd wrote: > > OK, ...
3 days, 20 hours ago (2017-05-18 19:16:16 UTC) #86
kylix_rd
On 2017/05/18 19:16:16, kylix_rd wrote: > On 2017/05/18 17:10:46, Peter Kasting wrote: > > On ...
2 days, 20 hours ago (2017-05-19 18:53:17 UTC) #91
Peter Kasting
On 2017/05/19 18:53:17, kylix_rd wrote: > On 2017/05/18 19:16:16, kylix_rd wrote: > > On 2017/05/18 ...
2 days, 20 hours ago (2017-05-19 19:02:21 UTC) #92
kylix_rd
On 2017/05/19 19:02:21, Peter Kasting wrote: > On 2017/05/19 18:53:17, kylix_rd wrote: > > On ...
2 days, 20 hours ago (2017-05-19 19:25:02 UTC) #93
sky
On 2017/05/19 19:02:21, Peter Kasting wrote: > On 2017/05/19 18:53:17, kylix_rd wrote: > > On ...
2 days, 17 hours ago (2017-05-19 22:06:48 UTC) #94
Peter Kasting
2 days, 17 hours ago (2017-05-19 22:09:51 UTC) #95
On 2017/05/19 22:06:48, sky wrote:
> Peter, I encourage you to put up a patch for converting InfoBars to using
> BoxLayout. I would hate for Allen to do all this work and then you realize
there
> is some reason InfoBars can't actually use BoxLayout... I think the chances of
> that happening are slim, but it would be good to know sooner rather than
later.

That's a good idea.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06