Unity

Merge lp://qastaging/~smspillaz/unity/unity.extract_switcher_interface into lp://qastaging/unity

  1. unity.extract_switcher_interface
  2. Merge into trunk
Proposed by Sam Spilsbury
Status: Rejected
Rejected by: Stephen M. Webb
Proposed branch: lp://qastaging/~smspillaz/unity/unity.extract_switcher_interface
Merge into: lp://qastaging/unity
Diff against target: 1080 lines (+489/-133)
15 files modified
launcher/SwitcherController.cpp (+1/-1)
launcher/SwitcherController.h (+51/-12)
launcher/SwitcherView.cpp (+61/-30)
launcher/SwitcherView.h (+50/-9)
plugins/unitydialog/src/unitydialog.cpp (+1/-1)
plugins/unityshell/src/GesturalWindowSwitcher.cpp (+10/-8)
plugins/unityshell/src/unitya11y.cpp (+1/-1)
plugins/unityshell/src/unityshell.cpp (+1/-0)
tests/MockSwitcherController.h (+62/-0)
tests/MockSwitcherView.h (+54/-0)
tests/StubSwitcherController.h (+155/-0)
tests/test-gestures/SwitcherControllerMock.h (+31/-2)
tests/test-gestures/sed_script_switcher (+1/-0)
tests/test-gestures/test_gestural_window_switcher.cpp (+1/-1)
tests/test_switcher_controller.cpp (+9/-68)
To merge this branch: bzr merge lp://qastaging/~smspillaz/unity/unity.extract_switcher_interface
Reviewer Review Type Date Requested Status
Sam Spilsbury (community) Needs Resubmitting
Stephen M. Webb (community) Disapprove
Marco Trevisan (Treviño) Pending
Tim Penhey Pending
Andrea Azzarone Pending
Unity Team Pending
Review via email: mp+140398@code.qastaging.launchpad.net

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

Commit message

Split the interface and implementation of SwitcherView and SwitcherController.

switcher::Controller is now split into switcher::Controller and
switcher::ControllerInterface . View is likewise split. GetView () now returns
the interface.

A new convenience method ConnectToMouseEvents was added, so that we don't
have to depend on nux::View in test scenarios.

MockSwitcherController was converted to Google Mock, the other
MockSwitcherController was renamed to StubSwitcherController

Description of the change

Split the interface and implementation of SwitcherView and SwitcherController.

switcher::Controller is now split into switcher::Controller and
switcher::ControllerInterface . View is likewise split. GetView () now returns
the interface.

A new convenience method ConnectToMouseEvents was added, so that we don't
have to depend on nux::View in test scenarios.

MockSwitcherController was converted to Google Mock, the other
MockSwitcherController was renamed to StubSwitcherController

Split off from (lp:~smspillaz/unity/unity.gesture_tests_no_sed)

I realize SwitcherViewInterface, SwitcherViewImpl, switcher::ControllerInterface are lame names. I'm not really sure what to do here. For the former at least, what I do for compiz is something like this:

namespace switcher
{
class SwitcherView ....
namespace impl
{
class SwitcherView
}
}

And then access both through their namespace qualifiers.

For the latter its a tad trickier. We can't just rename switcher::ControllerInterface to be switcher::Controller as its not really the full and complete interface for switcher::Controller (switcher::Controller inherits from nux::Object and other things which can't be mocked out)

To post a comment you must log in.
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Ping, other work blocked on this.

Revision history for this message
Andrea Azzarone (azzar1) wrote : Posted in a previous version of this proposal

84 + virtual void Show(ShowMode show, SortMode sort, std::vector<launcher::AbstractLauncherIcon::Ptr> results);
85 + virtual void Hide(bool accept_state=true);

We use gcc 4.7 so we can use override keyword ;)

+#include <time.h>

Not sure this is really needed.

=== added file 'tests/MockWindowManager.h'

???

Also some code does not use unity-stile and I don't like the abuse of impl keyword in:

impl::SwitcherViewImpl::Ptr(new impl::SwitcherViewImpl());

I just prefer to have SwitcherView for the interface and SwitcherViewImpl for the implementation but it's not a problem :)

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

> 84 + virtual void Show(ShowMode show, SortMode sort,
> std::vector<launcher::AbstractLauncherIcon::Ptr> results);
> 85 + virtual void Hide(bool accept_state=true);
>
> We use gcc 4.7 so we can use override keyword ;)
>
> +#include <time.h>
>
> Not sure this is really needed.

I'll get rid of it

>
>
> === added file 'tests/MockWindowManager.h'
>
> ???

Should be fixed by a trunk merge, thanks.

>
>
> Also some code does not use unity-stile and I don't like the abuse of impl
> keyword in:
>
> impl::SwitcherViewImpl::Ptr(new impl::SwitcherViewImpl());
>
> I just prefer to have SwitcherView for the interface and SwitcherViewImpl for
> the implementation but it's not a problem :)

I'll see if there's a way we can make it back into SwitcherView (for the interface)

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

It looks fine overall...

412 + typedef std::tuple <sigc::connection,
413 + sigc::connection,
414 + sigc::connection> MouseEventConnections;

Since they're all of the same type, why not using a std::array instead?

728 === added file 'tests/MockWindowManager.cpp.moved'
747 === added file 'tests/MockWindowManager.h.moved'

This probably needs to be re-merged?

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

On Tue, Dec 18, 2012 at 1:37 AM, Marco Trevisan (Treviño)
<mail@3v1n0.net> wrote:
> It looks fine overall...
>
> 412 + typedef std::tuple <sigc::connection,
> 413 + sigc::connection,
> 414 + sigc::connection> MouseEventConnections;
>
> Since they're all of the same type, why not using a std::array instead?

I've not looked into std::array - thanks for the tip. Does it have
similar semantics to tuple?

>
> 728 === added file 'tests/MockWindowManager.cpp.moved'
> 747 === added file 'tests/MockWindowManager.h.moved'
>
> This probably needs to be re-merged?

bah, yes.
> --
> https://code.launchpad.net/~smspillaz/unity/unity.extract_switcher_interface/+merge/137722
> You are the owner of lp:~smspillaz/unity/unity.extract_switcher_interface.

--
Sam Spilsbury

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

Re-merge complete

Revision history for this message
Stephen M. Webb (bregma) wrote :

This MP has too many unrelated changes: rafactoring the controller into interface/concrete classes, refactoring the view into interface/concrete classes, and adding mouse event connections.

Please break this into three separate MPs, each with a single purpose.

Additional note:

The view refactoring I find a little unclear: the controller refactor follows the traditional use of making the interface explicit in its name, but the view instead uses a subnamespace impl and embeds the work Impl into the concrete class name. Impl often implies a private internal implementation (pimpl). Keeping the naming convention consistent makes it easier for a reader to grasp the structure quickly.

review: Disapprove
Revision history for this message
Sam Spilsbury (smspillaz) wrote :

> This MP has too many unrelated changes: rafactoring the controller into
> interface/concrete classes, refactoring the view into interface/concrete
> classes, and adding mouse event connections.
>
> Please break this into three separate MPs, each with a single purpose.

I can do this, however keep in mind that most of lp:~smspillaz/unity/unity.gesture_tests_no_sed is a rather large change to break out a number of different interfaces - continuing to subdivide patches will result in long iteration times and effectively mean that work which will substantially reduce compile time will almost never be merged, which will be a shame because a number of people I've spoken to are quite interested in that.

I did consider splitting this into three patches initially, but I didn't do that because the changes, although not strictly coupled, were loosely related in substance and context.

Considering that we now have a veto, I will split this into three different patches.

>
>
> Additional note:
>
> The view refactoring I find a little unclear: the controller refactor follows
> the traditional use of making the interface explicit in its name, but the view
> instead uses a subnamespace impl and embeds the work Impl into the concrete
> class name. Impl often implies a private internal implementation (pimpl).
> Keeping the naming convention consistent makes it easier for a reader to grasp
> the structure quickly.

I can go with switcher::ViewInterface and switcher::View. Using the ::impl namespace is generally a convention I use in compiz to avoid class name encoding, however I really don't know what the accepted convention is in unity. Some people I've spoken to hate the -Interface, some prefer -Impl, others prefer namespaces, others prefer totally different names. Its a bit of a mess really :(

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

See the new proposals starting from here: lp:~smspillaz/unity/extract_switcher_controller_interface

review: Needs Resubmitting
Revision history for this message
Sam Spilsbury (smspillaz) wrote :

(Somebody else needs to reject this. Alas I am "community").

Unmerged revisions

2954. By Sam Spilsbury

Remove .moved files

2953. By Sam Spilsbury

Merge lp:unity

2952. By Sam Spilsbury

SwitcherViewInterface -> SwitcherView

2951. By Sam Spilsbury

Remove time.h

2950. By Sam Spilsbury

Merge lp:unity

2949. By Sam Spilsbury

Split the interface and implementation of SwitcherView and SwitcherController.

switcher::Controller is now split into switcher::Controller and
switcher::ControllerInterface . View is likewise split. GetView () now returns
the interface.

A new convenience method ConnectToMouseEvents was added, so that we don't
have to depend on nux::View in test scenarios.

MockSwitcherController was converted to Google Mock, the other
MockSwitcherController was renamed to StubSwitcherController

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.