Unity

Merge lp://qastaging/~unity-team/unity/3v1n0-quick-alt+tab-fixes into lp://qastaging/unity

  1. 3v1n0-quick-alt+tab-fixes
  2. Merge into trunk
Proposed by Marco Trevisan (Treviño)
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 2282
Proposed branch: lp://qastaging/~unity-team/unity/3v1n0-quick-alt+tab-fixes
Merge into: lp://qastaging/unity
Diff against target: 815 lines (+336/-97)
16 files modified
manual-tests/Switcher.txt (+1/-1)
plugins/unityshell/src/BamfLauncherIcon.cpp (+45/-30)
plugins/unityshell/src/Launcher.cpp (+3/-1)
plugins/unityshell/src/PluginAdapter.cpp (+91/-47)
plugins/unityshell/src/PluginAdapter.h (+4/-1)
plugins/unityshell/src/SwitcherController.cpp (+1/-0)
plugins/unityshell/src/UScreen.cpp (+9/-3)
plugins/unityshell/src/UScreen.h (+1/-0)
plugins/unityshell/src/WindowManager.cpp (+11/-1)
plugins/unityshell/src/WindowManager.h (+5/-2)
plugins/unityshell/src/unityshell.cpp (+8/-5)
tests/autopilot/autopilot/emulators/bamf.py (+11/-2)
tests/autopilot/autopilot/emulators/unity/switcher.py (+3/-0)
tests/autopilot/autopilot/tests/__init__.py (+12/-0)
tests/autopilot/autopilot/tests/test_launcher.py (+65/-0)
tests/autopilot/autopilot/tests/test_switcher.py (+66/-4)
To merge this branch: bzr merge lp://qastaging/~unity-team/unity/3v1n0-quick-alt+tab-fixes
Reviewer Review Type Date Requested Status
Thomi Richards (community) quality Approve
Tim Penhey (community) Approve
Sam Spilsbury Pending
Alex Launi quality Pending
Review via email: mp+102028@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2012-04-05.

Commit message

Fix the behaviour of alt-tab and clicking on the launcher icon to just raise the most recently used window, not all for that app.

BamfLauncherIcon's activation related code has been updated to be more multimonitor aware (for free I've fixed also some bugs that caused the windows not to be put in spread mode in multi-monitor), and to support both the "Launcher only on Primary Monitor" and "Launcher on all monitors" options.

PluginAdapter's FocusWindowGroup method has been updated to optionally only unminimize / raise and activate only the top window. This code would have been more optimized using a reverse iterator to fetch the top_window, but not to change the whole logic and to allow to keep the previous behavior (that initially we wanted for "long alt+tab") without duplicating code, I've just hacked that.
Implemented also GetWindowMonitor to workaround the mismatch we had with the compiz' window->outputDevice() and the UScreen values that now we use in the whole unity.

Screencast of the fixed version: http://ubuntuone.com/7YaWciQnaZHfzr35asSz0N

Description of the change

== Problem ==
Launcher, Alt-Tab - clicking on launcher item or selecting a app in Alt-Tab raises all app windows, not just most recently focused.

== Fix ==
BamfLauncherIcon's activation related code has been updated to be more multimonitor aware (for free I've fixed also some bugs that caused the windows not to be put in spread mode in multi-monitor), and to support both the "Launcher only on Primary Monitor" and "Launcher on all monitors" options.

PluginAdapter's FocusWindowGroup method has been updated to optionally only unminimize / raise and activate only the top window. This code would have been more optimized using a reverse iterator to fetch the top_window, but not to change the whole logic and to allow to keep the previous behavior (that initially we wanted for "long alt+tab") without duplicating code, I've just hacked that.
Implemented also GetWindowMonitor to workaround the mismatch we had with the compiz' window->outputDevice() and the UScreen values that now we use in the whole unity.

Screencast of the fixed version: http://ubuntuone.com/7YaWciQnaZHfzr35asSz0N

== Test ==
There are autopilot test for both bugs.

Thanks to Brandon for taking care of merging the old lp:~3v1n0/unity/quick-alt+tab-fixes with trunk and for the AP tests that I used as my base.

UNBLOCK

To post a comment you must log in.
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote : Posted in a previous version of this proposal

30 - if ((mapped && wm->IsWindowMapped(xid)) || !mapped)
31 + if ((mapped && wm->IsWindowMapped(xid) && !bamf_window_get_transient(BAMF_WINDOW(view))) || !mapped)
32 {

About this, I think I included that change mostly for testing purposes, I'm not sure it's actually wanted so move it out.

I didn't work too much on my branch lately since I didn't know if that was still the wanted desig, but if now it is, I'll be happy to get this merged :)

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote : Posted in a previous version of this proposal

I ran all the autopilot test that I thought these changes would effect.

Launcher Autopilot tests:
Ran 43 tests in 296.560s
OK

Switcher Autopilot tests:
Ran 21 tests in 187.666s
OK

Dash Autopilot tests:
Ran 32 tests in 225.549s
OK

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Tests OK, no iconic window regressions (which is what I was a little concerned about)

review: Approve
Revision history for this message
Tim Penhey (thumper) wrote : Posted in a previous version of this proposal

Clicking on the launcher to reveal only the top most one works.

However the quick alt-tab one does not work.

If I have two terminal windows and one gedit. Focus gedit and quick alt-tab, it raises both terminals.

review: Needs Fixing
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote : Posted in a previous version of this proposal

Probably it is caused by the fact that now we use a very small delay for the Alt+Tab.
A workaround could be to use another time reference to consider an Alt+Tab quick, more than the only window visibility.

I mean, even if the window is shown for 100ms or less, the Alt+Tab is surely a quick one (as there's no time to look to the content of the window).

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote : Posted in a previous version of this proposal

I just ended up making a timer called quick_tab_timer_, which will go for 200ms if you hold on to alt for any longer it makes quick_tab = false. Seems like a nice and customizable way.

Revision history for this message
Alex Launi (alexlauni) wrote : Posted in a previous version of this proposal

It shouldn't be too difficult to automate the manual tests. Please do not merge this with the manual tests.

review: Needs Fixing (quality)
Revision history for this message
Brandon Schaefer (brandontschaefer) wrote : Posted in a previous version of this proposal

@Alex

Sorry about having the manual test in there. I was trying to get this branch in 5.10 and was rushing to get it done. Never a good sign when it comes to quality. There are now autopilot test for each bug!

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote : Posted in a previous version of this proposal

So I guess diff continutes to complain about there not being a new line...I even took the trunk version of Switcher.txt and replaced it and it still gives that problem...

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote : Posted in a previous version of this proposal

I've just checked this code again, there's a problem with the minimized windows when using the launcher icon.
Don't worry about that Brandon, I'll take care of that ;)

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote : Posted in a previous version of this proposal

Yeah, I just saw that. At lease we know what design wants now :). Thank you!

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

Awesome work! Confirmed everything I could with only 1 monitor, bot AP test pass as well as doing a lot of manual testing :)

Revision history for this message
Tim Penhey (thumper) wrote :

The code looks fine, and I've tested the behaviour and tweaked the AP tests. Thomi will review the tests.

review: Approve
Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote :

+1

review: Approve (quality)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.