Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2890233003:
Dynamically Adjust Screen Item Size According To Item Count (Closed)
DescriptionDynamically Adjust Screen Item Size According To Item Count
When Mac has two screens, and we try to do a screen share,
the second screen looks missing. It is actually there, but
it is in the second row, and the second row is just out of
the visual region, so one needs to scroll the list to see it,
and thus appears missing.
This CL shrinks the item size when Mac has multiple screens,
so we can display two items in a row, and the second row is
partially visible, and thus no confusion for the end user.
BUG=721387
Review-Url: https://codereview.chromium.org/2890233003
Cr-Commit-Position: refs/heads/master@{#474110}
Committed: https://chromium.googlesource.com/chromium/src/+/5f834453abb3347d32f71a304bd10b793ff2ae0a
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add Size Detection At Init #
Total comments: 2
Patch Set 3 : Move size detection to list observer #
Total comments: 6
Patch Set 4 : style #Patch Set 5 : Fix Unit test fail #Messages
Total messages: 20 (9 generated)
Description was changed from ========== Dynamically Adjust Screen Item Size According To Item Count BUG= ========== to ========== Dynamically Adjust Screen Item Size According To Item Count When Mac has two screens, and we try to do a screen share, the second screen looks missing. It is actually there, but it is in the second row, and the second row is just out of the visual region, so one needs to scroll the list to see it, and thus appears missing. This CL shrinks the item size when Mac has multiple screens, so we can display two items in a row, and the second row is partially visible, and thus no confusion for the end user. BUG=721387 ==========
qiangchen@chromium.org changed reviewers: + tapted@chromium.org
Can you take a look at this bug fix? Just several lines of change.
https://codereview.chromium.org/2890233003/diff/1/chrome/browser/ui/cocoa/med... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2890233003/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:285: createImageBrowserWithSize:NSMakeSize(kSingleScreenWidth, Can you pass the correct size here by inspecting |screenList_|? It seems odd to alter the size when each item is loaded. https://codereview.chromium.org/2890233003/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:616: } what if bowser != screenBrowser_? The old code would setTitleHidden:NO. Now it doesn't.
fix one, and explain one. https://codereview.chromium.org/2890233003/diff/1/chrome/browser/ui/cocoa/med... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2890233003/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:285: createImageBrowserWithSize:NSMakeSize(kSingleScreenWidth, On 2017/05/22 04:54:47, tapted wrote: > Can you pass the correct size here by inspecting |screenList_|? It seems odd to > alter the size when each item is loaded. Done. https://codereview.chromium.org/2890233003/diff/1/chrome/browser/ui/cocoa/med... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:616: } On 2017/05/22 04:54:47, tapted wrote: > what if bowser != screenBrowser_? The old code would setTitleHidden:NO. Now it > doesn't. For those cases, we never setTitleHidden to YES at any point, so it is safe.
https://codereview.chromium.org/2890233003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2890233003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:613: setCellSize:NSMakeSize(kMultipleScreenWidth, kMultipleScreenHeight)]; is this still needed? I don't think it should be here. In the general case, the size of items determines the number of items the imagebrowser needs to show, and the number of calls it makes to this delegate method. Changing the size could trigger more calls to this method, or cause the imagebrowser to discard items it had returned earlier (possibly even in a loop). If you really need it, add a comment since it looks really odd. Like (if tested) "if the user adds or removes a screen while the browser is showing then..."
https://codereview.chromium.org/2890233003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2890233003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:613: setCellSize:NSMakeSize(kMultipleScreenWidth, kMultipleScreenHeight)]; On 2017/05/22 23:54:01, tapted wrote: > is this still needed? I don't think it should be here. In the general case, the > size of items determines the number of items the imagebrowser needs to show, and > the number of calls it makes to this delegate method. Changing the size could > trigger more calls to this method, or cause the imagebrowser to discard items it > had returned earlier (possibly even in a loop). > > If you really need it, add a comment since it looks really odd. Like (if tested) > "if the user adds or removes a screen while the browser is showing then..." The list is dynamically updated in another thread, so it is unsafe to rely on the logic at initialization stage, as it is likely that at initialization, the items are not ready yet. I moved the code to list observer handler. So we can avoid repeatedly doing such setting at the UI event.
lgtm https://codereview.chromium.org/2890233003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2890233003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:706: // Switch to multiple sources mode nit: full stop at end https://codereview.chromium.org/2890233003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:744: [browser reloadData]; move this after the setCellSize call? https://codereview.chromium.org/2890233003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:747: } nit: curlies not needed
The CQ bit was checked by qiangchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2890233003/#ps60001 (title: "style")
https://codereview.chromium.org/2890233003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm (right): https://codereview.chromium.org/2890233003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:706: // Switch to multiple sources mode On 2017/05/23 20:36:11, tapted wrote: > nit: full stop at end Done. https://codereview.chromium.org/2890233003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:744: [browser reloadData]; On 2017/05/23 20:36:11, tapted wrote: > move this after the setCellSize call? Done. https://codereview.chromium.org/2890233003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:747: } On 2017/05/23 20:36:11, tapted wrote: > nit: curlies not needed Done.
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by qiangchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2890233003/#ps80001 (title: "Fix Unit test fail")
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": 80001, "attempt_start_ts": 1495581369178380,
"parent_rev": "a9822cebdf63992f0ae013f524bc1f34ab3cf148", "commit_rev":
"5f834453abb3347d32f71a304bd10b793ff2ae0a"}
Message was sent while issue was closed.
Description was changed from ========== Dynamically Adjust Screen Item Size According To Item Count When Mac has two screens, and we try to do a screen share, the second screen looks missing. It is actually there, but it is in the second row, and the second row is just out of the visual region, so one needs to scroll the list to see it, and thus appears missing. This CL shrinks the item size when Mac has multiple screens, so we can display two items in a row, and the second row is partially visible, and thus no confusion for the end user. BUG=721387 ========== to ========== Dynamically Adjust Screen Item Size According To Item Count When Mac has two screens, and we try to do a screen share, the second screen looks missing. It is actually there, but it is in the second row, and the second row is just out of the visual region, so one needs to scroll the list to see it, and thus appears missing. This CL shrinks the item size when Mac has multiple screens, so we can display two items in a row, and the second row is partially visible, and thus no confusion for the end user. BUG=721387 Review-Url: https://codereview.chromium.org/2890233003 Cr-Commit-Position: refs/heads/master@{#474110} Committed: https://chromium.googlesource.com/chromium/src/+/5f834453abb3347d32f71a304bd1... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/5f834453abb3347d32f71a304bd1... |
