Merge lp://qastaging/~3v1n0/unity/lim into lp://qastaging/unity
- lim
- Merge into trunk
Status: | Merged |
---|---|
Merge reported by: | Marco Trevisan (Treviño) |
Merged at revision: | not available |
Proposed branch: | lp://qastaging/~3v1n0/unity/lim |
Merge into: | lp://qastaging/unity |
Diff against target: |
200 lines (+130/-9) 3 files modified
UnityCore/AppmenuIndicator.cpp (+47/-5) UnityCore/AppmenuIndicator.h (+17/-3) tests/test_indicator_appmenu.cpp (+66/-1) |
To merge this branch: | bzr merge lp://qastaging/~3v1n0/unity/lim |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Thomi Richards | Pending | ||
Andrea Azzarone | Pending | ||
Review via email:
|
This proposal supersedes a proposal from 2012-01-25.
Commit message
Description of the change
Service and UnityCore work for locally integrated menus.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Marco Trevisan (Treviño) (3v1n0) wrote : Posted in a previous version of this proposal | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
862 + if (sscanf (entry_id, "%p", &entry) == 1)
863 + {
864 + /* Checking that entry is really an IndicatorObject
865 + * To do that, we use an hack that allows to check if the pointer we read is
866 + * actually a valid pointer without crashing. This can be done using the
867 + * low-level write function to read from the pointer to a valid fds (we use
868 + * a pipe for convenience). Write will fail without crashing if we try to
869 + * read from an invalid pointer, so we can finally be pretty sure if the
870 + * pointed entry is an IndicatorObject
871 + * parent IndicatorObject */
872 +
873 + int fds[2];
874 +
875 + if (pipe (fds) > -1)
876 + {
877 + size_t data_size = sizeof (IndicatorObjec
878 +
879 + if (write (fds[1], entry, data_size) != data_size)
880 + {
881 + entry = NULL;
882 + }
883 + else
884 + {
885 + data_size = sizeof (IndicatorObject*);
886 +
887 + if (write (fds[1], entry->
888 + !INDICATOR_
889 + {
890 + entry = NULL;
891 + }
892 + }
893 +
894 + close (fds[0]);
895 + close (fds[1]);
896 + }
897 + }
Clever, but, why are we even checking for the validity of a pointer here ? Are we passing pointers around in dbus ?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Marco Trevisan (Treviño) (3v1n0) wrote : Posted in a previous version of this proposal | # |
> 862 + if (sscanf (entry_id, "%p", &entry) == 1)
> 863 + {
> 864 + /* Checking that entry is really an IndicatorObject
> [...]
>
> Clever, but, why are we even checking for the validity of a pointer here ? Are
> we passing pointers around in dbus ?
Yes.
That's what we've done so far. Actually they're just unique ID, but since they're pointers in fact we can just use these ids to quickly get the struct we're referring to, without using other data to store them that would increase the computation and the memory used (and considering the overhead that dbus already causes to us when scrubbing menus, the faster we are, the best it is). Since we can be more smart (and safe), using low-level things we use them.
Also, since the IndicatorObject
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Mikkel Kamstrup Erlandsen (kamstrup) wrote : Posted in a previous version of this proposal | # |
On 1 February 2012 17:40, Marco Trevisan (Treviño) <mail@3v1n0.net> wrote:
>> 862 + if (sscanf (entry_id, "%p", &entry) == 1)
>> 863 + {
>> 864 + /* Checking that entry is really an IndicatorObject
>> [...]
>>
>> Clever, but, why are we even checking for the validity of a pointer here ? Are
>> we passing pointers around in dbus ?
>
> Yes.
> That's what we've done so far. Actually they're just unique ID, but since they're pointers in fact we can just use these ids to quickly get the struct we're referring to, without using other data to store them that would increase the computation and the memory used (and considering the overhead that dbus already causes to us when scrubbing menus, the faster we are, the best it is). Since we can be more smart (and safe), using low-level things we use them.
>
> Also, since the IndicatorObject
There's no way to know that, and the pipe() trick does not ensure that
either. If the IndicatorObject
allocated (fx.) it'll still be valid memory even after freed, but
might contain garbage.
I am not entirely understanding the reasoning here. Is it that
creating a pipe and checking the write is cheaper than a hashtable
lookup? I doubt it as pipe(), write(), and close() requires a few mode
switches. And arguably I'd consider the hashtable trick more safe.
(although the pipe trick is neat, I am also wondering if there
wouldn't be some lower level call that checked this directly..? not
arguing in the favour of that though :-))
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Marco Trevisan (Treviño) (3v1n0) wrote : Posted in a previous version of this proposal | # |
First of all, using pipe() is just done for convenience (to quickly get a writable fd), you can do that also using open with any valid temporary file (but not /dev/null, since it's a special file).
> > Also, since the IndicatorObject
> this is the only way we have to check if we're really working with a valid
> one.
>
> There's no way to know that, and the pipe() trick does not ensure that
> either. If the IndicatorObject
> allocated (fx.) it'll still be valid memory even after freed, but
> might contain garbage.
Yes, but... The fact is: if the pointer we found is pointing to an area of memory where we have no rights, without using this trick we'd crash. Using this trick instead, if that pointer is not on a valid area, write won't be able to read from it and instead of crashing it will give an EFAULT error.
Now, if that area is readable, write won't give any error, but at that point INDICATOR_IS_OBJECT will be able to check the pointed object without crashing, since it's in a valid area...
I've made tests locally to see if this was working on a manually allocated area (with garbage too) or even checking "self", and all the results are with me, because if INDICATOR_IS_OBJECT (and so g_type_
> I am not entirely understanding the reasoning here. Is it that
> creating a pipe and checking the write is cheaper than a hashtable
> lookup? I doubt it as pipe(), write(), and close() requires a few mode
> switches. And arguably I'd consider the hashtable trick more safe.
Well, on this side I think that is hard to be cheaper than this... Especially on memory side.
Also, that method is not crash-free by design... Here we can be sure that no crash will happen.
> (although the pipe trick is neat, I am also wondering if there
> wouldn't be some lower level call that checked this directly..? not
> arguing in the favour of that though :-))
No... I guess we should improve glib! :)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Mikkel Kamstrup Erlandsen (kamstrup) wrote : Posted in a previous version of this proposal | # |
how often is get_indicator_
Also, you should be logging a critical if we're passed an invalid pointer. This means that there is a bug in Unity or someone playing tricks on us.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Marco Trevisan (Treviño) (3v1n0) wrote : Posted in a previous version of this proposal | # |
> how often is get_indicator_
> creating pipes all over the place...
Every time we get a signal from the unity panel to open a menu, to scroll or to middle-click... So basically on clicks over menus, scrubbing, scrolling or middle-clicking. Plus when the gometries of an entry changes (on added/removed, and when its position on the screen change).
So this mostly happen when there's user interaction.
As said, if you think it's better, we can also just create a temporary file when the service starts, open it and keeping its fd until we stop the panel service... Using that for our pointer-checking work.
Anyway I guess that generally our system really opens a lot of fd's when doing more high-level operations (for DBus for example, but also for more standard things), that we don't care about...
> Also, you should be logging a critical if we're passed an invalid pointer.
> This means that there is a bug in Unity or someone playing tricks on us.
Yes, I agree about that. It was already on my TODO list ;)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Thomi Richards (thomir-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
Hi,
+static IndicatorObject
1025 +get_indicator_
1026 +{
1027 + IndicatorObject
1028 + entry = NULL;
1029 +
1030 + if (sscanf (entry_id, "%p", &entry) == 1)
1031 + {
1032 + /* Checking that entry is really an IndicatorObject
1033 + * To do that, we use an hack that allows to check if the pointer we read is
1034 + * actually a valid pointer without crashing. This can be done using the
1035 + * low-level write function to read from the pointer to a valid fds (we use
1036 + * a pipe for convenience). Write will fail without crashing if we try to
1037 + * read from an invalid pointer, so we can finally be pretty sure if the
1038 + * pointed entry is an IndicatorObject
1039 + * parent IndicatorObject */
1040 +
1041 + int fds[2];
1042 +
1043 + if (pipe (fds) > -1)
1044 + {
1045 + size_t data_size = sizeof (IndicatorObjec
1046 +
1047 + if (write (fds[1], entry, data_size) != data_size)
1048 + {
1049 + entry = NULL;
1050 + }
1051 + else
1052 + {
1053 + if (g_slist_find (self->
1054 + {
1055 + if (!INDICATOR_
1056 + entry = NULL;
1057 + }
1058 + else
1059 + {
1060 + entry = NULL;
1061 + }
1062 + }
1063 +
1064 + close (fds[0]);
1065 + close (fds[1]);
1066 + }
1067 + else
1068 + {
1069 + entry = NULL;
1070 + }
1071 + }
1072 +
1073 + if (!entry)
1074 + {
1075 + g_warning("The entry id '%s' you're trying to parse is not a valid IndicatorObject
1076 + }
1077 +
1078 + return entry;
1079 }
1080
I'm sorry, please don't land this code. There are several things wrong with it:
1) You're using pointers as a unique ID. That's fine - since pointers to unique objects are a good way to generate a unique id. Doing ptr -> int is fine, doing int -> ptr is not. Besides, you're now assuming that the source of your unique IDs is never going to change; if for some reason it does change, you now need to update this code as well.
2) The code needs a big comment to explain what it does. That's usually a prettyy good clue that the code needs to be made more obvious.
3) As Mikkel already pointed out, you're *assuming* that this hack has better performance, which may not be the case, since IO will involve several kernel-mode switches. At the very least, if you *really* need the performance, you should have done some profiling of the entire codebase, figured out which bits needed improving, and fixed those. Even then, I'd like to see a comparative speed test between this code and a hash table lookup (which should, of course, be constant-time and pretty darn fast for any sensible hash-table implementation).
4) As Mikkel also already pointed out, this method may yield false positives, so it doesn't even work as advertised.
I'd suggest that the correct method here is to read the id, check if it's in your hash table. If it is, do a table lookup to retrieve the value. Simple!
Cheers
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Marco Trevisan (Treviño) (3v1n0) wrote : Posted in a previous version of this proposal | # |
Ok, I've removed the "evil" hack.
Even if I think that it was safe, I must say that it was slower.
I've made some tests, and you're right.
I've tried to put it in because I liked the idea :P
The panel is now safe again.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Thomi Richards (thomir-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
> I've made some tests, and you're right.
> I've tried to put it in because I liked the idea :P
That's always tempting ;)
I've spotted a few small things:
1) Please rename either the 'panel_
2) Another naming thing: PanelIndicatorE
IsProxyVisible
IsRemoteVisible
etc.
Otherwise, the code looks good, although the panel service code makes we want to stab myself in the eye.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Marco Trevisan (Treviño) (3v1n0) wrote : Posted in a previous version of this proposal | # |
> 1) Please rename either the 'panel_
> 'panel_
> does not show the entry then the function needs to be renamed.
The fact is that panel_service_
I'll see if I can find a better name, but it makes sense to me anyway.
> 2) Another naming thing: PanelIndicatorE
> ince it doesn't check whether the PanelIndivatorE
> it check's whether the remote object is visible or not. Some alternative names
> might be:
>
> IsProxyVisible
> IsRemoteVisible
No, I don't want to do that, because the View is actually a kind of wrapper of the remote entry, when the remote entry is not visible, neither the view will be and back. Also all the current methods of that class refers to the view, even if they're currently using the values of the remote value. The fact that we have a remote value is a private thing, and we shouldn't care about that.
For the record, I've put this on WIP again because we need to include a new parameter to the EntryActivate signal to include the geometry of that entry.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Andrea Azzarone (azzar1) wrote : Posted in a previous version of this proposal | # |
44 +const std::string SETTING_
45 +const std::string SET
TING_KEY(
Maybe they shuld be in a unnamed namespace?
---
48 + : Indicator(name),
49 + gsettings_
50 + integrated_(false)
For the next time... :
48 + : Indicator(name)
49 + , gsettings_
50 + , integrated_(false)
---
setting_
53 + CheckSettingVal
54 + });
If you're not using the argument gchar* key, please use an unnamed variable too...
---
g_idle_add_full (G_PRIORITY_
g_variant_get (parameters, "(s(iiuu))", &entry_name, &geo.x, &geo.y, &geo.width, &geo.height);
Remove the whitespace between the function name and the bracket.
---
AppmenuIndicator *appmenu = dynamic_
:(
---
751 - * Copyright (C) 2010 Canonical Ltd
752 + * Copyright (C) 2012 Canonical Ltd
Please do 2010-2012
---
760 +#include <NuxCore/Rect.h>
I think you can use a forward declaration.
---
+ bool IsVisible();
Const?
---+ bool IsVisible();
Btw all of these are not blocking ;)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Marco Trevisan (Treviño) (3v1n0) wrote : Posted in a previous version of this proposal | # |
> 44 +const std::string SETTING_
> 45 +const std::string SET
> TING_KEY(
>
> Maybe they shuld be in a unnamed namespace?
Right. I thought I had used.
> 48 + : Indicator(name),
> 49 + gsettings_
> 50 + integrated_(false)
>
> For the next time... :
>
> 48 + : Indicator(name)
> 49 + , gsettings_
> 50 + , integrated_(false)
Well, I don't like this syntax with prefixed commas... But if you all prefer that...
> setting_
> gchar* key) {
> 53 + CheckSettingVal
> 54 + });
>
> If you're not using the argument gchar* key, please use an unnamed variable
> too...
Right, I forgot to remove it (at the beginning I used the key).
> g_idle_add_full (G_PRIORITY_
> NULL);
> g_variant_get (parameters, "(s(iiuu))", &entry_name, &geo.x, &geo.y,
> &geo.width, &geo.height);
>
> Remove the whitespace between the function name and the bracket.
Damned you! :)
> AppmenuIndicator *appmenu = dynamic_
>
> :(
We don't have any way. But it's totally safe: we do two checks, first IsAppmenu(), then this one.
> 751 - * Copyright (C) 2010 Canonical Ltd
> 752 + * Copyright (C) 2012 Canonical Ltd
>
> Please do 2010-2012
Sure.
> ---
>
> 760 +#include <NuxCore/Rect.h>
>
> I think you can use a forward declaration.
I thought too... But it didn't work when I firstly compiled. Now I removed it again and it works... Magic!
> ---
>
> + bool IsVisible();
>
> Const?
>
> ---+ bool IsVisible();
That is done in lp:~3v1n0/unity/lim-panel... Here just focus on the UnityCore unity-panel-service changes.
> Btw all of these are not blocking ;)
If they are not blocking, please don't set your review as needs fixing! :)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Andrea Azzarone (azzar1) : Posted in a previous version of this proposal | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michal Hruby (mhr3) wrote : Posted in a previous version of this proposal | # |
Just one tiny thing:
1035 + " <arg type='(iiuu)' name='entry_id' />"
Doesn't look like id, could you update the name? Other than that it looks fine now.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Marco Trevisan (Treviño) (3v1n0) wrote : Posted in a previous version of this proposal | # |
> Just one tiny thing:
>
> 1035 + " <arg type='(iiuu)' name='entry_id' />"
>
> Doesn't look like id, could you update the name? Other than that it looks fine
> now.
Indeed! Copy&Paste issue :)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Thomi Richards (thomir-deactivatedaccount) : Posted in a previous version of this proposal | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Unity Merger (unity-merger) wrote : Posted in a previous version of this proposal | # |
The Jenkins job https:/
Not merging it.
I've just fixed few small issues on the code, and tested it on the real world. The core part works as expected, so I think it can be landed if we need it in trunk for unity-2d.
More tests will arrive soon by the way.