Merge lp:~aacid/unity/do_not_reuse_menus_on_order_change into lp:unity
| Status: | Merged |
|---|---|
| Approved by: | Marco Trevisan (Treviño) on 2012-10-09 |
| Approved revision: | 2837 |
| Merged at revision: | 2815 |
| Proposed branch: | lp:~aacid/unity/do_not_reuse_menus_on_order_change |
| Merge into: | lp:unity |
| Diff against target: |
662 lines (+471/-6) 11 files modified
UnityCore/DBusIndicators.cpp (+28/-5) UnityCore/DBusIndicators.h (+4/-0) UnityCore/Indicator.cpp (+15/-0) UnityCore/Indicator.h (+1/-0) tests/CMakeLists.txt (+4/-1) tests/test_dbus_indicators.cpp (+99/-0) tests/test_indicator.cpp (+3/-0) tests/test_service_main.c (+4/-0) tests/test_service_panel.c (+253/-0) tests/test_service_panel.h (+46/-0) tests/test_utils.h (+14/-0) |
| To merge this branch: | bzr merge lp:~aacid/unity/do_not_reuse_menus_on_order_change |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Marco Trevisan (Treviño) | 2012-10-05 | Approve on 2012-10-08 | |
|
Review via email:
|
|||
Commit Message
Do not reuse the menu entries if their order changes
Since the indicators api only have "add" and "remove" signals, if we reuse an entry that is not in the correct order it will case the menus or whatever this indicator represents to be in the wrong order
Description of the Change
Do not reuse the menu entries if their order changes
Since the indicators api only have "add" and "remove" signals, if we reuse an entry that is not in the correct order it will case the menus or whatever this indicator represents to be in the wrong order
| Albert Astals Cid (aacid) wrote : | # |
| Albert Astals Cid (aacid) wrote : | # |
Unit test added
| Marco Trevisan (Treviño) (3v1n0) wrote : | # |
112 + DBusIndicators(
In unity we prefer to use this syntax instead:
std::string const& dbus_name
8 -const std::string SERVICE_
However, probably instead of adding an optional parameter, I would add another one (better if protected, so that you'll make accessible only in a test mock class) that takes the service name parameter.
91 + : connected(false), pimpl(new Impl(dbus_name, this))
Better two lines here.
115 + nux::Property<bool> connected;
Don't use a property here, for two reasons: a non-RO property like is this one, is meant to be editable, while you only use to store a value; also you don't really need that... Just add an IsConnected() (protected as well?) method that proxies gproxy_
On the testing side, yes... That's what I meant: good! :)
However, on code side you can be more clean by using the TestDBusIndicators class: so, add in the constructor (or in a SetUp() function) the calls you're duplicating in the tests to initialize them, and the cleanup calls in a TearDown() method.
Also instead of using g_timeout, you can use glib::Timeout, since probably you won't able to use utils::WaitUntil
+ session = g_bus_get_
I guess that using a glib::DbusProxy would be better here.
182 === added file 'tests/
I'd prefer it to be named test_dbus_
PS: also cleanup the commented code in test.
| Albert Astals Cid (aacid) wrote : | # |
ok, all suggestions should be implemented if i did not miss something
| Marco Trevisan (Treviño) (3v1n0) wrote : | # |
331 + DBusIndicatorsTest* dbus_indicators;
Would be nice to have DBusIndicators::Ptr instead, so you don't need to worry about destruction (unless it must be done before unreffing the loop)
336 +TEST_F(
337 +{
338 + SetUp();
357 + TearDown();
358 +}
No need to do this... GTest will do this automatically, so forget about setting-up and tearing-down once you've defined these testing::Test functions.
341 + auto timeout_check = [&] () -> bool
342 + {
343 + nChecks++;
344 + bool quit_loop = dbus_indicators
345 + if (quit_loop)
346 + g_main_
347 + return !quit_loop;
348 + };
349
350 + nChecks = 0;
351 + timeout.reset(new glib::Timeout(100, timeout_check));
352 +
353 + g_main_
No need to repeat this for every test...
Just do it in the SetUp function, and probably instead of quitting the loop is enough to add an ASSERT there.
| Marco Trevisan (Treviño) (3v1n0) wrote : | # |
108 + GVariant *CallSync(string const& method_name,
109 + GVariant* parameters,
Ah, please also add the * near to the typename (i.e. GVariant*) and, fix the parameters indentation.
| Albert Astals Cid (aacid) wrote : | # |
Second round of updates hope i did not miss anything :-)
| Marco Trevisan (Treviño) (3v1n0) wrote : | # |
Thanks a lot for your changes (and patience), it looks very nice now! ;)
| Unity Merger (unity-merger) wrote : | # |
The Jenkins job https:/
Not merging it.


Working in a unittest