Unity

Merge lp:~rodrigo-moya/unity/a11y-global-listeners into lp:unity

  1. a11y-global-listeners
  2. Merge into trunk
Proposed by Rodrigo Moya
Status: Merged
Approved by: Rodrigo Moya
Approved revision: no longer in the source branch.
Merged at revision: 769
Proposed branch: lp:~rodrigo-moya/unity/a11y-global-listeners
Merge into: lp:unity
Diff against target: 172 lines (+94/-25)
1 file modified
src/unity-util-accessible.cpp (+94/-25)
To merge this branch: bzr merge lp:~rodrigo-moya/unity/a11y-global-listeners
Reviewer Review Type Date Requested Status
Alejandro Piñeiro (community) Approve
Alex Launi (community) Approve
Review via email: [email protected]

Description of the change

Implement global listeners for UnityUtilAccessible class

To post a comment you must log in.
Revision history for this message
Alex Launi (alexlauni) wrote :

8 - covering_part = (float)(window->serverWidth () * window->serverHeight ()) / (float)(screen_width * screen_height)
9 + covering_part = (float)(window->serverWidth () * window->serverHeight ()) / (float)(screen_width * screen_height);

Can you remove this, and push a new rev before this gets merged? We've fixed this already, and this will just cause a conflict when merging your branch.

69 +static guint
70 +add_listener (GSignalEmissionHook listener,
71 + const gchar *object_type,
72 + const gchar *signal_name,
73 + const gchar *hook_data)
74 +{

Please align these.

Looks good, what do I need to do to test that this is actually working?

review: Approve
Revision history for this message
Alejandro Piñeiro (apinheiro) wrote :

118 + /* FIXME: need to specifically process window: events (create, destroy,
119 + minimize, maximize, restore, activate, deactivate) */
120 +
121 + split_string = g_strsplit (event_type, ":", 3);
122 + if (split_string)
123 + {
124 + rc = add_listener (listener, split_string[1], split_string[2], event_type);
125 +
126 + g_strfreev (split_string);
127 + }

Take into account that although the window event registration is not supported, the bridge will still try to connect to them. So with this code:
  * For a normal atk event the event_type would be something like: "Gtk:AtkDocument:load-complete"
    So the add_listener would be something like: add_listener (listener, "AtkDocument", "load-complete", event_type)
    And it is right

  * For a window event, it would be something like: "window:create"
    And the add_listener would be something like add_listener (listener, "create", NULL (or not), event_type)

    What it is wrong, as, for example, the second parameter is used to find a type by name, as there isn¡t any type with this name.

Testing it, I found this error messages:

** (<unknown>:14134): WARNING **: Invalid object type deactivate
(<unknown>:14134): GLib-CRITICAL **: g_hash_table_insert_internal: assertion `hash_table != NULL' failed

So, my advice is go back to the cally-util implementation used as base, and use the "window" if. And do nothing in this case, moving the warning message that you added here (as I have this warning about some methods not supported on CallyUtil)

review: Needs Fixing
Revision history for this message
Alejandro Piñeiro (apinheiro) wrote :

>
> Looks good, what do I need to do to test that this is actually working?

Well, as I said, I tested it and there are some things that fail.

Anyway, I also want to add that although we can test if the listeners are installed without problems, take into account that the purpose is install global listeners, and for the moment the current ATK support doesn't emit any event yet, so we can't test if the listeners are properly executed.

Revision history for this message
Rodrigo Moya (rodrigo-moya) wrote :

> 8 - covering_part = (float)(window->serverWidth () *
> window->serverHeight ()) / (float)(screen_width * screen_height)
> 9 + covering_part = (float)(window->serverWidth () *
> window->serverHeight ()) / (float)(screen_width * screen_height);
>
> Can you remove this, and push a new rev before this gets merged? We've fixed
> this already, and this will just cause a conflict when merging your branch.
>
done

> 69 +static guint
> 70 +add_listener (GSignalEmissionHook listener,
> 71 + const gchar *object_type,
> 72 + const gchar *signal_name,
> 73 + const gchar *hook_data)
> 74 +{
>
> Please align these.
>
they are aligned, it's just LP not showing it correctly AFAICS

Revision history for this message
Rodrigo Moya (rodrigo-moya) wrote :

> 118 + /* FIXME: need to specifically process window: events (create,
> destroy,
> 119 + minimize, maximize, restore, activate, deactivate) */
> 120 +
> 121 + split_string = g_strsplit (event_type, ":", 3);
> 122 + if (split_string)
> 123 + {
> 124 + rc = add_listener (listener, split_string[1], split_string[2],
> event_type);
> 125 +
> 126 + g_strfreev (split_string);
> 127 + }
>
> Take into account that although the window event registration is not
> supported, the bridge will still try to connect to them. So with this code:
> * For a normal atk event the event_type would be something like:
> "Gtk:AtkDocument:load-complete"
> So the add_listener would be something like: add_listener (listener,
> "AtkDocument", "load-complete", event_type)
> And it is right
>
> * For a window event, it would be something like: "window:create"
> And the add_listener would be something like add_listener (listener,
> "create", NULL (or not), event_type)
>
> What it is wrong, as, for example, the second parameter is used to find a
> type by name, as there isn¡t any type with this name.
>
> Testing it, I found this error messages:
>
> ** (<unknown>:14134): WARNING **: Invalid object type deactivate
> (<unknown>:14134): GLib-CRITICAL **: g_hash_table_insert_internal: assertion
> `hash_table != NULL' failed
>
> So, my advice is go back to the cally-util implementation used as base, and
> use the "window" if. And do nothing in this case, moving the warning message
> that you added here (as I have this warning about some methods not supported
> on CallyUtil)
>
ok, done

Revision history for this message
Alejandro Piñeiro (apinheiro) wrote :

109 + if (g_str_equal ("window", split_string[0]))
110 + {
111 + /* FIXME: need to specifically process window: events (create, destroy,
112 + minimize, maximize, restore, activate, deactivate) */
113 + rc = add_listener (listener, split_string[0], split_string[1], event_type);
114 + }

This is wrong. See what I said on my previous comment:

> So, my advice is go back to the cally-util implementation used as base, and
> use the "window" if. And do nothing in this case, moving the warning message
> that you added here (as I have this warning about some methods not supported
> on CallyUtil)

So as the window event are not emitted at all, you shouldn't add a listener to this events. And split_string[0] will be just "window", so it will fail executing g_type_from_name ("window") on add_listener code.

That add_listener line should be removed.

I tested this branch with that line removed, and those nasty messages are not present, so the add listener code seems to work.

In order to test that the listeners are working we need some AtkObjects emitting signals. But in the same way we need the listeners to check that the signals are emitted properly. So, after correct this little error, IMHO, the branch should be merged, and then test it once some ATK events got emitted.

review: Needs Fixing
Revision history for this message
Alejandro Piñeiro (apinheiro) wrote :

Ok, I have tested it after the last revision. Set to approved.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/unity-util-accessible.cpp'
2--- src/unity-util-accessible.cpp 2011-01-13 01:59:18 +0000
3+++ src/unity-util-accessible.cpp 2011-01-17 17:25:53 +0000
4@@ -14,6 +14,7 @@
5 * along with this program. If not, see <http://www.gnu.org/licenses/>.
6 *
7 * Authored by: Alejandro Piñeiro Iglesias <[email protected]>
8+ * Rodrigo Moya <[email protected]>
9 */
10
11 #include <stdlib.h>
12@@ -30,14 +31,17 @@
13 static guint unity_util_accessible_add_global_event_listener (GSignalEmissionHook listener,
14 const gchar* event_type);
15 static void unity_util_accessible_remove_global_event_listener (guint remove_listener);
16-static guint unity_util_accessible_add_key_event_listener (AtkKeySnoopFunc listener,
17- gpointer data);
18-static void unity_util_accessible_remove_key_event_listener (guint remove_listener);
19 static AtkObject * unity_util_accessible_get_root (void);
20 static const gchar * unity_util_accessible_get_toolkit_name (void);
21 static const gchar * unity_util_accessible_get_toolkit_version (void);
22
23+typedef struct {
24+ guint idx;
25+ gulong hook_id;
26+ guint signal_id;
27+} UnityUtilListenerInfo;
28
29+static GHashTable *listener_list = NULL;
30 static AtkObject* root = NULL;
31
32 G_DEFINE_TYPE (UnityUtilAccessible, unity_util_accessible, ATK_TYPE_UTIL);
33@@ -53,8 +57,6 @@
34
35 atk_class->add_global_event_listener = unity_util_accessible_add_global_event_listener;
36 atk_class->remove_global_event_listener = unity_util_accessible_remove_global_event_listener;
37- atk_class->add_key_event_listener = unity_util_accessible_add_key_event_listener;
38- atk_class->remove_key_event_listener = unity_util_accessible_remove_key_event_listener;
39 atk_class->get_root = unity_util_accessible_get_root;
40 atk_class->get_toolkit_name = unity_util_accessible_get_toolkit_name;
41 atk_class->get_toolkit_version = unity_util_accessible_get_toolkit_version;
42@@ -63,7 +65,6 @@
43 static void
44 unity_util_accessible_init (UnityUtilAccessible *unity_util_accessible)
45 {
46- /* instance init: usually not required */
47 }
48
49 static AtkObject*
50@@ -92,35 +93,103 @@
51 return "0.1";
52 }
53
54+static guint
55+add_listener (GSignalEmissionHook listener,
56+ const gchar *object_type,
57+ const gchar *signal_name,
58+ const gchar *hook_data)
59+{
60+ GType type;
61+ guint signal_id;
62+ guint rc = 0;
63+ static guint listener_idx = 1;
64+
65+ if (!listener_list)
66+ listener_list = g_hash_table_new_full (g_int_hash, g_int_equal, NULL, g_free);
67+
68+ type = g_type_from_name (object_type);
69+ if (type)
70+ {
71+ signal_id = g_signal_lookup (signal_name, type);
72+ if (signal_id > 0)
73+ {
74+ UnityUtilListenerInfo *listener_info;
75+
76+ rc = listener_idx;
77+ listener_info = g_new0 (UnityUtilListenerInfo, 1);
78+ listener_info->idx = listener_idx;
79+ listener_info->hook_id = g_signal_add_emission_hook (signal_id, 0, listener,
80+ g_strdup (hook_data),
81+ (GDestroyNotify) g_free);
82+ listener_info->signal_id = signal_id;
83+
84+ g_hash_table_insert (listener_list, &(listener_info->idx), listener_info);
85+
86+ listener_idx++;
87+ }
88+ else
89+ g_debug ("Signal type %s not supported\n", signal_name);
90+ }
91+ else
92+ g_warning ("Invalid object type %s\n", object_type);
93+
94+ return rc;
95+}
96
97 static guint
98 unity_util_accessible_add_global_event_listener (GSignalEmissionHook listener,
99 const gchar* event_type)
100 {
101- /* FIXME: implement me!! */
102- return 0;
103+ gchar **split_string;
104+ guint rc = 0;
105+
106+ split_string = g_strsplit (event_type, ":", 3);
107+ if (split_string)
108+ {
109+ if (g_str_equal ("window", split_string[0]))
110+ {
111+ /* FIXME: need to specifically process window: events (create, destroy,
112+ minimize, maximize, restore, activate, deactivate) */
113+ }
114+ else
115+ {
116+ rc = add_listener (listener, split_string[1], split_string[2], event_type);
117+ }
118+
119+ g_strfreev (split_string);
120+ }
121+
122+ return rc;
123 }
124
125 static void
126 unity_util_accessible_remove_global_event_listener (guint remove_listener)
127 {
128- /* FIXME: implement me!! */
129-}
130-
131-static guint
132-unity_util_accessible_add_key_event_listener (AtkKeySnoopFunc listener,
133- gpointer data)
134-{
135- /* FIXME: implement me!! */
136- return 0;
137-}
138-
139-static void
140-unity_util_accessible_remove_key_event_listener (guint remove_listener)
141-{
142- /* FIXME: implement me!! */
143-}
144-
145+ if (remove_listener > 0)
146+ {
147+ UnityUtilListenerInfo *listener_info;
148+
149+ listener_info = (UnityUtilListenerInfo *) g_hash_table_lookup (listener_list, &remove_listener);
150+ if (listener_info != NULL)
151+ {
152+ if (listener_info->hook_id != 0 && listener_info->signal_id != 0)
153+ {
154+ g_signal_remove_emission_hook (listener_info->signal_id,
155+ listener_info->hook_id);
156+ g_hash_table_remove (listener_list, &remove_listener);
157+ }
158+ else
159+ {
160+ g_warning ("Invalid listener hook_id %ld or signal_id %d",
161+ listener_info->hook_id, listener_info->signal_id);
162+ }
163+ }
164+ else
165+ g_warning ("No listener with the specified ID: %d", remove_listener);
166+ }
167+ else
168+ g_warning ("Invalid listener_id: %d", remove_listener);
169+}
170
171 void
172 unity_util_accessible_add_window (nux::BaseWindow *window)