Re: RFR: 8281723: Spinner with split horizontal arrows and a border places right arrow incorrectly [v2]
On Wed, 9 Mar 2022 07:48:53 GMT, John Hendrikx wrote: >> I added a test case for `SpinnerSkin` that checks the arrow positioning. >> >> While adding the tests I discovered more problems with the positioning aside >> from the one mentioned in the JBS ticket. >> >> 1) Vertical split arrow placement also forgot to take the padding into >> account while placing the decrement arrow button -- I've taken the liberty >> to fix that problem as well in the same PR. >> >> 2) When arrows are placed next to each other either on the right or left, >> the arrow widths are not normalized to be the width of the widest arrow. >> All other placements will normalize either the width or height, except for >> these two. Specifically, when the arrows are **split** on the left and >> right they **are** normalized to the same width. >> >> For point 2, here is the problem illustrated with actual widths on left and >> layout result on right: >> >> [ <- ] [ -> ] [ spinner ] --> [ <- ] [ -> ] [ >> spinner ] >> [ spinner ] [ <- ] [ -> ] --> [ spinner ] [ <- ] >> [ -> ] >> >> While for split horizontal it does normalize the width to that of the widest >> arrow, and so layout becomes: >> >> [ <- ] [ spinner ] [ -> ] --> [ <- ] [ spinner ] >> [ -> ] >> >> While I'm here I could fix this as well, and adjust the test case to match. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright year Hadn't noticed it was ready to be integrated. Here goes. - PR: https://git.openjdk.java.net/jfx/pull/748
Re: RFR: 8281723: Spinner with split horizontal arrows and a border places right arrow incorrectly [v2]
On Wed, 9 Mar 2022 07:48:53 GMT, John Hendrikx wrote: >> I added a test case for `SpinnerSkin` that checks the arrow positioning. >> >> While adding the tests I discovered more problems with the positioning aside >> from the one mentioned in the JBS ticket. >> >> 1) Vertical split arrow placement also forgot to take the padding into >> account while placing the decrement arrow button -- I've taken the liberty >> to fix that problem as well in the same PR. >> >> 2) When arrows are placed next to each other either on the right or left, >> the arrow widths are not normalized to be the width of the widest arrow. >> All other placements will normalize either the width or height, except for >> these two. Specifically, when the arrows are **split** on the left and >> right they **are** normalized to the same width. >> >> For point 2, here is the problem illustrated with actual widths on left and >> layout result on right: >> >> [ <- ] [ -> ] [ spinner ] --> [ <- ] [ -> ] [ >> spinner ] >> [ spinner ] [ <- ] [ -> ] --> [ spinner ] [ <- ] >> [ -> ] >> >> While for split horizontal it does normalize the width to that of the widest >> arrow, and so layout becomes: >> >> [ <- ] [ spinner ] [ -> ] --> [ <- ] [ spinner ] >> [ -> ] >> >> While I'm here I could fix this as well, and adjust the test case to match. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright year This looks straight-forward enough that a single reviewer is fine. - PR: https://git.openjdk.java.net/jfx/pull/748
Re: RFR: 8281723: Spinner with split horizontal arrows and a border places right arrow incorrectly [v2]
On Wed, 9 Mar 2022 07:48:53 GMT, John Hendrikx wrote: >> I added a test case for `SpinnerSkin` that checks the arrow positioning. >> >> While adding the tests I discovered more problems with the positioning aside >> from the one mentioned in the JBS ticket. >> >> 1) Vertical split arrow placement also forgot to take the padding into >> account while placing the decrement arrow button -- I've taken the liberty >> to fix that problem as well in the same PR. >> >> 2) When arrows are placed next to each other either on the right or left, >> the arrow widths are not normalized to be the width of the widest arrow. >> All other placements will normalize either the width or height, except for >> these two. Specifically, when the arrows are **split** on the left and >> right they **are** normalized to the same width. >> >> For point 2, here is the problem illustrated with actual widths on left and >> layout result on right: >> >> [ <- ] [ -> ] [ spinner ] --> [ <- ] [ -> ] [ >> spinner ] >> [ spinner ] [ <- ] [ -> ] --> [ spinner ] [ <- ] >> [ -> ] >> >> While for split horizontal it does normalize the width to that of the widest >> arrow, and so layout becomes: >> >> [ <- ] [ spinner ] [ -> ] --> [ <- ] [ spinner ] >> [ -> ] >> >> While I'm here I could fix this as well, and adjust the test case to match. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright year The fix looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/748
Re: RFR: 8281723: Spinner with split horizontal arrows and a border places right arrow incorrectly [v2]
On Wed, 9 Mar 2022 07:48:53 GMT, John Hendrikx wrote: >> I added a test case for `SpinnerSkin` that checks the arrow positioning. >> >> While adding the tests I discovered more problems with the positioning aside >> from the one mentioned in the JBS ticket. >> >> 1) Vertical split arrow placement also forgot to take the padding into >> account while placing the decrement arrow button -- I've taken the liberty >> to fix that problem as well in the same PR. >> >> 2) When arrows are placed next to each other either on the right or left, >> the arrow widths are not normalized to be the width of the widest arrow. >> All other placements will normalize either the width or height, except for >> these two. Specifically, when the arrows are **split** on the left and >> right they **are** normalized to the same width. >> >> For point 2, here is the problem illustrated with actual widths on left and >> layout result on right: >> >> [ <- ] [ -> ] [ spinner ] --> [ <- ] [ -> ] [ >> spinner ] >> [ spinner ] [ <- ] [ -> ] --> [ spinner ] [ <- ] >> [ -> ] >> >> While for split horizontal it does normalize the width to that of the widest >> arrow, and so layout becomes: >> >> [ <- ] [ spinner ] [ -> ] --> [ <- ] [ spinner ] >> [ -> ] >> >> While I'm here I could fix this as well, and adjust the test case to match. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright year Fix + Tests look good to me - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/748
Re: RFR: 8281723: Spinner with split horizontal arrows and a border places right arrow incorrectly [v2]
> I added a test case for `SpinnerSkin` that checks the arrow positioning. > > While adding the tests I discovered more problems with the positioning aside > from the one mentioned in the JBS ticket. > > 1) Vertical split arrow placement also forgot to take the padding into > account while placing the decrement arrow button -- I've taken the liberty to > fix that problem as well in the same PR. > > 2) When arrows are placed next to each other either on the right or left, the > arrow widths are not normalized to be the width of the widest arrow. All > other placements will normalize either the width or height, except for these > two. Specifically, when the arrows are **split** on the left and right they > **are** normalized to the same width. > > For point 2, here is the problem illustrated with actual widths on left and > layout result on right: > > [ <- ] [ -> ] [ spinner ] --> [ <- ] [ -> ] [ > spinner ] > [ spinner ] [ <- ] [ -> ] --> [ spinner ] [ <- ] > [ -> ] > > While for split horizontal it does normalize the width to that of the widest > arrow, and so layout becomes: > > [ <- ] [ spinner ] [ -> ] --> [ <- ] [ spinner ] > [ -> ] > > While I'm here I could fix this as well, and adjust the test case to match. John Hendrikx has updated the pull request incrementally with one additional commit since the last revision: Update copyright year - Changes: - all: https://git.openjdk.java.net/jfx/pull/748/files - new: https://git.openjdk.java.net/jfx/pull/748/files/8c7059e0..9bb77d56 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=748=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=748=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/748.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/748/head:pull/748 PR: https://git.openjdk.java.net/jfx/pull/748
