Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2959853002:
Adding smarter resizing logic for the context menu on orientation changes (Closed)
|
Created:
3 years, 5 months ago by Daniel Park Modified:
3 years, 5 months ago CC:
chromium-reviews, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding smarter resizing logic for the context menu on orientation changes
* Renamed CONTEXT_MENU_MIN_PADDING_PX to mContextMenuMinimumPaddingPx
* Grouped context_menu_min_vertical_padding and context_menu_min_side_padding into
context_menu_min_padding.
* Added more comments in TabularContextMenuViewPager#onMeasure
* Made padding into a global variable called CONTEXT_MENU_MIN_PADDING_PX.
* Renamed context_menu_height_padding to context_menu_min_vertical_padding with half
the value, but multiplied all usages by 2 so the total values are the same.
* Added context_menu_min_side_padding.
* Rewrote sizing logic so width is the minimum of 500 dp or total width - 2 * side_padding
* Added a max height to context menu row
* Added a max height to the context menu so it no longer leaks over the edge.
* The list view now scrolls when the context menu reaches max height.
Previous issue link: https://codereview.chromium.org/2956943002/
I had to recreate it after fixing the owner issue.
BUG=731173
Review-Url: https://codereview.chromium.org/2959853002
Cr-Commit-Position: refs/heads/master@{#484364}
Committed: https://chromium.googlesource.com/chromium/src/+/c76fe2d15d81e2e125ad5efc57a70ab143870a6e
Patch Set 1 #
Total comments: 4
Patch Set 2 : Cleaning up comments. #
Total comments: 6
Patch Set 3 : comments #
Messages
Total messages: 33 (20 generated)
Description was changed from ========== Adding smarter resizing logic for the context menu on orientation changes Renamed CONTEXT_MENU_MIN_PADDING_PX to mContextMenuMinimumPaddingPx Grouped context_menu_min_vertical_padding and context_menu_min_side_padding into context_menu_min_padding. Added more comments in TabularContextMenuViewPager#onMeasure Made padding into a global variable called CONTEXT_MENU_MIN_PADDING_PX. Renamed context_menu_height_padding to context_menu_min_vertical_padding with half the value, but multiplied all usages by 2 so the total values are the same. Added context_menu_min_side_padding. Rewrote sizing logic so width is the minimum of 500 dp or total width - 2 * side_padding Added a max height to context menu row Added a max height to the context menu so it no longer leaks over the edge. The list view now scrolls when the context menu reaches max height. BUG=731173 ========== to ========== Adding smarter resizing logic for the context menu on orientation changes * Renamed CONTEXT_MENU_MIN_PADDING_PX to mContextMenuMinimumPaddingPx * Grouped context_menu_min_vertical_padding and context_menu_min_side_padding into context_menu_min_padding. * Added more comments in TabularContextMenuViewPager#onMeasure * Made padding into a global variable called CONTEXT_MENU_MIN_PADDING_PX. * Renamed context_menu_height_padding to context_menu_min_vertical_padding with half the value, but multiplied all usages by 2 so the total values are the same. * Added context_menu_min_side_padding. * Rewrote sizing logic so width is the minimum of 500 dp or total width - 2 * side_padding * Added a max height to context menu row * Added a max height to the context menu so it no longer leaks over the edge. * The list view now scrolls when the context menu reaches max height. BUG=731173 ==========
Description was changed from ========== Adding smarter resizing logic for the context menu on orientation changes * Renamed CONTEXT_MENU_MIN_PADDING_PX to mContextMenuMinimumPaddingPx * Grouped context_menu_min_vertical_padding and context_menu_min_side_padding into context_menu_min_padding. * Added more comments in TabularContextMenuViewPager#onMeasure * Made padding into a global variable called CONTEXT_MENU_MIN_PADDING_PX. * Renamed context_menu_height_padding to context_menu_min_vertical_padding with half the value, but multiplied all usages by 2 so the total values are the same. * Added context_menu_min_side_padding. * Rewrote sizing logic so width is the minimum of 500 dp or total width - 2 * side_padding * Added a max height to context menu row * Added a max height to the context menu so it no longer leaks over the edge. * The list view now scrolls when the context menu reaches max height. BUG=731173 ========== to ========== Adding smarter resizing logic for the context menu on orientation changes * Renamed CONTEXT_MENU_MIN_PADDING_PX to mContextMenuMinimumPaddingPx * Grouped context_menu_min_vertical_padding and context_menu_min_side_padding into context_menu_min_padding. * Added more comments in TabularContextMenuViewPager#onMeasure * Made padding into a global variable called CONTEXT_MENU_MIN_PADDING_PX. * Renamed context_menu_height_padding to context_menu_min_vertical_padding with half the value, but multiplied all usages by 2 so the total values are the same. * Added context_menu_min_side_padding. * Rewrote sizing logic so width is the minimum of 500 dp or total width - 2 * side_padding * Added a max height to context menu row * Added a max height to the context menu so it no longer leaks over the edge. * The list view now scrolls when the context menu reaches max height. BUG=731173 ==========
Description was changed from ========== Adding smarter resizing logic for the context menu on orientation changes * Renamed CONTEXT_MENU_MIN_PADDING_PX to mContextMenuMinimumPaddingPx * Grouped context_menu_min_vertical_padding and context_menu_min_side_padding into context_menu_min_padding. * Added more comments in TabularContextMenuViewPager#onMeasure * Made padding into a global variable called CONTEXT_MENU_MIN_PADDING_PX. * Renamed context_menu_height_padding to context_menu_min_vertical_padding with half the value, but multiplied all usages by 2 so the total values are the same. * Added context_menu_min_side_padding. * Rewrote sizing logic so width is the minimum of 500 dp or total width - 2 * side_padding * Added a max height to context menu row * Added a max height to the context menu so it no longer leaks over the edge. * The list view now scrolls when the context menu reaches max height. BUG=731173 ========== to ========== Adding smarter resizing logic for the context menu on orientation changes * Renamed CONTEXT_MENU_MIN_PADDING_PX to mContextMenuMinimumPaddingPx * Grouped context_menu_min_vertical_padding and context_menu_min_side_padding into context_menu_min_padding. * Added more comments in TabularContextMenuViewPager#onMeasure * Made padding into a global variable called CONTEXT_MENU_MIN_PADDING_PX. * Renamed context_menu_height_padding to context_menu_min_vertical_padding with half the value, but multiplied all usages by 2 so the total values are the same. * Added context_menu_min_side_padding. * Rewrote sizing logic so width is the minimum of 500 dp or total width - 2 * side_padding * Added a max height to context menu row * Added a max height to the context menu so it no longer leaks over the edge. * The list view now scrolls when the context menu reaches max height. BUG=731173 ==========
Description was changed from ========== Adding smarter resizing logic for the context menu on orientation changes * Renamed CONTEXT_MENU_MIN_PADDING_PX to mContextMenuMinimumPaddingPx * Grouped context_menu_min_vertical_padding and context_menu_min_side_padding into context_menu_min_padding. * Added more comments in TabularContextMenuViewPager#onMeasure * Made padding into a global variable called CONTEXT_MENU_MIN_PADDING_PX. * Renamed context_menu_height_padding to context_menu_min_vertical_padding with half the value, but multiplied all usages by 2 so the total values are the same. * Added context_menu_min_side_padding. * Rewrote sizing logic so width is the minimum of 500 dp or total width - 2 * side_padding * Added a max height to context menu row * Added a max height to the context menu so it no longer leaks over the edge. * The list view now scrolls when the context menu reaches max height. BUG=731173 ========== to ========== Adding smarter resizing logic for the context menu on orientation changes * Renamed CONTEXT_MENU_MIN_PADDING_PX to mContextMenuMinimumPaddingPx * Grouped context_menu_min_vertical_padding and context_menu_min_side_padding into context_menu_min_padding. * Added more comments in TabularContextMenuViewPager#onMeasure * Made padding into a global variable called CONTEXT_MENU_MIN_PADDING_PX. * Renamed context_menu_height_padding to context_menu_min_vertical_padding with half the value, but multiplied all usages by 2 so the total values are the same. * Added context_menu_min_side_padding. * Rewrote sizing logic so width is the minimum of 500 dp or total width - 2 * side_padding * Added a max height to context menu row * Added a max height to the context menu so it no longer leaks over the edge. * The list view now scrolls when the context menu reaches max height. Previous issue link: https://codereview.chromium.org/2956943002/ I had to recreate it after fixing the owner issue. BUG=731173 ==========
danielpark@chromium.org changed reviewers: + dfalcantara@chromium.org, tedchoc@chromium.org, twellington@chromium.org
The CQ bit was checked by danielpark@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...
https://codereview.chromium.org/2959853002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2959853002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:42: // set the context menu width English sentences, capital letters and punctuation. https://codereview.chromium.org/2959853002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:49: // figure out which view is about to displayed & measure that view's height This current only applies to the block at line 62, right? It seems that what it's doing is measuring how tall the displayed view and the tabs are when displayed together.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2959853002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2959853002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:42: // set the context menu width On 2017/06/26 21:57:23, dfalcantara wrote: > English sentences, capital letters and punctuation. Done. https://codereview.chromium.org/2959853002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:49: // figure out which view is about to displayed & measure that view's height On 2017/06/26 21:57:23, dfalcantara wrote: > This current only applies to the block at line 62, right? > > It seems that what it's doing is measuring how tall the displayed view and the > tabs are when displayed together. Done.
The CQ bit was checked by danielpark@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/2959853002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2959853002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:42: // Sets the context menu width to the min of max_width and screen width minus padding. Too detailed about the wrong things... How about: "The width of the context menu is defined so that it leaves space between itself and the screen's edges. It's also bounded to prevent the menu from stretching across a large display (i.e. a tablet screen)." https://codereview.chromium.org/2959853002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:48: How about: The height of the context menu is calculated as the sum of: 1) The tab bar's height, which is visible only when the context menu requires it. 2) The height of the View being displayed for the current tab. https://codereview.chromium.org/2959853002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:72: int fullHeight = menuHeight + tabHeight; Maybe: Cap the height of the menu so that it fits on the screen without touching the screen edges.
danielpark@chromium.org changed reviewers: - dfalcantara@chromium.org
https://codereview.chromium.org/2959853002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2959853002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:42: // Sets the context menu width to the min of max_width and screen width minus padding. On 2017/06/29 20:13:08, gone wrote: > Too detailed about the wrong things... How about: > > "The width of the context menu is defined so that it leaves space between itself > and the screen's edges. It's also bounded to prevent the menu from stretching > across a large display (i.e. a tablet screen)." Done. https://codereview.chromium.org/2959853002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:48: On 2017/06/29 20:13:08, gone wrote: > How about: > > The height of the context menu is calculated as the sum of: > 1) The tab bar's height, which is visible only when the context menu requires > it. > 2) The height of the View being displayed for the current tab. Done. https://codereview.chromium.org/2959853002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:72: int fullHeight = menuHeight + tabHeight; On 2017/06/29 20:13:08, gone wrote: > Maybe: > > Cap the height of the menu so that it fits on the screen without touching the > screen edges. Done.
danielpark@chromium.org changed reviewers: + dfalcantara@chromium.org
lgtm
rs lgtm
The CQ bit was checked by danielpark@chromium.org
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by danielpark@chromium.org
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": 40001, "attempt_start_ts": 1499283831893470,
"parent_rev": "a9f628ff52bffcfcae37a669c8bae5da1c560364", "commit_rev":
"c76fe2d15d81e2e125ad5efc57a70ab143870a6e"}
Message was sent while issue was closed.
Description was changed from ========== Adding smarter resizing logic for the context menu on orientation changes * Renamed CONTEXT_MENU_MIN_PADDING_PX to mContextMenuMinimumPaddingPx * Grouped context_menu_min_vertical_padding and context_menu_min_side_padding into context_menu_min_padding. * Added more comments in TabularContextMenuViewPager#onMeasure * Made padding into a global variable called CONTEXT_MENU_MIN_PADDING_PX. * Renamed context_menu_height_padding to context_menu_min_vertical_padding with half the value, but multiplied all usages by 2 so the total values are the same. * Added context_menu_min_side_padding. * Rewrote sizing logic so width is the minimum of 500 dp or total width - 2 * side_padding * Added a max height to context menu row * Added a max height to the context menu so it no longer leaks over the edge. * The list view now scrolls when the context menu reaches max height. Previous issue link: https://codereview.chromium.org/2956943002/ I had to recreate it after fixing the owner issue. BUG=731173 ========== to ========== Adding smarter resizing logic for the context menu on orientation changes * Renamed CONTEXT_MENU_MIN_PADDING_PX to mContextMenuMinimumPaddingPx * Grouped context_menu_min_vertical_padding and context_menu_min_side_padding into context_menu_min_padding. * Added more comments in TabularContextMenuViewPager#onMeasure * Made padding into a global variable called CONTEXT_MENU_MIN_PADDING_PX. * Renamed context_menu_height_padding to context_menu_min_vertical_padding with half the value, but multiplied all usages by 2 so the total values are the same. * Added context_menu_min_side_padding. * Rewrote sizing logic so width is the minimum of 500 dp or total width - 2 * side_padding * Added a max height to context menu row * Added a max height to the context menu so it no longer leaks over the edge. * The list view now scrolls when the context menu reaches max height. Previous issue link: https://codereview.chromium.org/2956943002/ I had to recreate it after fixing the owner issue. BUG=731173 Review-Url: https://codereview.chromium.org/2959853002 Cr-Commit-Position: refs/heads/master@{#484364} Committed: https://chromium.googlesource.com/chromium/src/+/c76fe2d15d81e2e125ad5efc57a7... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c76fe2d15d81e2e125ad5efc57a7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
