Merge lp:~bowmore/unity/fix_icon_workarea_issues into lp:unity

Proposed by Bowmore
Status: Superseded
Proposed branch: lp:~bowmore/unity/fix_icon_workarea_issues
Merge into: lp:unity
Diff against target: 28 lines (+2/-2)
2 files modified
plugins/unityshell/src/Launcher.cpp (+1/-1)
plugins/unityshell/src/PanelController.cpp (+1/-1)
To merge this branch: bzr merge lp:~bowmore/unity/fix_icon_workarea_issues
Reviewer Review Type Date Requested Status
Unity Team Pending
Gord Allott Pending
Review via email: mp+102174@code.launchpad.net

This proposal has been superseded by a proposal from 2012-07-23.

Description of the change

This change fixes the sporadic compression of the desktop icon workarea in height.

The reason is that the basewindow geometry initially is used for the panel and launcher struts and thus results in a top margin (strut) with the height 300ps instead of 24px that applies to the unity panel, thus losing 276px in workspace height.

This is sporadic (a race condition issue between unity/compiz and nautilus) but more frequent on older computers.

Also see bug #886667

To post a comment you must log in.
Revision history for this message
Gord Allott (gordallott) wrote :

Hi, so this code looks fine, However i'm wondering about testing. we have a system called autopilot but i don't think this is a good fit for that unless there is a nice repeatable methodology for triggering this bug.

At the very least there are manual tests that we have written up in the manual-tests/ directory of unity, if you could add a test in there for this bug that follows the same syntax as the rest that would be great

Revision history for this message
Bowmore (bowmore) wrote :

As a former tester and designer some 20+ years ago I'm familiar with the auto- and manual test concepts as such.

First I would appreciate some complete links for those that I can study and learn from for future use. Not sure what directory "manual-tests/ directory of unity" refers to.

About this bug there is no easy way even to set up a manual test case. That would e.g require some smart patching to catch any improper writing into the struts in Nux. Furthermore, it is obvious that Unity initially sets an improper geometry (nux basewindow defaults) which has to be fixed anyway which my proposed patch suggests.

Basically what happens is that the nux basewindow geometry for e.g the launcher here is treated as a top strut (height=300) in Nux acc to the algorithm at the end of its function XInputWindow::GetStrutsData(). This setting then is not updated until the correct panel height (24) is set. That is, sporadically we can end up with a too small height for the Nautilus workarea dependent on when Nautilus is triggered to draw the workarea.

Then, how to tie and verify this change towards bug #886667?
Well, that's not an easy one, i.e first to show the wrong settings and then to see to that Nautilus catches those. I've no idea how to achieve this with a manual test case.

A final solution for the Unity session must be to implement startup phases acc to:
http://live.gnome.org/SessionManagement/NewGnomeSession
That would at least "hide" bugs like these ;)

Revision history for this message
Bowmore (bowmore) wrote :

What about a workspace test case including four corner desktop icons, one at each corner. Then running a number of restarts, let's say three, to check whether any of those have moved from their original positions. If so the test has failed.

This is more of a general test case for the workspace size consistancy that probably already (should) exist. There are a few other minor workspace issues as well that might be revealed this way.

Unfortunatelly my old ati graphic card broke down so replaced it with an nvidia legacy that require nvidia-173 so waiting for an upgrade to video abi 11 to be able to run unity again.

Revision history for this message
Bowmore (bowmore) wrote :

I see no action on this merge proposal.
Hereby withdrawing my bug assignment.

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

Please, could you merge this with trunk again?
I get a conflict in in panel/PanelController.cpp due to the latest trunk changes.

I think that this is quite hard to test, however could you at least please include some comments saying why the InputWindowEnableStruts calls should be after the geometry setting?

Thank you, and sorry for the delay.

Revision history for this message
Bowmore (bowmore) wrote :

Thanks for the response.

I haven't been able to compile Unity to trace this issue as I'm not yet familiar with its patch method so have just made a desk check here. So there might be some issues with the patch that I haven't foreseen.

As I recall it the main issue was the setting of the launcher's strut that uses the basewindow default geometry instead of the launcher's dito causing this bug. Concerning the setting of the panel strut this was more kind of a bad coding pratice, i.e doing things in the wrong order not being intuitive but seems to end up to work anyway maybe due to later patchings.

Basically, first update the launcher/panel geometry, then update the corresponding strut for that geometry.

I will do a recheck and come back with more details.

Revision history for this message
Bowmore (bowmore) wrote :

Made a recheck of the panelcontroller part.

window->InputWindowEnableStruts(true)
-------------------------------------
Calls EnableStruts(true) to enable struts for the panel basewindow that in turn calls SetStruts.
SetStruts then uses the current geometry (geometry_) in nux which here still holds the basewindow default geometry (100,100,320,200) that will create a top strut for that basewindow with the height of 300px acc to XInputWindow::GetStrutsData(). This is wrong!

window->SetGeometry(geo)
------------------------
Updates the geometry (geometry_) in nux to the panel's geometry and calls SetStruts() again as struts is now enabled.
SetStruts then uses the current geometry (geometry_) which here now holds the panel's geometry and creates a correct top strut overwriting the previous top strut setting for that basewindow. Fortunally this works as the basewindow geometry also happens to result in a top strut, otherwise we would end up with two struts for that basewindow!

The intension with the fix was to first call SetGeometry to update the basewindow geometry. As struts is not yet enabled InputWindowEnableStruts(true) is then required to enable struts to set the strut for that basewindow. Not sure however what conflict this fix causes.

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

I think what Marco meant is for you to update your branch to the current unity trunk, since merging from your branch to trunk results in a merge conflict in panel/PanelController.cpp. If that won't be done, the branch - even when approved, cannot get merged into trunk. It has to be made merge-able first.

Revision history for this message
Bowmore (bowmore) wrote :

Thanks, downloaded he current branch and discovered that both the Launcher and PanelController have been moved to other directory. Then the question is how do I update a merge proposal?

First time (new) I did this:

bzr commit
bzr push lp:~bowmore/unity/fix_icon_workarea_issue
bzr lp-open lp:~bowmore/unity/fix_icon_workarea_issues

Can I do the same to replace the current patch with a new one, or?

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

Hi Bowmore,

Do the following:

bzr merge lp:unity
// fix conflicts
bzr resolve
bzr commit -m "Merge trunk, resolved conflicts."
bzr push # It should remember the previous location

The diff here will automatically update.

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

I've done the resolution myself, and will resubmit

Revision history for this message
Bowmore (bowmore) wrote :

Thanks for the howto and your resubmit:
https://code.launchpad.net/~thumper/unity/fix_icon_workarea_issues/+merge/116388
Looks fine to me ;-)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'plugins/unityshell/src/Launcher.cpp'
--- plugins/unityshell/src/Launcher.cpp 2012-04-15 12:13:39 +0000
+++ plugins/unityshell/src/Launcher.cpp 2012-04-16 20:01:54 +0000
@@ -1583,8 +1583,8 @@
1583void1583void
1584Launcher::UpdateOptions(Options::Ptr options)1584Launcher::UpdateOptions(Options::Ptr options)
1585{1585{
1586 SetIconSize(options->tile_size, options->icon_size);
1586 SetHideMode(options->hide_mode);1587 SetHideMode(options->hide_mode);
1587 SetIconSize(options->tile_size, options->icon_size);
15881588
1589 ConfigureBarrier();1589 ConfigureBarrier();
1590 EnsureAnimation();1590 EnsureAnimation();
15911591
=== modified file 'plugins/unityshell/src/PanelController.cpp'
--- plugins/unityshell/src/PanelController.cpp 2012-04-04 20:21:45 +0000
+++ plugins/unityshell/src/PanelController.cpp 2012-04-16 20:01:54 +0000
@@ -252,10 +252,10 @@
252 window->SetBackgroundColor(nux::Color(0.0f, 0.0f, 0.0f, 0.0f));252 window->SetBackgroundColor(nux::Color(0.0f, 0.0f, 0.0f, 0.0f));
253 window->ShowWindow(true);253 window->ShowWindow(true);
254 window->EnableInputWindow(true, "panel", false, false);254 window->EnableInputWindow(true, "panel", false, false);
255 window->InputWindowEnableStruts(true);
256 window->SetGeometry(geo);255 window->SetGeometry(geo);
257 window->SetMinMaxSize(geo.width, geo.height);256 window->SetMinMaxSize(geo.width, geo.height);
258 window->SetLayout(layout);257 window->SetLayout(layout);
258 window->InputWindowEnableStruts(true);
259259
260 windows_.push_back(window);260 windows_.push_back(window);
261261