Unity

Merge lp://qastaging/~dandrader/unity/lp940612 into lp://qastaging/unity

  1. lp940612
  2. Merge into trunk
Proposed by Daniel d'Andrada
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 2240
Proposed branch: lp://qastaging/~dandrader/unity/lp940612
Merge into: lp://qastaging/unity
Diff against target: 1178 lines (+831/-69)
19 files modified
plugins/unityshell/src/GeisAdapter.cpp (+10/-10)
plugins/unityshell/src/GeisAdapter.h (+10/-10)
plugins/unityshell/src/GestureEngine.cpp (+22/-40)
plugins/unityshell/src/GestureEngine.h (+1/-2)
tests/CMakeLists.txt (+14/-7)
tests/test-gesture-engine/CMakeLists.txt (+40/-0)
tests/test-gesture-engine/GeisAdapterMock.cpp (+30/-0)
tests/test-gesture-engine/GeisAdapterMock.h (+148/-0)
tests/test-gesture-engine/PluginAdapterMock.cpp (+33/-0)
tests/test-gesture-engine/PluginAdapterMock.h (+37/-0)
tests/test-gesture-engine/X11_mock.cpp (+40/-0)
tests/test-gesture-engine/X11_mock.h (+37/-0)
tests/test-gesture-engine/compiz_mock/core/core.h (+29/-0)
tests/test-gesture-engine/compiz_mock/core/screen.h (+68/-0)
tests/test-gesture-engine/compiz_mock/core/window.h (+66/-0)
tests/test-gesture-engine/sed_script (+14/-0)
tests/test-gesture-engine/test_gesture_engine.cpp (+165/-0)
tests/test-gesture-engine/ubus-server-mock.cpp (+32/-0)
tests/test-gesture-engine/ubus-server-mock.h (+35/-0)
To merge this branch: bzr merge lp://qastaging/~dandrader/unity/lp940612
Reviewer Review Type Date Requested Status
Tim Penhey (community) Approve
Review via email: mp+99956@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2012-03-28.

Commit message

Fix the three-finger move for unity.

Description of the change

Fixes bug #940612.

Updated according to comments from previous proposal.

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote : Posted in a previous version of this proposal

Please keep the variable type and variable name separate:
  const CompWindowVector &client_list_stacking
should be
  CompWindowVector const& client_list_stacking
(or
  const CompWindowVector& client_list_stacking
   - but my preference is for the first)

> if (client_list_stacking.size() == 0)

can just be:

  if (client_list_stacking.empty())

> int pos_x = (int) fpos_x;

Please don't use C style casts. It isn't needed here (mostly
certain about that).

The do/while loop misses the first element of the vector if there
is more than one element.

A traditional for loop is more idiomatic and understandable.
Also, can remove the window and result temporaries from outside the loop.
And... if using the iterators, you don't need the empty check first
as the initial check makes sure of that too.

for (auto iter = client_list_stacking.rbegin(),
          end = client_list_stacking.rend();
     iter != end; ++iter)
{
    CompWindow* window = *iter;

    if (pos_x >= window->x() && pos_x <= (window->width() + window->x())
        &&
        pos_y >= window->y() && pos_y <= (window->height() + window->y()))
      return window;
}

return nullptr;

review: Needs Fixing
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Updated. The loop sure looks better now.

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

You don't need this check in the method any more, because it is caught by rbegin() == rend().

171 + if (client_list_stacking.empty())
172 + return nullptr;

Once you push, you don't need to resubmit. Launchpad will just update the diff.

Do we have any tests for this?

Is there any way to "fake" utouch events?

review: Needs Fixing
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

> You don't need this check in the method any more, because it is caught by
> rbegin() == rend().
>
> 171 + if (client_list_stacking.empty())
> 172 + return nullptr;

True. Line removed.

> Once you push, you don't need to resubmit. Launchpad will just update the
> diff.

Alright. I've pushed the updated version. Let's see.

> Do we have any tests for this?

No.

> Is there any way to "fake" utouch events?

Only if you do some mocking in a unity test.

But you can record and then replay multitouch events straight out of a /dev/input/eventX file using utouch-evemu-tools (for the recording) and libutouch-evemu1 in your test. But it would be a functional or integration test since several layers would be involved (events coming out of /dev/input/eventX into xserver and then sent as XInput2 events to compiz)

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

Unless autopilot tests are trivial to add for this, I'd suggest we have a manual test for now.

There is a template in the manual-tests directory.

Code looks fine now.

review: Needs Fixing
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

> Unless autopilot tests are trivial to add for this, I'd suggest we have a
> manual test for now.
> [...]

It now has unit tests.

Revision history for this message
Tim Penhey (thumper) :
review: Approve

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.