New issue
Advanced search Search tips

Issue 665696 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

shelf position can't be changed in unified desktop mode

Project Member Reported by osh...@chromium.org, Nov 16 2016

Issue description

55.0.2883.42 (Official Build) beta (64-bit)
Platform	8872.44.0 (Official Build) beta-channel samus

I believe this is regression.
 
Components: UI>Shell>MultipleMonitor
Cc: ovanieva@chromium.org
I can't repro this. It is possible to change the shelf position in UD.

However, it probably makes sense to revisit the shelf behavior in UD at this point.

- We currently limit the shelf to be in the first display (i.e. upper left display in the matrix).
   - Shelf position > Left, Bottom, Right are limited only in the first display.

- There is a bug in the workarea bounds when the shelf position is set to right. It subtracts the shelf height as if the shelf is placed in the second display. [See attached].
Screenshot 2017-12-06 at 2.52.49 PM.png
813 KB View Download
Status: Started (was: Assigned)
I was actually able to repro by chance. Looking into this...
Cc: jamescook@chromium.org msw@chromium.org
James & msw,

I found the reason why this is broken. CL with suggested fix is here https://chromium-review.googlesource.com/c/chromium/src/+/816135.

- All you need to do to repro this issue, is 
  + Set a different shelf alignment when you're not in unified mode.
  + Enable Unified Mode.
  + Try to change the shelf alignment, and observe that it fails.

- Unified Desktop has a single display with ID = display::kUnifiedDisplayId = -10. Because of this negative value, it is not possible to save a per display shelf alignment pref for the unified display [1].

- However, it is possible to change the pref value, since display ID is always equal to the primary display ID [2]. The handler for the pref change doesn't just use the new value to set the shelf alignment, instead it tries to figure out which value to set according to some rules [3].

- Since there had been a per-display shelf alignment before moving to unified mode, and no one for the unified display itself, |has_per_display_prefs| is true [4], local prefs are not recommended, and hence we return the default value which is the BOTTOM alignment.

- So the result is, the pref value is changed, but it is never applied.

[1]: https://cs.chromium.org/chromium/src/ash/public/cpp/shelf_prefs.cc?q=SetPerDisplayPref&sq=package:chromium&l=91

[2]: https://cs.chromium.org/chromium/src/ash/public/cpp/shelf_prefs.cc?q=SetShelfAlignmentPref&sq=package:chromium&l=225

[3]: https://cs.chromium.org/chromium/src/ash/public/cpp/shelf_prefs.cc?q=GetPerDisplayPref&sq=package:chromium&l=33

[4]: https://cs.chromium.org/chromium/src/ash/public/cpp/shelf_prefs.cc?q=GetPerDisplayPref&sq=package:chromium&l=72
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/30a718372e1c872ef6dff7b2e93a0add15277c7b

commit 30a718372e1c872ef6dff7b2e93a0add15277c7b
Author: Ahmed Fakhry <afakhry@google.com>
Date: Fri Dec 08 18:00:24 2017

Fix not being able to change shelf alignment in Unified Desktop Mode

Make it possible to set a per-display shelf pref setting for the Unified
display. Add test coverage for the case when a per-display shelf settings
was added prior to enabling Unified Mode, which used to result in not being
able to set the shelf alignment after going into Unified Mode.

BUG= 665696 
TEST=Added test coverage

Change-Id: I57c35801721c0e3e6b3d0c60237be26ef1c3abe0
Reviewed-on: https://chromium-review.googlesource.com/816135
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522819}
[modify] https://crrev.com/30a718372e1c872ef6dff7b2e93a0add15277c7b/ash/public/cpp/shelf_prefs.cc
[modify] https://crrev.com/30a718372e1c872ef6dff7b2e93a0add15277c7b/ash/shelf/shelf_controller_unittest.cc

Status: Fixed (was: Started)
Status: Archived (was: Fixed)
Status: Fixed (was: Archived)

Sign in to add a comment