Thanks a lot for that patch Jouni.
Some functional testing shows that it works pretty reliably, thank you.
Let's dig deep in the code side of things.
foreword: below _NET_NUMBER_OF_DESKTOPS and _NET_DESKTOP_LAYOUT refer to
the root window's X properties
WorkspacesInfo::updateWorkspaceGeometry() has 3 functions:
- deciding on the number of workspaces (_NET_NUMBER_OF_DESKTOPS) if not done so
by an external program (e.g. metacity)
- deciding on the layout of the workspaces (_NET_DESKTOP_LAYOUT) if not done so
by an external program (often called pager) (e.g. gnome-panel)
- maintaining WorskpacesInfo's properties 'count', 'rows', 'columns',
'orientation' and 'startingCorner' up to date and in sync with
_NET_NUMBER_OF_DESKTOPS and _NET_DESKTOP_LAYOUT
As it is in trunk the code has the following issues:
- if _NET_NUMBER_OF_DESKTOPS is not set and
WorkspacesInfo::updateWorkspaceGeometry decides to default to 4, the property
_NET_NUMBER_OF_DESKTOPS should be set to 4 as well
- "sanity checks" performed when _NET_DESKTOP_LAYOUT is set but incomplete
should be:
1) applied in all cases, even when _NET_DESKTOP_LAYOUT is not set
2) the case rows == 0 && columns == 0 should be smarter than just setting the
number of rows to 2 and instead compute a number of rows and columns that make
the layout be as close as possible to a square (as done in the patch)
- the function is simply too long to be easily understood. Here is a proposal
pseudo-code reorganisation:
getWorkspaceCountFromX(workspaceCount)
getWorkspacesLayoutFromX(orientation, columns, rows, startingCorner)
if workspaceCount not set:
workspaceCount = 4
setWorkspaceCountToX(workspaceCount)
if orientation not set:
orientation = horizontal
XLayoutNeedsUpdate = true
if columns and rows not set:
columns, rows = values to make up a square
XLayoutNeedsUpdate = true
else if columns not set:
columns = ceil(workspaceCount / rows)
XLayoutNeedsUpdate = true
else if rows not set:
rows = ceil(workspaceCount / columns)
XLayoutNeedsUpdate = true
if startingCorner not set:
startingCorner = topLeft
XLayoutNeedsUpdate = true
if XLayoutNeedsUpdate:
setWorkspacesLayoutToX(orientation, columns, rows, startingCorner)
updateLocalProperties(workspaceCount, orientation, columns, rows,
startingCorner)
The patch you propose has the following shortcomings of its own:
- the "best approximation for number of rows and cols" is improving on what was
done before (just setting the number of rows to 2) but yet duplicating the
logic making the code harder to follow
- 'updateX' is set to true when the member variable differs from the local
variable (e.g. m_rows != rows); that covers a more cases than it should and we
end up resetting _NET_DESKTOP_LAYOUT more than necessary. For example: the very
first time WorkspacesInfo::updateWorkspaceGeometry is called by the constructor
_NET_DESKTOP_LAYOUT is going to be set even though it might already contain all
the right values. _NET_DESKTOP_LAYOUT should only be set when it does not
contain one of the values needed to know the layout entirely (e.g. columns is
not set). Currently we end up not respecting the layout that an external pager
may have set.
- updating local members (m_columns, m_rows, etc.) and emitting changed signals
should be done at the very last moment in order to avoid the function being
re-entered. Concretely, setX11IntProperty(_NET_DESKTOP_LAYOUT, (unsigned
char*)newvalues, 4) should be done before.
- tiny niggles
* 'updateX' could be slightly more explicit. 'XLayoutNeedsUpdate' is one
possibility.
* 'newvalues' could be more explicit. For example 'newLayout' or even just
'layout' if we reuse it to also fetch the current _NET_DESKTOP_LAYOUT
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/715587
Title:
[workspace switcher] Inconsistency between workspace layout in
graphical switcher and that seen when using keyboard shortcuts
To manage notifications about this bug go to:
https://bugs.launchpad.net/unity-2d/+bug/715587/+subscriptions
--
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
