Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2078773003:
Realign message center popups when displays are added or removed (Closed)
DescriptionRealign message center popups when displays are added or removed
When switching between displays with different resolutions (e.g., plugging in an external monitor to a laptop), there is no "DisplayMetricsChanged" event on Linux. This causes notifications to appear in the wrong position. Handle this by checking if the primary display has changed whenever a display is added or removed.
BUG=621010
Committed: https://crrev.com/86453717f7fe8a57bb407cadcf337aab80fd7e22
Cr-Commit-Position: refs/heads/master@{#405036}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Realign message center popups when displays are added or removed #
Total comments: 2
Patch Set 3 : Realign message center popups when displays are added or removed #Patch Set 4 : Realign message center popups when displays are added or removed #
Messages
Total messages: 40 (10 generated)
Description was changed from ========== Realign message center popups when displays are added or removed BUG=498523 ========== to ========== Realign message center popups when displays are added or removed BUG=498523 ==========
osandov@osandov.com changed reviewers: + mukai@chromium.org, stevenjb@chromium.org
I don't think this is the right place to fix. If it's the same display but the display resolution have changed, the configuration changed event has to be emitted. You should look into the display event code on linux.
Description was changed from ========== Realign message center popups when displays are added or removed BUG=498523 ========== to ========== Realign message center popups when displays are added or removed When switching between displays with different resolutions (e.g., plugging in an external monitor to a laptop), there is no "DisplayMetricsChanged" event. This causes notifications to appear in the wrong position. BUG=621010 ==========
On 2016/06/17 07:01:11, Jun Mukai wrote: > I don't think this is the right place to fix. If it's the same display but the > display resolution have changed, the configuration changed event has to be > emitted. > > You should look into the display event code on linux. Hi, Jun, Sorry, I should have written a better description. This is happening when adding a larger display, not when changing the resolution of an existing display. I've updated the issue description and opened up a new bug report. Does this solution make more sense in that case? Thanks.
On 2016/06/17 07:16:47, osandov wrote: > On 2016/06/17 07:01:11, Jun Mukai wrote: > > I don't think this is the right place to fix. If it's the same display but the > > display resolution have changed, the configuration changed event has to be > > emitted. > > > > You should look into the display event code on linux. > > Hi, Jun, > > Sorry, I should have written a better description. This is happening when adding > a larger display, not when changing the resolution of an existing display. I've > updated the issue description and opened up a new bug report. Does this solution > make more sense in that case? > > Thanks. IIRC desktop_popup_alignment_delegate is a per-display object. If it's a new screen, a new instance should be created and that would configure the notification locations properly. Existing instances don't have to care about display added/removed events.
On 2016/06/17 08:00:52, Jun Mukai wrote: > On 2016/06/17 07:16:47, osandov wrote: > > On 2016/06/17 07:01:11, Jun Mukai wrote: > > > I don't think this is the right place to fix. If it's the same display but > the > > > display resolution have changed, the configuration changed event has to be > > > emitted. > > > > > > You should look into the display event code on linux. > > > > Hi, Jun, > > > > Sorry, I should have written a better description. This is happening when > adding > > a larger display, not when changing the resolution of an existing display. > I've > > updated the issue description and opened up a new bug report. Does this > solution > > make more sense in that case? > > > > Thanks. > > IIRC desktop_popup_alignment_delegate is a per-display object. > > If it's a new screen, a new instance should be created and that would configure > the notification locations properly. > Existing instances don't have to care about display added/removed events. Hm, I followed the call hierarchy from the DesktopPopupAlignmentDelegate constructor all the way up to BrowserProcessImpl::notification_ui_manager() and it looks like a singleton to me, am I missing something?
mukai@chromium.org changed reviewers: + dewittj@chromium.org
On 2016/06/17 08:18:58, osandov wrote: > On 2016/06/17 08:00:52, Jun Mukai wrote: > > On 2016/06/17 07:16:47, osandov wrote: > > > On 2016/06/17 07:01:11, Jun Mukai wrote: > > > > I don't think this is the right place to fix. If it's the same display but > > the > > > > display resolution have changed, the configuration changed event has to be > > > > emitted. > > > > > > > > You should look into the display event code on linux. > > > > > > Hi, Jun, > > > > > > Sorry, I should have written a better description. This is happening when > > adding > > > a larger display, not when changing the resolution of an existing display. > > I've > > > updated the issue description and opened up a new bug report. Does this > > solution > > > make more sense in that case? > > > > > > Thanks. > > > > IIRC desktop_popup_alignment_delegate is a per-display object. > > > > If it's a new screen, a new instance should be created and that would > configure > > the notification locations properly. > > Existing instances don't have to care about display added/removed events. > > Hm, I followed the call hierarchy from the DesktopPopupAlignmentDelegate > constructor all the way up to BrowserProcessImpl::notification_ui_manager() and > it looks like a singleton to me, am I missing something? Hm, I see. So, to be clear, the situation would be something like - a new display is added and the display becomes primary - DesktopPopupAlignmentDelegate doesn't reconfigure with the new primary display Is that right? I am wondering how it's dealt with in Windows. Probably +dewittj is a better reviewer.
If I remember correctly, when this was implemented we did get a DisplayMetricsChanged event if a monitor was added/removed. It may have regressed in the intervening time, I don't think there is test coverage of this type of thing. It'd be interesting to try to get such coverage. Since there isn't really coverage, I'd request that you run the updated code on a Windows box and a Mac box to make sure that it works as expected there.
ah I see you are an external contributor. If that is not plausible for you, I think we can get someone within Google to test on those platforms. Please let me know.
On 2016/06/17 17:41:36, dewittj wrote: > ah I see you are an external contributor. If that is not plausible for you, I > think we can get someone within Google to test on those platforms. Please let > me know. Yeah I don't have easy access to those platforms, I'd appreciate it if someone at Google could test it out.
https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... File ui/message_center/views/desktop_popup_alignment_delegate.cc (right): https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... ui/message_center/views/desktop_popup_alignment_delegate.cc:87: const display::Display& new_display) { rename to added_display https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... ui/message_center/views/desktop_popup_alignment_delegate.cc:88: display::Display display = screen_->GetPrimaryDisplay(); nit: rename to primary_display https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... ui/message_center/views/desktop_popup_alignment_delegate.cc:91: RecomputeAlignment(display); Lots of code duplication between these, could you please refactor into a helper function? https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... ui/message_center/views/desktop_popup_alignment_delegate.cc:97: const display::Display& old_display) { rename to removed_display https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... ui/message_center/views/desktop_popup_alignment_delegate.cc:99: display::Display display = screen_->GetPrimaryDisplay(); primary_display
Hm, so that's still bad on other platforms? Considering this class is actually a singleton and primary display can be changed, this might be the best way to deal with this? Hmm. https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... File ui/message_center/views/desktop_popup_alignment_delegate.cc (right): https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... ui/message_center/views/desktop_popup_alignment_delegate.cc:87: const display::Display& new_display) { Can you also leave a comment for why these are necessary? That would be something like: // Primary display can change upon adding or removing display, and // this will have to recompute the alignment on the new screen. https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... ui/message_center/views/desktop_popup_alignment_delegate.cc:109: if (display.id() == display_id_) { Can it be renamed as |primary_display_id_|? That's more descriptive.
This is a slightly clearer version with the common code factored out.
This is a matter of communication style, but usually you're supposed to reply on individual inline comments. If you simply follow what the reviewer said, you can simply reply as 'done' (actually there is a shortcut link for this). Please do that manner, then it's clearer for reviewers to see how their comments are addressed. The newer code is looking good, but I'll defer to dewittj for the approval.
Thanks for the guidance. https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... File ui/message_center/views/desktop_popup_alignment_delegate.cc (right): https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... ui/message_center/views/desktop_popup_alignment_delegate.cc:87: const display::Display& new_display) { On 2016/06/20 20:28:54, Jun Mukai wrote: > Can you also leave a comment for why these are necessary? > > That would be something like: > > // Primary display can change upon adding or removing display, and > // this will have to recompute the alignment on the new screen. Done. https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... ui/message_center/views/desktop_popup_alignment_delegate.cc:87: const display::Display& new_display) { On 2016/06/20 17:06:00, dewittj wrote: > rename to added_display Done. https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... ui/message_center/views/desktop_popup_alignment_delegate.cc:88: display::Display display = screen_->GetPrimaryDisplay(); On 2016/06/20 17:06:00, dewittj wrote: > nit: rename to primary_display This moved after refactoring, but also done. https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... ui/message_center/views/desktop_popup_alignment_delegate.cc:91: RecomputeAlignment(display); On 2016/06/20 17:06:00, dewittj wrote: > Lots of code duplication between these, could you please refactor into a helper > function? Done. https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... ui/message_center/views/desktop_popup_alignment_delegate.cc:97: const display::Display& old_display) { On 2016/06/20 17:06:00, dewittj wrote: > rename to removed_display Done. https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... ui/message_center/views/desktop_popup_alignment_delegate.cc:99: display::Display display = screen_->GetPrimaryDisplay(); On 2016/06/20 17:05:59, dewittj wrote: > primary_display Done, moved in the refactoring.
Sorry, one more. https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... File ui/message_center/views/desktop_popup_alignment_delegate.cc (right): https://codereview.chromium.org/2078773003/diff/1/ui/message_center/views/des... ui/message_center/views/desktop_popup_alignment_delegate.cc:109: if (display.id() == display_id_) { On 2016/06/20 20:28:54, Jun Mukai wrote: > Can it be renamed as |primary_display_id_|? That's more descriptive. Done.
I verified that the multiscreen behavior on Windows seems fine even with the patch. https://codereview.chromium.org/2078773003/diff/20001/ui/message_center/views... File ui/message_center/views/desktop_popup_alignment_delegate.cc (right): https://codereview.chromium.org/2078773003/diff/20001/ui/message_center/views... ui/message_center/views/desktop_popup_alignment_delegate.cc:112: // Set to kInvalidDisplayID so the alignment is updated regardless of whether Why always recompute the alignment?
https://codereview.chromium.org/2078773003/diff/20001/ui/message_center/views... File ui/message_center/views/desktop_popup_alignment_delegate.cc (right): https://codereview.chromium.org/2078773003/diff/20001/ui/message_center/views... ui/message_center/views/desktop_popup_alignment_delegate.cc:112: // Set to kInvalidDisplayID so the alignment is updated regardless of whether On 2016/06/27 17:37:27, dewittj wrote: > Why always recompute the alignment? I wanted to reuse the helper to handle all of the cases. We need the recompute if the primary display changes resolution or switches to another display. If another display resizes, we do some unnecessary work, but this seemed like the cleanest way to implement it.
Hi -- I want to make sure what will happen on Windows without this patch. Because DesktopPopupAlignmentDelegate only shows the notification on the primary display -- - boot with multiple displays, make sure a chrome notification appears on a display - disconnect that display (or close the lid if it's a notebook) - reshow the chrome notifications If it's already working well on Windows (i.e. the chrome notification appears at the right position on the other display), that means that Windows already takes care of the case like this, in a different code location. If so -- I still think it's not the right thing to fix here -- find out why Windows works well and make the fix in the same place (of Linux-specific code). If Windows doesn't work well on Windows too -- +1 to go ahead with this.
Chrome Canary on windows seems to have correct behavior.
On 2016/06/28 15:24:30, dewittj wrote: > Chrome Canary on windows seems to have correct behavior. Hm, that's confusing. Conceptually, adding a new display doesn't change the existing display. Is the Windows code generating spurious DisplayChanged events when a display is added? Or do the display IDs get reassigned in some way? It's unclear to me how this would be fixed in the Linux code. Adding an extra DisplayChanged event when no display has actually changed doesn't seem right. I can take a look at the Windows code but I wouldn't be able to test it.
On 2016/06/29 07:20:36, osandov wrote: > On 2016/06/28 15:24:30, dewittj wrote: > > Chrome Canary on windows seems to have correct behavior. > > Hm, that's confusing. Conceptually, adding a new display doesn't change the > existing display. Is the Windows code generating spurious DisplayChanged events > when a display is added? Or do the display IDs get reassigned in some way? It's > unclear to me how this would be fixed in the Linux code. Adding an extra > DisplayChanged event when no display has actually changed doesn't seem right. I > can take a look at the Windows code but I wouldn't be able to test it. A possibility would be that a 'primary display' might be unchanged on Windows -- physical display relationship for the primary display might change, i.e. display_id might change along with resolution changes event, on MetricsChanged event? If that's the case -- personally I prefer the current Linux behavior and +1 to make this change for Linux (I am not sure if we should fix this Windows behavior -- probably we shouldn't). Sorry for the confusion and prevention... I'm now positive to this patch.
This patch needs a rebase. I also did some research, it looks like if you have only one display active and switch between laptop and desktop, only MetricsChanged is called, and display ID does not change. If you have "extend" mode on, when you connect or disconnect the cable Display Added/Removed is called AND metrics is called.
Description was changed from ========== Realign message center popups when displays are added or removed When switching between displays with different resolutions (e.g., plugging in an external monitor to a laptop), there is no "DisplayMetricsChanged" event. This causes notifications to appear in the wrong position. BUG=621010 ========== to ========== Realign message center popups when displays are added or removed When switching between displays with different resolutions (e.g., plugging in an external monitor to a laptop), there is no "DisplayMetricsChanged" event on Linux. This causes notifications to appear in the wrong position. Handle this by checking if the primary display has changed whenever a display is added or removed. BUG=621010 ==========
Right, looks like the display abstraction behaves slightly differently on Linux and Windows, but this should handle both. Rebased and retested.
I'll approve this patch but I'd like an explicit comment about how the behavior differs on different platforms, so that future readers don't have to go find a dual-monitor Windows platform like I did to figure it out :) Once that is in it will look l-g-t-m.
Thanks, updated with a comment that should hopefully make things clear for anyone else who comes across this :)
lgtm, thanks for your patience!
The CQ bit was checked by osandov@osandov.com
The CQ bit was unchecked by osandov@osandov.com
The CQ bit was checked by osandov@osandov.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Realign message center popups when displays are added or removed When switching between displays with different resolutions (e.g., plugging in an external monitor to a laptop), there is no "DisplayMetricsChanged" event on Linux. This causes notifications to appear in the wrong position. Handle this by checking if the primary display has changed whenever a display is added or removed. BUG=621010 ========== to ========== Realign message center popups when displays are added or removed When switching between displays with different resolutions (e.g., plugging in an external monitor to a laptop), there is no "DisplayMetricsChanged" event on Linux. This causes notifications to appear in the wrong position. Handle this by checking if the primary display has changed whenever a display is added or removed. BUG=621010 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Realign message center popups when displays are added or removed When switching between displays with different resolutions (e.g., plugging in an external monitor to a laptop), there is no "DisplayMetricsChanged" event on Linux. This causes notifications to appear in the wrong position. Handle this by checking if the primary display has changed whenever a display is added or removed. BUG=621010 ========== to ========== Realign message center popups when displays are added or removed When switching between displays with different resolutions (e.g., plugging in an external monitor to a laptop), there is no "DisplayMetricsChanged" event on Linux. This causes notifications to appear in the wrong position. Handle this by checking if the primary display has changed whenever a display is added or removed. BUG=621010 Committed: https://crrev.com/86453717f7fe8a57bb407cadcf337aab80fd7e22 Cr-Commit-Position: refs/heads/master@{#405036} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/86453717f7fe8a57bb407cadcf337aab80fd7e22 Cr-Commit-Position: refs/heads/master@{#405036} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
