Merge lp:~njpatel/unity/nicer-glib-signals into lp:unity
Status: | Merged |
---|---|
Merged at revision: | 1291 |
Proposed branch: | lp:~njpatel/unity/nicer-glib-signals |
Merge into: | lp:unity |
Diff against target: |
1961 lines (+1361/-170) 34 files modified
CMakeLists.txt (+3/-3) UnityCore/CMakeLists.txt (+3/-1) UnityCore/DBusIndicators.cpp (+79/-83) UnityCore/GLibSignal-inl.h (+299/-0) UnityCore/GLibSignal.cpp (+87/-0) UnityCore/GLibSignal.h (+307/-0) UnityCore/UnityCore.h (+0/-32) plugins/unityshell/src/FavoriteStoreGSettings.h (+1/-1) plugins/unityshell/src/IconTexture.cpp (+8/-7) plugins/unityshell/src/Launcher.cpp (+1/-1) plugins/unityshell/src/LauncherIcon.cpp (+1/-1) plugins/unityshell/src/PanelHomeButton.cpp (+1/-1) plugins/unityshell/src/PanelIndicatorObjectEntryView.cpp (+2/-1) plugins/unityshell/src/PanelIndicatorObjectEntryView.h (+1/-1) plugins/unityshell/src/PanelIndicatorObjectView.h (+1/-1) plugins/unityshell/src/PanelMenuView.cpp (+1/-1) plugins/unityshell/src/PanelTitlebarGrabAreaView.cpp (+1/-1) plugins/unityshell/src/PanelTray.h (+0/-2) plugins/unityshell/src/PanelView.cpp (+1/-1) plugins/unityshell/src/PanelView.h (+1/-1) plugins/unityshell/src/PlacesHomeView.cpp (+1/-1) plugins/unityshell/src/PlacesSearchBar.cpp (+1/-1) plugins/unityshell/src/PlacesSimpleTile.cpp (+1/-1) plugins/unityshell/src/PlacesView.cpp (+1/-1) plugins/unityshell/src/QuicklistMenuItem.cpp (+1/-1) plugins/unityshell/src/WindowButtons.cpp (+10/-20) po/unity.pot (+1/-1) tests/CMakeLists.txt (+16/-4) tests/test_glib_signals.cpp (+366/-0) tests/test_glib_signals_utils.cpp (+102/-0) tests/test_glib_signals_utils.h (+46/-0) tests/test_glib_signals_utils_marshal.list (+6/-0) tests/test_indicator_entry.cpp (+1/-1) tests/test_main.cpp (+10/-0) |
To merge this branch: | bzr merge lp:~njpatel/unity/nicer-glib-signals |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tim Penhey (community) | 2011-07-09 | Approve on 2011-07-15 | |
Unity Team | 2011-07-09 | Pending | |
Review via email:
|
Description of the change
This branch adds support for a cleaner way to connect to signals on GObjects inside Unity.
The idea behind the branch is that, with the current way, we need to take care of a few very annoying things:
- Connect the signal to a static member in our class and access members/variables from the callback via "self->"
- We need to remember to disconnect the signals (g_signal_
Both of these things have led to clumsy/cluttered code, or bugs where we've accidentally looked over something (like disconnecting).
The idea behind this branch is to:
- Allow the calling class to connect to signals on a GObject using sigc::slots (either sigc::ptr_fun or, more likely, sigc::mem_fun).
- Automatically handle disconnection of the signal on destruction of a Signal instance.
- Add a SignalManager class to manage multiple connections to different GObjects for an object. It will automatically disconnect the signals when it destructs.
Both these things mean that it's possible to ditch the multiple "static void Foo", "guint my_signal_id_", and "g_signal_
= API/Code =
I've tried to make it as clean as possible to use API-wise. My only real regret is having to use "new Signal<..." with a smartpointer to pass Signal instances to the SignalManager class. As explained in the code, this was mostly to strike a balance between a clean API and also to reduce the likelihood of bugs being introduced if the Signal class was passed in by reference and it's details "stolen" from it by the instance in the Vector (only one Signal class should manage a unique connection at any time).
Apart from that, I heard you like templates, so I put templates in your templates, so you can derive when you derive.
Oh, there're tests.
- 1287. By Neil J. Patel on 2011-07-09
-
[merge] trunk
- 1288. By Neil J. Patel on 2011-07-09
-
Some cleanups
Sam Spilsbury (smspillaz) wrote : | # |
- 1289. By Neil J. Patel on 2011-07-10
-
Duh, commit and push all the work before merge review
Neil J. Patel (njpatel) wrote : | # |
Thanks for reminding me, totally forgot to commit and push the generation before proposing the merge.
We already do this in services/
- 1290. By Neil J. Patel on 2011-07-10
-
some style fixes
Tim Penhey (thumper) wrote : | # |
OK Neil,
Well your description makes me keen to look at the code :-)
First question, since we are looking to use more glib stuff inside nux itself,
do you think perhaps the GLibSignal headers should go into nux itself?
Looking at a line like this:
signal_
(proxy_, "g-signal", sigc::mem_fun(this, &Impl::
It makes me wonder if we could simplify it. Given that the compiler has
enough type inference based on the sigc::mem_fun, we could perhaps get the
compiler to work out what glib::Signal we need, so we don't need to specify
it. I'm not entirely clear on the magic needed, but I'm 99% sure that it
could be done, allowing something like:
signal_
Perhaps for later...
Authored by in UnityCore/
Please don't use capital O, or lowercase L for any single character variables
or type markers. It becomes non-obvious as zero is a valid template arg
(if the template is expecting an int tempate param).
Also, aligning the params in a multiline arguement clause actually hinders
readability (for me) - see the "O object"
http://
recommends that the accessors look like:
GObject* object() const;
std::string const& name() const;
The Signal template class constructor should take the std::string as a const&.
You should make the SignalManager non-copyable, and it doesn't need to define
a destructor, and certainly not a virtual destructor. The add method should
take SignalBase::Ptr const& as the param, not SignalBase*.
UnityCore/
I'm not clear on why you want the GObject type defined in the Signal template
parameteres. It seems to me that they could be removed completely and just
use a GObject*. Oh... I see now. It is the first param to the callback.
This was a little confusing as you have the first param given a special name.
Comments in tests are OK :-) I'd like to know what
TestGLibSignals
Also, why do you sometimes pass in the address of a gboolean to
g_signal_
I don't undestand the gobject test signal class and marshallers, but I'll
trust you on that.
On the whole, a bit +1 from me, but ask if you want to fix the Add method to
not need new signal ...
- 1291. By Neil J. Patel on 2011-07-11
-
Don't use upper case o
- 1292. By Neil J. Patel on 2011-07-11
-
get_foo/foo
- 1293. By Neil J. Patel on 2011-07-11
-
Use std::string const& in Signal template
- 1294. By Neil J. Patel on 2011-07-11
-
Remove Signal manager destructor
Tim Penhey (thumper) wrote : | # |
r1294 fails to compile for me locally... I'm looking into it.
Tim Penhey (thumper) wrote : | # |
CMakeFiles/
test_glib_
test_glib_
test_glib_
and many more. It seems that the test is trying to link against the installed unity-core library rather than the one it has just compiled. Ideas?
- 1295. By Neil J. Patel on 2011-07-12
-
Prioritise library in UnityCore
- 1296. By Neil J. Patel on 2011-07-13
-
Switch to sigc::nil
- 1297. By Neil J. Patel on 2011-07-13
-
[merge] trunk
- 1298. By Neil J. Patel on 2011-07-13
-
[merge] c++0x support
- 1299. By Neil J. Patel on 2011-07-13
-
cleanup
Neil J. Patel (njpatel) wrote : | # |
> OK Neil,
>
> Well your description makes me keen to look at the code :-)
>
> First question, since we are looking to use more glib stuff inside nux itself,
> do you think perhaps the GLibSignal headers should go into nux itself?
Sure, though let's get through a review here and then I can move over to Nux.
> Looking at a line like this:
>
> signal_manager_.Add (new glib::Signal<void, GDBusProxy*, char*, char*,
> GVariant*>
> (proxy_, "g-signal", sigc::mem_fun(this, &Impl::
>
> It makes me wonder if we could simplify it. Given that the compiler has
> enough type inference based on the sigc::mem_fun, we could perhaps get the
> compiler to work out what glib::Signal we need, so we don't need to specify
> it. I'm not entirely clear on the magic needed, but I'm 99% sure that it
> could be done, allowing something like:
>
> signal_
> sigc::mem_fun(this, &Impl::
> Perhaps for later...
So, I implemented what we spoke about, however the compiler refuses to let it happen. It expects sigc::slot to be declared with the template types rather than being able to infer them from passes in sigc::bound_
Changing the arguments to take a sigc::bound_
I'm not against that, but I'm hoping I messed something up that another pair of eyes can take a look at. I've moved everything to sigc::nil, could you try your idea out and let me know if you've more luck with it?
> Authored by in UnityCore/
>
> Please don't use capital O, or lowercase L for any single character variables
> or type markers. It becomes non-obvious as zero is a valid template arg
> (if the template is expecting an int tempate param).
Both fixed.
> Also, aligning the params in a multiline arguement clause actually hinders
> readability (for me) - see the "O object"
I like it the way it is but not enough to care, can make that change before committing.
> http://
> recommends that the accessors look like:
>
> GObject* object() const;
> std::string const& name() const;
>
> The Signal template class constructor should take the std::string as a const&.
>
> You should make the SignalManager non-copyable, and it doesn't need to define
> a destructor, and certainly not a virtual destructor.
All fixed.
> The add method should
> take SignalBase::Ptr const& as the param, not SignalBase*.
Hmm, doing this means that manager->Add becomes Add(SignalBase:
> UnityCore/
Why?
> Comments in tests are OK :-) I'd like to know what
> TestGLibSignals
If the object is no longer around, we shouldn't do anything stu...
- 1300. By Neil J. Patel on 2011-07-13
-
Make SignalManager noncopyable
Tim Penhey (thumper) wrote : | # |
Hi Neil,
After several hours of fluffing around trying to get a nicer Add method, I've
given up as well. It seems that the sigc wrappers don't give us access to the
type information for the function call arguments. We could perhaps submit a
patch to the sigc guys, but not with this branch.
I forgot that the shared_ptr has an explicit constructor, so passing through
the shared pointer would be a bit cumbersome as you mention. So lets not
worry about that here.
By having UnityCore/
about, and creating an artificial dependency between our code and other
headers that have no impact on the code in the source file. This increases
coupling between the files and increases compilation time.
Tim Penhey (thumper) wrote : | # |
Actually, can you just change the using namespace sigc in the header to "using sigc::nul" ?
You should never have a using namespace * in a header.
- 1301. By Neil J. Patel on 2011-07-15
-
[merge] trunk
- 1302. By Neil J. Patel on 2011-07-15
-
s/using namepspace sigc/using sigc::nil/
- 1303. By Neil J. Patel on 2011-07-15
-
Get rid of UnityCore.h
- 1304. By Neil J. Patel on 2011-07-15
-
Try to not pad out the parameters for a method
Neil J. Patel (njpatel) wrote : | # |
Okay, so last few commits should hopefully make this good for merge. Getting rid of UnityCore.h must have inflated the diff a bit but couldn't help that.
Neil J. Patel (njpatel) wrote : | # |
Ah, forgot to answer the gboolean comment on this and Jason's merge proposal properly: bool and gboolean can be different sizes because "typedef int gboolean" (on my system sizeof(bool) = 1 and sizeof(
So, since tackling a lot of this at the end of the last cycle (we managed to fix some of the worst crashes by auditing the bool/gboolean code), I've tried to make absolutely sure that we don't mix them up at all, even if it might be okay in some cases, I'd rather be more strict about it.
Ideally we'd reach a point where the few areas where we hit gboolean/
Sam Spilsbury (smspillaz) wrote : | # |
Yep, that's static_cast <gboolean> ((bool) false) == TRUE :)
Tim Penhey (thumper) wrote : | # |
On Mon, 18 Jul 2011 02:56:22 you wrote:
> Yep, that's static_cast <gboolean> ((bool) false) == TRUE :)
Casting can the be the source of many many problems. This is just one of the
reasons I don't like them.
They are a useful tool, but just because you have a hammer doesn't mean you
need to hit everything with it.
Neil J. Patel (njpatel) wrote : | # |
Honestly I just do bool foo = my_gboolean == TRUE;
Seems the safest to me :)
Sent from my iPhone
On 17 Jul 2011, at 15:56, Sam Spilsbury <email address hidden> wrote:
> Yep, that's static_cast <gboolean> ((bool) false) == TRUE :)
>
> --
> https:/
> You are the owner of lp:~njpatel/unity/nicer-glib-signals.
Tim Penhey (thumper) wrote : | # |
On Mon, 18 Jul 2011 09:42:36 you wrote:
> Honestly I just do bool foo = my_gboolean == TRUE;
>
> Seems the safest to me :)
bool foo = my_gboolean is fine.
it is the casting of a bool to gboolean that is the issue.
The big issue here is exactly the above:
gboolean foo = 42; // entirely valid
foo -> true
bool(foo) -> true
foo != FALSE -> true
foo == TRUE -> false !!!!
We should not be doing == TRUE anywhere.
TRUE is defined to be !FALSE.
FALSE is defined to be 0.
The C (and C++) standard say that 0 is false, and anything else is considered
to be true. If we are checking for equality to TRUE on an integral type, we
may well have situations where the variable is neither TRUE nor FALSE. We
should only ever check for FALSE, or != FALSE. Checking for TRUE is a world
of pain for integral types.
Yo Dawg,
I went a little deeper into this code and found that in tests/ you've added what looks like generated marshallers for glib callbacks. It might be better to autogenerate that a build time instead of shipping it in the source distribution since the *.list could change whenever we need a new marshaller. For example
add_executable (test-signals ... ${CMAKE_ CURRENT_ BINARY_ DIR}/generated/ test_glib_ signals_ utils_marshal. cpp
...)
file (MAKE_DIRECTORY ${CMAKE_ CURRENT_ BUILD_DIR} /generated) CURRENT_ SOURCE_ DIR}/test_ glib_signals_ utils_marshal. list CURRENT_ BINARY_ DIR}/generated)
add_custom_command (TARGET test-signals PRE_BUILD
COMMAND glib-genmarshals ${CMAKE_
WORKING_DIRECTORY ${CMAKE_
That way we can create and perceive the marshallers at the same time, without the person doing the compiling ever knowing that!
(I can't comment on the nature of the code itself, knowing little about glib, however I do intend to run the tests and try it out)