Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2179243002:
exo: Set widget parent to target root window (Closed)
|
Created:
4 years, 5 months ago by Dominik Laskowski Modified:
4 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionexo: Set widget parent to target root window
Windows created by the active window on a secondary display would appear
on the primary display instead of the same display.
BUG=631136
TEST=weston-terminal: Right-click > Open Terminal on secondary display.
TEST=Opening windows in ARC apps still works.
Committed: https://crrev.com/8effe9a13cf037ffc7ededa776607665340e1d83
Cr-Commit-Position: refs/heads/master@{#409042}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Rebase #Patch Set 3 : Fix incorrect root window for popups #
Total comments: 3
Messages
Total messages: 22 (7 generated)
The CQ bit was checked by domlaskowski@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...
domlaskowski@chromium.org changed reviewers: + oshima@chromium.org, reveman@chromium.org
https://codereview.chromium.org/2179243002/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2179243002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:891: params.bounds = initial_bounds_; This isn't correct (at least for arc++). I think the correct way is to simply pass the context, not parent, and let the initial bounds find the correct root window. I believe it's empty for arc++ windows?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2179243002/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2179243002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:890: ash::Shell::GetContainer(ash::Shell::GetTargetRootWindow(), container_); When |parent_| is set then this needs to be the same root window as the parent otherwise popups might show up on a different root window. https://codereview.chromium.org/2179243002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:891: params.bounds = initial_bounds_; On 2016/07/26 at 01:07:11, oshima wrote: > This isn't correct (at least for arc++). > > I think the correct way is to simply pass the context, not parent, and let the initial bounds find the correct root window. > > I believe it's empty for arc++ windows? Initial bounds are always "gfx::Rect(1, 1)" for arc++ windows to provide absolute positioning of sub-surfaces in the root window coordinate space. Initial bounds is never set for normal xdg shell surfaces. It will be set for popups and is derived from the parent position in that case.
https://codereview.chromium.org/2179243002/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2179243002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:891: params.bounds = initial_bounds_; On 2016/07/26 14:46:01, reveman wrote: > On 2016/07/26 at 01:07:11, oshima wrote: > > This isn't correct (at least for arc++). > > > > I think the correct way is to simply pass the context, not parent, and let the > initial bounds find the correct root window. > > > > I believe it's empty for arc++ windows? > > Initial bounds are always "gfx::Rect(1, 1)" for arc++ windows to provide > absolute positioning of sub-surfaces in the root window coordinate space. > > Initial bounds is never set for normal xdg shell surfaces. It will be set for > popups and is derived from the parent position in that case. If the initial bounds is set in screen coordinate, it should pick the right root window. That's being said, for popup, it probably should set the transient parent so that it stays in the same root window.
https://codereview.chromium.org/2179243002/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2179243002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:891: params.bounds = initial_bounds_; On 2016/07/26 at 20:40:56, oshima wrote: > On 2016/07/26 14:46:01, reveman wrote: > > On 2016/07/26 at 01:07:11, oshima wrote: > > > This isn't correct (at least for arc++). > > > > > > I think the correct way is to simply pass the context, not parent, and let the > > initial bounds find the correct root window. > > > > > > I believe it's empty for arc++ windows? > > > > Initial bounds are always "gfx::Rect(1, 1)" for arc++ windows to provide > > absolute positioning of sub-surfaces in the root window coordinate space. > > > > Initial bounds is never set for normal xdg shell surfaces. It will be set for > > popups and is derived from the parent position in that case. > > If the initial bounds is set in screen coordinate, it should pick the right root window. That's being said, for popup, it probably should set the transient parent so that it stays in the same root window. Initial bounds is parent origin + offset provided by the client so part of this is in the client's control and we should probably not allow the client to put the popup in the wrong root window. I think transient parent is already set correctly for popups.
https://codereview.chromium.org/2179243002/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2179243002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:890: ash::Shell::GetContainer(ash::Shell::GetTargetRootWindow(), container_); On 2016/07/26 14:46:01, reveman wrote: > When |parent_| is set then this needs to be the same root window as the parent > otherwise popups might show up on a different root window. Good point. See my reply below. https://codereview.chromium.org/2179243002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:891: params.bounds = initial_bounds_; On 2016/07/26 21:38:33, reveman wrote: > On 2016/07/26 at 20:40:56, oshima wrote: > > On 2016/07/26 14:46:01, reveman wrote: > > > On 2016/07/26 at 01:07:11, oshima wrote: > > > > This isn't correct (at least for arc++). > > > > > > > > I think the correct way is to simply pass the context, not parent, and let > the > > > initial bounds find the correct root window. > > > > > > > > I believe it's empty for arc++ windows? > > > > > > Initial bounds are always "gfx::Rect(1, 1)" for arc++ windows to provide > > > absolute positioning of sub-surfaces in the root window coordinate space. > > > > > > Initial bounds is never set for normal xdg shell surfaces. It will be set > for > > > popups and is derived from the parent position in that case. > > > > If the initial bounds is set in screen coordinate, it should pick the right > root window. That's being said, for popup, it probably should set the transient > parent so that it stays in the same root window. > > Initial bounds is parent origin + offset provided by the client so part of this > is in the client's control and we should probably not allow the client to put > the popup in the wrong root window. > > I think transient parent is already set correctly for popups. Initial bounds are in screen coordinates regardless of |params.parent| (hence the need for [1]), but I think part of the reason why popups show up on the wrong root window (in addition to the client issue above) is that they are made transient after Widget::Init has set their bounds. This should be fixed by setting |params.parent| to |parent_|. I believe you're right about passing a context instead for ARC windows. [1] https://codereview.chromium.org/2175373003/
I got sidetracked attempting workarounds for the incorrect position and size of popups in overview mode, before finding out about b/28817702. IIUC, the plan is to fix that from the Ash side. The above patch ensures popups are created on the correct root window. We can't pass a context for ARC windows, because we need to use the requested container.
https://codereview.chromium.org/2179243002/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2179243002/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:891: params.parent = parent_ ? parent_ : Pretty sure this will break xdg shell modal dialogs. Parent will be set in this case but the widget should not be a child of the parent widget as that would result in the dialog being moved if the parent was moved. that's not the expected behavior. Can this be something like "parent_ ? parent_->GetRootWindow()" instead and the transient child code below is kept as it is? fyi, gtk3-demo is useful to test xdg shell implementation.
https://codereview.chromium.org/2179243002/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2179243002/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:891: params.parent = parent_ ? parent_ : On 2016/07/29 18:27:07, reveman wrote: > Pretty sure this will break xdg shell modal dialogs. Parent will be set in this > case but the widget should not be a child of the parent widget as that would > result in the dialog being moved if the parent was moved. that's not the > expected behavior. Can this be something like "parent_ ? > parent_->GetRootWindow()" instead and the transient child code below is kept as > it is? > > fyi, gtk3-demo is useful to test xdg shell implementation. That's still not right. Let's discuss offline.
https://codereview.chromium.org/2179243002/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2179243002/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:891: params.parent = parent_ ? parent_ : On 2016/07/29 18:27:07, reveman wrote: > Pretty sure this will break xdg shell modal dialogs. Parent will be set in this > case but the widget should not be a child of the parent widget as that would > result in the dialog being moved if the parent was moved. that's not the > expected behavior. Can this be something like "parent_ ? > parent_->GetRootWindow()" instead and the transient child code below is kept as > it is? > > fyi, gtk3-demo is useful to test xdg shell implementation. Unless |params.child| is true, Widget::Init makes the widget a transient child of |params.parent|, and then uses |params.parent| as a context to find a parent root window. This is equivalent to the old code, except the root window is the same as that of the parent. I've verified that modal dialogs in gtk3-demo behave as expected, modulo an unrelated bug where windows underneath can be focused.
On 2016/07/29 20:37:09, Dominik Laskowski wrote: > https://codereview.chromium.org/2179243002/diff/40001/components/exo/shell_su... > File components/exo/shell_surface.cc (right): > > https://codereview.chromium.org/2179243002/diff/40001/components/exo/shell_su... > components/exo/shell_surface.cc:891: params.parent = parent_ ? parent_ : > On 2016/07/29 18:27:07, reveman wrote: > > Pretty sure this will break xdg shell modal dialogs. Parent will be set in > this > > case but the widget should not be a child of the parent widget as that would > > result in the dialog being moved if the parent was moved. that's not the > > expected behavior. Can this be something like "parent_ ? > > parent_->GetRootWindow()" instead and the transient child code below is kept > as > > it is? > > > > fyi, gtk3-demo is useful to test xdg shell implementation. > > Unless |params.child| is true, Widget::Init makes the widget a transient child > of |params.parent|, and then uses |params.parent| as a context to find a parent > root window. This is equivalent to the old code, except the root window is the > same as that of the parent. > > I've verified that modal dialogs in gtk3-demo behave as expected, modulo an > unrelated bug where windows underneath can be focused. Dominik is correct. I also realized that we can't simply rely on the bounds because it has to be in particular cotnainer, so lgtm.
On 2016/07/29 at 20:37:09, domlaskowski wrote: > https://codereview.chromium.org/2179243002/diff/40001/components/exo/shell_su... > File components/exo/shell_surface.cc (right): > > https://codereview.chromium.org/2179243002/diff/40001/components/exo/shell_su... > components/exo/shell_surface.cc:891: params.parent = parent_ ? parent_ : > On 2016/07/29 18:27:07, reveman wrote: > > Pretty sure this will break xdg shell modal dialogs. Parent will be set in this > > case but the widget should not be a child of the parent widget as that would > > result in the dialog being moved if the parent was moved. that's not the > > expected behavior. Can this be something like "parent_ ? > > parent_->GetRootWindow()" instead and the transient child code below is kept as > > it is? > > > > fyi, gtk3-demo is useful to test xdg shell implementation. > > Unless |params.child| is true, Widget::Init makes the widget a transient child of |params.parent|, and then uses |params.parent| as a context to find a parent root window. This is equivalent to the old code, except the root window is the same as that of the parent. > > I've verified that modal dialogs in gtk3-demo behave as expected, modulo an unrelated bug where windows underneath can be focused. Ok, thanks for explaining and thanks for verifying. Lgtm.
The CQ bit was checked by domlaskowski@chromium.org
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== exo: Set widget parent to target root window Windows created by the active window on a secondary display would appear on the primary display instead of the same display. BUG=631136 TEST=weston-terminal: Right-click > Open Terminal on secondary display. TEST=Opening windows in ARC apps still works. ========== to ========== exo: Set widget parent to target root window Windows created by the active window on a secondary display would appear on the primary display instead of the same display. BUG=631136 TEST=weston-terminal: Right-click > Open Terminal on secondary display. TEST=Opening windows in ARC apps still works. Committed: https://crrev.com/8effe9a13cf037ffc7ededa776607665340e1d83 Cr-Commit-Position: refs/heads/master@{#409042} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8effe9a13cf037ffc7ededa776607665340e1d83 Cr-Commit-Position: refs/heads/master@{#409042} |
