Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2018213002:
Set the Bluetooth chooser list height to cut through the middle of a list item. (Closed)
|
Created:
4 years, 6 months ago by Jeffrey Yasskin Modified:
4 years, 6 months ago CC:
chromium-reviews, rolfe, fbeaufortchromium, juncai Base URL:
https://chromium.googlesource.com/chromium/src.git@lkcr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet the Bluetooth chooser list height to cut through the middle of a list item.
This isn't the perfect fix, but it's easy and doesn't require us to
figure out how to draw shadows.
This also uses the DecorView's height instead of the WindowManager's Display's height in order to work well in multi-window mode.
BUG=614300, 603536
Committed: https://crrev.com/af63104f71ad62bd89ba5c8bc61fde5c421fdd4c
Cr-Commit-Position: refs/heads/master@{#396966}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Use _DP suffix and use the decor view's height instead of the display's height. #Patch Set 3 : Remove some unused WindowManager code. #
Total comments: 4
Patch Set 4 : Remove an unnecessary cast. #
Messages
Total messages: 28 (11 generated)
jyasskin@chromium.org changed reviewers: + finnur@chromium.org, tedchoc@chromium.org
Screenshots at https://drive.google.com/file/d/0B8-yQ8-lB7qGcm4tOE4zR1VWZmM/view?usp=sharing and https://drive.google.com/file/d/0B8-yQ8-lB7qGbWRJVm04ZlJjOHc/view?usp=sharing. This isn't really as nice as showing a shadow, but FadingEdgeScrollView is a ScrollView, which says not to nest a ListView inside it: https://developer.android.com/reference/android/widget/ScrollView.html. This change was easy, and will deal with most of the problem.
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018213002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018213002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018213002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018213002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/05/28 01:09:08, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. I'm not sure why but it looks like it's not half of a list item but a little bit less. Can you enable "Show layout bounds" Android Developer Option and share a screenshot? See for instance: http://i.imgur.com/MuMit8E.png
On 2016/05/30 11:15:28, François Beaufort wrote: > On 2016/05/28 01:09:08, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > I'm not sure why but it looks like it's not half of a list item but a little bit > less. > Can you enable "Show layout bounds" Android Developer Option and share a > screenshot? > See for instance: http://i.imgur.com/MuMit8E.png I'll get a screenshot tomorrow, but I suspect it looks like less than half the list item because most of the item is lower-case letters, which have most of their weight below the mid-line.
On 2016/05/31 02:26:04, Jeffrey Yasskin wrote: > On 2016/05/30 11:15:28, François Beaufort wrote: > > On 2016/05/28 01:09:08, commit-bot: I haz the power wrote: > > > Dry run: This issue passed the CQ dry run. > > > > I'm not sure why but it looks like it's not half of a list item but a little > bit > > less. > > Can you enable "Show layout bounds" Android Developer Option and share a > > screenshot? > > See for instance: http://i.imgur.com/MuMit8E.png > > I'll get a screenshot tomorrow, but I suspect it looks like less than half the > list item because most of the item is lower-case letters, which have most of > their weight below the mid-line. You're absolutely right Jeffrey: http://i.imgur.com/IwW4Ign.png I guess there is nothing we can do about this then. Feel free to disregard my comment.
Oh, hey. Neat. LGTM. One nit. https://codereview.chromium.org/2018213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2018213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:260: private static final int LIST_ROW_HEIGHT = 48; nit: The convention in this class is to use _DP suffix.
https://codereview.chromium.org/2018213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2018213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:323: new LinearLayout.LayoutParams(LayoutParams.MATCH_PARENT, getListHeight(metrics))); what happens on rotation? This doesn't seem to be get recalculated there. This seems like it would break where the landscape portion can not contain the same number of elements as portrait. https://codereview.chromium.org/2018213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:332: float heightDp = metrics.heightPixels / metrics.density * LISTVIEW_HEIGHT_PERCENT; what does heightPixels return in multiwindow mode (when the app is docked in portrait mode)? Wondering if we should be basing this on the Window rect or something more tied to the app (seems to be what we do in AppMenu and LocationBarLayout which are the two places I know off hand that do this sort of height forcing behavior).
Description was changed from ========== Set the Bluetooth chooser list height to cut through the middle of a list item. This isn't the perfect fix, but it's easy and doesn't require us to figure out how to draw shadows. BUG=614300 ========== to ========== Set the Bluetooth chooser list height to cut through the middle of a list item. This isn't the perfect fix, but it's easy and doesn't require us to figure out how to draw shadows. This also uses the DecorView's height instead of the WindowManager's Display's height in order to work well in multi-window mode. BUG=614300,603536 ==========
https://codereview.chromium.org/2018213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2018213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:260: private static final int LIST_ROW_HEIGHT = 48; On 2016/05/31 17:20:17, Finnur wrote: > nit: The convention in this class is to use _DP suffix. Whoops, done. https://codereview.chromium.org/2018213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:323: new LinearLayout.LayoutParams(LayoutParams.MATCH_PARENT, getListHeight(metrics))); On 2016/05/31 17:55:58, Ted C wrote: > what happens on rotation? This doesn't seem to be get recalculated there. This > seems like it would break where the landscape portion can not contain the same > number of elements as portrait. It doesn't adjust either before or after this patch. I'd like to get this simple fix in without blocking on that more complicated fix. (I'm calling it complicated because, while that fix might turn out to be a simple code change, I don't know the code well enough to know what the code change will be.) https://codereview.chromium.org/2018213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:332: float heightDp = metrics.heightPixels / metrics.density * LISTVIEW_HEIGHT_PERCENT; On 2016/05/31 17:55:58, Ted C wrote: > what does heightPixels return in multiwindow mode (when the app is docked in > portrait mode)? > > Wondering if we should be basing this on the Window rect or something more tied > to the app (seems to be what we do in AppMenu and LocationBarLayout which are > the two places I know off hand that do this sort of height forcing behavior). n-preview-3/reference/android/view/Display.html#getMetrics(android.util.DisplayMetrics) says: "- If requested from non-Activity context (e.g. Application context via (WindowManager) getApplicationContext().getSystemService(Context.WINDOW_SERVICE)) metrics will report real size of the display based on current rotation. - If requested from activity resulting metrics will correspond to current window metrics. In this case the size can be smaller than physical size in multi-window mode." ItemChooserDialog is constructed with a WindowAndroid.getActivity() rather than getApplicationContext(), so it ought to return the same height as getWindowDelegate().getDecorViewHeight(), but in fact it doesn't: Split landscape: manager.getDefaultDisplay().getMetrics(metrics); metrics.heightPixels: is 1440 ((Activity) mContext).getWindow().getDecorView().getHeight() is 1440 mDialog.getWindow().getDecorView().getHeight() is 0. Then rotate to portrait and re-open dialog: metrics.heightPixels: 1952 Activity decorViewHeight: 990 Dialog decorView height: 0 context.getResources().getDisplayMetrics() works like the WindowManager's metrics. I've switched to using the decorView height, which made me change lots of Contexts to Activities.
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018213002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018213002/40001
lgtm https://codereview.chromium.org/2018213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2018213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:323: new LinearLayout.LayoutParams(LayoutParams.MATCH_PARENT, getListHeight(metrics))); On 2016/05/31 21:06:13, Jeffrey Yasskin wrote: > On 2016/05/31 17:55:58, Ted C wrote: > > what happens on rotation? This doesn't seem to be get recalculated there. > This > > seems like it would break where the landscape portion can not contain the same > > number of elements as portrait. > > It doesn't adjust either before or after this patch. > > I'd like to get this simple fix in without blocking on that more complicated > fix. (I'm calling it complicated because, while that fix might turn out to be a > simple code change, I don't know the code well enough to know what the code > change will be.) Acknowledged. https://codereview.chromium.org/2018213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:332: float heightDp = metrics.heightPixels / metrics.density * LISTVIEW_HEIGHT_PERCENT; On 2016/05/31 21:06:13, Jeffrey Yasskin wrote: > On 2016/05/31 17:55:58, Ted C wrote: > > what does heightPixels return in multiwindow mode (when the app is docked in > > portrait mode)? > > > > Wondering if we should be basing this on the Window rect or something more > tied > > to the app (seems to be what we do in AppMenu and LocationBarLayout which are > > the two places I know off hand that do this sort of height forcing behavior). > > n-preview-3/reference/android/view/Display.html#getMetrics(android.util.DisplayMetrics) > says: > > "- If requested from non-Activity context (e.g. Application context via > (WindowManager) > getApplicationContext().getSystemService(Context.WINDOW_SERVICE)) > metrics will report real size of the display based on current rotation. > - If requested from activity resulting metrics will correspond to current > window metrics. > In this case the size can be smaller than physical size in multi-window mode." > > ItemChooserDialog is constructed with a WindowAndroid.getActivity() rather than > getApplicationContext(), so it ought to return the same height as > getWindowDelegate().getDecorViewHeight(), but in fact it doesn't: > > Split landscape: > > manager.getDefaultDisplay().getMetrics(metrics); metrics.heightPixels: is 1440 > ((Activity) mContext).getWindow().getDecorView().getHeight() is 1440 > mDialog.getWindow().getDecorView().getHeight() is 0. > > Then rotate to portrait and re-open dialog: > metrics.heightPixels: 1952 > Activity decorViewHeight: 990 > Dialog decorView height: 0 > > context.getResources().getDisplayMetrics() works like the WindowManager's > metrics. > > > I've switched to using the decorView height, which made me change lots of > Contexts to Activities. And the bigger challenge pre-N is Samsung multi-window which has a bunch of odd behaviors. This looks good though. https://codereview.chromium.org/2018213002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2018213002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:131: mItemChooserDialog = new ItemChooserDialog((Activity) mActivity, this, labels); mActivity is an Activity, so it doesn't look like you'd need this cast https://codereview.chromium.org/2018213002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java (right): https://codereview.chromium.org/2018213002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:265: public void testListHeight() throws InterruptedException { it'd be nice to not start the activity to run this test, but that is annoying to do. I'll try to make this easier at some point.
Thanks! https://codereview.chromium.org/2018213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java (right): https://codereview.chromium.org/2018213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:323: new LinearLayout.LayoutParams(LayoutParams.MATCH_PARENT, getListHeight(metrics))); On 2016/05/31 21:13:15, Ted C wrote: > On 2016/05/31 21:06:13, Jeffrey Yasskin wrote: > > On 2016/05/31 17:55:58, Ted C wrote: > > > what happens on rotation? This doesn't seem to be get recalculated there. > > This > > > seems like it would break where the landscape portion can not contain the > same > > > number of elements as portrait. > > > > It doesn't adjust either before or after this patch. > > > > I'd like to get this simple fix in without blocking on that more complicated > > fix. (I'm calling it complicated because, while that fix might turn out to be > a > > simple code change, I don't know the code well enough to know what the code > > change will be.) > > Acknowledged. And I've filed a bug to track this: https://crbug.com/616227 https://codereview.chromium.org/2018213002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2018213002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:131: mItemChooserDialog = new ItemChooserDialog((Activity) mActivity, this, labels); On 2016/05/31 21:13:15, Ted C wrote: > mActivity is an Activity, so it doesn't look like you'd need this cast Whoops, removed. https://codereview.chromium.org/2018213002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java (right): https://codereview.chromium.org/2018213002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:265: public void testListHeight() throws InterruptedException { On 2016/05/31 21:13:15, Ted C wrote: > it'd be nice to not start the activity to run this test, but that is annoying to > do. I'll try to make this easier at some point. That'll be great. Thanks!
The CQ bit was checked by jyasskin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2018213002/#ps60001 (title: "Remove an unnecessary cast.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018213002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018213002/60001
Message was sent while issue was closed.
Description was changed from ========== Set the Bluetooth chooser list height to cut through the middle of a list item. This isn't the perfect fix, but it's easy and doesn't require us to figure out how to draw shadows. This also uses the DecorView's height instead of the WindowManager's Display's height in order to work well in multi-window mode. BUG=614300,603536 ========== to ========== Set the Bluetooth chooser list height to cut through the middle of a list item. This isn't the perfect fix, but it's easy and doesn't require us to figure out how to draw shadows. This also uses the DecorView's height instead of the WindowManager's Display's height in order to work well in multi-window mode. BUG=614300,603536 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Set the Bluetooth chooser list height to cut through the middle of a list item. This isn't the perfect fix, but it's easy and doesn't require us to figure out how to draw shadows. This also uses the DecorView's height instead of the WindowManager's Display's height in order to work well in multi-window mode. BUG=614300,603536 ========== to ========== Set the Bluetooth chooser list height to cut through the middle of a list item. This isn't the perfect fix, but it's easy and doesn't require us to figure out how to draw shadows. This also uses the DecorView's height instead of the WindowManager's Display's height in order to work well in multi-window mode. BUG=614300,603536 Committed: https://crrev.com/af63104f71ad62bd89ba5c8bc61fde5c421fdd4c Cr-Commit-Position: refs/heads/master@{#396966} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/af63104f71ad62bd89ba5c8bc61fde5c421fdd4c Cr-Commit-Position: refs/heads/master@{#396966} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
