Merge lp:~abreu-alexandre/unity/dbus-stick-unstick-launcher-icon into lp:unity
Status: | Merged |
---|---|
Approved by: | Marco Trevisan (Treviño) on 2013-03-06 |
Approved revision: | 3168 |
Merged at revision: | 3188 |
Proposed branch: | lp:~abreu-alexandre/unity/dbus-stick-unstick-launcher-icon |
Merge into: | lp:unity |
Diff against target: |
184 lines (+125/-1) 3 files modified
launcher/LauncherController.cpp (+79/-1) launcher/LauncherControllerPrivate.h (+1/-0) tests/test_launcher_controller.cpp (+45/-0) |
To merge this branch: | bzr merge lp:~abreu-alexandre/unity/dbus-stick-unstick-launcher-icon |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
PS Jenkins bot (community) | continuous-integration | Approve on 2013-03-06 | |
Marco Trevisan (Treviño) | 2013-02-21 | Approve on 2013-03-06 | |
Review via email:
|
Description of the change
Add method to Launcher dbus interface to stick/unstick a launcher icon.
Needed in order to fix:
Marco Trevisan (Treviño) (3v1n0) wrote : | # |
- 3168. By Alexandre Abreu on 2013-02-27
-
Fix MR nits
Alexandre Abreu (abreu-alexandre) wrote : | # |
> 22 + std::string app_uri;
> 23 + if (desktop_
> 24 + return app_uri;
>
> You can just return "" in that case, without initializing app_uri.
Done.
> Also at the end of the function returning a temporary it's fine anyway..
mmh, it's returned by value & copy constructed ...
> 46 +Controller:
> icon_uri,
> 47 + bool sticky)
>
> Please, keep everything in one-line.
Done.
>
>
> 61 + AbstractLaunche
> 62 + GetIconByUri(
>
> One line, plus use const& (or auto const&).
Done.
>
>
> 72 + SortAndUpdate();
>
> Please avoid to do this, if nothing has been actually changed.
Good catch, done.
>
>
> 86 + else
> 87 + favorite_
>
> Add brackets here too please. However in that case, when can happen that a
> favorite has not a related application icon? Going in edge cases it's ok,
> anyway.
Done.
>
>
> 140 + ASSERT_
>
> This is quite redundant as it should be guarantee by other tests we have.
Yeah, but it is somewhat consistent w/ the test's logic & strict internals here imo,
Alexandre Abreu (abreu-alexandre) wrote : | # |
ping :)
Marco Trevisan (Treviño) (3v1n0) wrote : | # |
Ops, sorry I missed, this... About app_uri variable I meant to remove it at all, but it's not a big thing, so we can stay with it, considering that I can clean it when merging this with lp:~3v1n0/unity/gdbus-server-use, so approved! ;)
22 + std::string app_uri; path.empty( ))
23 + if (desktop_
24 + return app_uri;
You can just return "" in that case, without initializing app_uri.
Also at the end of the function returning a temporary it's fine anyway..
46 +Controller: :Impl:: OnLauncherUpdat eIconStickyStat e(std:: string const& icon_uri,
47 + bool sticky)
Please, keep everything in one-line.
61 + AbstractLaunche rIcon:: Ptr existing_icon_entry = target_ uri);
62 + GetIconByUri(
One line, plus use const& (or auto const&).
72 + SortAndUpdate();
Please avoid to do this, if nothing has been actually changed.
86 + else store.RemoveFav orite(target_ uri);
87 + favorite_
Add brackets here too please. However in that case, when can happen that a favorite has not a related application icon? Going in edge cases it's ok, anyway.
140 + ASSERT_ FALSE(favorite_ store.IsFavorit e(app_icon- >RemoteUri( )));
This is quite redundant as it should be guarantee by other tests we have.