Implement APIs for another non-Python panel.

Implement APIs for another non-Python panel.

1. Support icon and prop_list = null in ibus_property_new with GIR.
2. Add getter methods in IBusText and IBusProperty since GJS cannot access
   the members in C-Structure.
3. Add ibus_get_language_name() since GIR libxml2 does not provide the
   useful APIs.
4. Implement flags in ibus_bus_request_name() to follow DBus
   RequestName signal spec.
   http://dbus.freedesktop.org/doc/dbus-specification.html#message-bus-names
   This is needed to terminate the current IBus panel.
   E.g. IBus GTK panel is launched by ibus-daemon but another panel is
   launched by gnome-shell.
5. Support IBUS_BUS_NAME_FLAG_ALLOW_REPLACEMENT in ui/gtk/main.py
6. Fix bus_component_set_factory() not to call
   bus_component_factory_destroy_cb() twice.
7. Hide ibus_text_new_from_static_string() for GIR.
8. Add ibus_is_running_gnome_shell() for ibus-ui-gtk because
   gnome-shell runs earlier than ibus-ui-gtk.

[fujiwara, 2011-03-11 08:34:41]

Now I noticed I have to use ibus/ibus.git location for this review.
Thanks for the update:
https://github.com/phuang/ibus/commit/3ea8ba748c3699b5928b2a7932165ff25be205d2
I'm not sure if I can rebase this patch again with this review id.

Copied from the review #4216048.
On 2011/02/24 18:34:30, Peng wrote:
> I did not find the code for handling some connection releases a bus name. I
> think when a connectin releases a bus name, daemon should check the waiting
> queue, and assign the bus name to a waiting connection.

OK, I updated the destroy() function.

> http://codereview.appspot.com/4216048/diff/1/bus/dbusimpl.c#newcode255
> bus/dbusimpl.c:255: owner->conn = connection;
> No need add ref to keep conn alive?

You're right.

> http://codereview.appspot.com/4216048/diff/1/bus/dbusimpl.c#newcode326
> bus/dbusimpl.c:326: BusDBusImpl           *dbus)
> Only one space between the longest type name and *variable.

As I talked off line, GNOME uses spaces converted by a tab and this way can show
the diff clearly in case the function arguments are added or removed.
But we agreed with your coding.

> http://codereview.appspot.com/4216048/diff/1/bus/dbusimpl.c#newcode404
> bus/dbusimpl.c:404: return FALSE;
> Return TRUE? Name does not have owner, the name should be allowed replace.
> right?

You're right.

> http://codereview.appspot.com/4216048/diff/1/bus/dbusimpl.c#newcode797
> bus/dbusimpl.c:797: gchar *oldname = NULL;
> old_owner_name is more readable, or just remove the variable.
> I think the var is not necessary. You could use bus_connection_get_unique_name
> (old_owner_conn) directly.

OK renamed the variable.
Unfortunately the owner is cleaned and the backup old_owner_name is needed.

> http://codereview.appspot.com/4216048/diff/1/bus/dbusimpl.c#newcode840
> bus/dbusimpl.c:840: goto out;
> Please remove goto instead you could use:
> 
> do {
>    ...
>    if (A) {
>        break;
>    }
>    if (b) {
>        break;
>    }
>    ...
> } while (0);
> 
> // out:

As we talked, the goto is not bad in such a case in GNOME not to have another
indent spaces.
Also the code was originally copied from DBus.
Probably the current code is ok for you.

> http://codereview.appspot.com/4216048/diff/1/bus/dbusimpl.c#newcode875
> bus/dbusimpl.c:875: (gpointer) g_strdup (bus_connection_add_name (connection,
> name)),
> Why not use service->name? It may avoid allocate extra string.

I think we need to call bus_connection_add_name() there.
If you mean to separate the bus_connection_add_name() and g_strdup() from the
one line, it's also ok with me.

> http://codereview.appspot.com/4216048/diff/1/bus/dbusimpl.c#newcode996
> bus/dbusimpl.c:996: /* broadcast the message to clients that listen to the
> signal. */
> From the dbus spec, the signal is onlt sent to the application that lost the
> name.

You're right.
I added one argument of the BusConnection.

> http://codereview.appspot.com/4216048/diff/1/src/ibustext.h#newcode173
> src/ibustext.h:173: * ibus_text_get_is_static:
> I think script language should not use IBusText with static == TRUE. Could you
> please find a way to forbidden export static related APIs to script languages?
> 
> Myabe add some marks in comment?

OK, I added (skip) for ibus_text_new_from_static_string().

> http://codereview.appspot.com/4216048/diff/1/src/ibustypes.h#newcode158
> src/ibustypes.h:158: IBUS_BUS_NAME_FLAG_DO_NOT_QUEUE        = 4,
> I prefer to use 1, (1 << 1), (1 << 2), (1 << 3)

You're right.

> http://codereview.appspot.com/4216048/diff/1/src/ibustypes.h#newcode176
> src/ibustypes.h:176: IBUS_BUS_REQUEST_NAME_REPLY_ALREADY_OWNER   = 4,
> same

Probably the shift won't work in this case.

On 2011/03/02 23:25:13, Peng Huang wrote:
> http://codereview.appspot.com/4216048/diff/8001/bus/dbusimpl.c#newcode464
> bus/dbusimpl.c:464: G_TYPE_OBJECT,
> Should use BUS_TYPE_CONNECTION? Please check it.

You're right.

I added add_signal_receiver() for DBus and I could receive the NameLost signal.
https://github.com/fujiwarat/ibus/blob/gjs/ui/gtk/main.py#L59

I added signal_subscribe() for GDBus but it seems the NameLost signal is not
sent.
https://github.com/fujiwarat/ibus/blob/gjs/ui/gjs-g-s/indicator.js#L62
https://github.com/fujiwarat/ibus/blob/gjs/ui/gjs/indicator.js#L65

g-s maintainer recommends to update ibus code.
https://bugzilla.gnome.org/show_bug.cgi?id=641531#c15
> > > - The correct way to instantiate objects is "new Namespace.Class({ param:
> > > value, param2: value2 })". Use the "Namespace.Class.new" syntax only when
> > > absolutely needed (should be never in GTK+3).
> > 
> > OK, I changed the GTK part. IBus needs the format since some classes doesn't
> > use the properties.
> 
> I'd say "fix them on the IBus part", but then it's ok like that.

E.g. new IBus.PanelService.new(connection) calls
ibus_panel_service_new(connection)
But the maintainer recommends to call new IBus.PanelService({connection:
connection})
Unfortunately it doesn't work currently because ibus_panel_service_new() has the
hard-coded object-path = IBUS_PATH_PANEL.
IBus.PanelService({connection: connection}) means not to call
ibus_panel_service_new() but call IBusPanelServiceClass->set_property().
So if we don't use the *_new() function, we need to patch the *_set_property()
function.

I guess this fix is not required but nice to have.

[Peng, 2011-03-11 15:19:20]

http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c
File bus/dbusimpl.c (right):

http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode200
bus/dbusimpl.c:200: "    <method name='ListQueuedOwners'>"
It is better to implement it as well.

http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode255
bus/dbusimpl.c:255: g_assert (connection != NULL);
g_assert(BUS_IS_CONNECTION(connection)) ?

http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode257
bus/dbusimpl.c:257: owner = (BusConnectionOwner *) g_new0 (BusConnectionOwner,
1);
How about use g_slice_new() and g_slice_free()

http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode300
bus/dbusimpl.c:300: service = g_new0 (BusNameService, 1);
g_slice_new() and g_slice_free()?

http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode338
bus/dbusimpl.c:338: if (dbus) {
I think dbus should not be NULL. right?
So check it is not necessary?
maybe just need g_assert().

http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode368
bus/dbusimpl.c:368: if (dbus && service->owners == NULL) {
Same with Line 338?

http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode372
bus/dbusimpl.c:372: g_object_ref (owner->conn),
Need g_object_ref()? Please double check it.

http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode394
bus/dbusimpl.c:394: if (dbus &&
Same with Line 338

http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode399
bus/dbusimpl.c:399: g_object_ref (owner->conn),
need g_obejct_ref()? Could you double check it.

http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode1002
bus/dbusimpl.c:1002: static guint32 serial = 0;
Could you change serial to object member, and make all functions share a same
serial?

http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode1372
bus/dbusimpl.c:1372: "");
Is new owner always ""? If there is a connection are waiting the name, it will
be the new owner.

http://codereview.appspot.com/4279042/diff/1/ibus/common.py
File ibus/common.py (right):

http://codereview.appspot.com/4279042/diff/1/ibus/common.py#newcode56
ibus/common.py:56: "is_running_gnome_shell",
Do we need this?

http://codereview.appspot.com/4279042/diff/1/src/ibustext.h
File src/ibustext.h (right):

http://codereview.appspot.com/4279042/diff/1/src/ibustext.h#newcode179
src/ibustext.h:179: gboolean         ibus_text_get_is_static           
(IBusText       *text);
js should does not need this function. Please skip it too.

http://codereview.appspot.com/4279042/diff/1/src/ibusutil.c
File src/ibusutil.c (right):

http://codereview.appspot.com/4279042/diff/1/src/ibusutil.c#newcode100
src/ibusutil.c:100: filename = g_build_filename ("/usr",
"share/xml/iso-codes/iso_639.xml", NULL);
It is better to not hard code "/usr"?

http://codereview.appspot.com/4279042/diff/1/src/ibusutil.c#newcode137
src/ibusutil.c:137: ibus_is_running_gnome_shell (void)
Is it necessary?

http://codereview.appspot.com/4279042/diff/1/ui/gtk/main.py
File ui/gtk/main.py (right):

http://codereview.appspot.com/4279042/diff/1/ui/gtk/main.py#newcode76
ui/gtk/main.py:76: print "Got NameLost signal", name
Maybe it is not good to quit when lost the name ownership.
Instead we could hide ibus status icon, and show it again when it get the
ownership back.

And if the behavior is configurable, it will be much better.

http://codereview.appspot.com/4279042/diff/1/ui/gtk/main.py#newcode97
ui/gtk/main.py:97: stop_by_shell = False
I think it is not good to has such argument. It is for gnome-shell only.

I suggest to add something like "not replace the current ui".

If it is true, it will check if there is a ibus ui, if it is, it will not
replace it.

[fujiwara, 2011-04-01 22:20:32]

Sorry for the late response.
I had thought what is better for ListQueuedOwner.

On 2011/03/11 15:19:20, Peng wrote:
> http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode1002
> bus/dbusimpl.c:1002: static guint32 serial = 0;
> Could you change serial to object member, and make all functions share a same
> serial?

Talking off line, I understood s/to/into/ .

> http://codereview.appspot.com/4279042/diff/1/ui/gtk/main.py#newcode97
> ui/gtk/main.py:97: stop_by_shell = False
> I think it is not good to has such argument. It is for gnome-shell only.
> 
> I suggest to add something like "not replace the current ui".
> 
> If it is true, it will check if there is a ibus ui, if it is, it will not
> replace it.

I don't understand what you mean here.

[Peng, 2011-04-02 15:13:31]

On 2011/04/01 22:20:32, fujiwara wrote:
> Sorry for the late response.
> I had thought what is better for ListQueuedOwner.
> 
> On 2011/03/11 15:19:20, Peng wrote:
> > http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode1002
> > bus/dbusimpl.c:1002: static guint32 serial = 0;
> > Could you change serial to object member, and make all functions share a
same
> > serial?
> 
> Talking off line, I understood s/to/into/ .
> 
> > http://codereview.appspot.com/4279042/diff/1/ui/gtk/main.py#newcode97
> > ui/gtk/main.py:97: stop_by_shell = False
> > I think it is not good to has such argument. It is for gnome-shell only.
> > 
> > I suggest to add something like "not replace the current ui".
> > 
> > If it is true, it will check if there is a ibus ui, if it is, it will not
> > replace it.
> 
> I don't understand what you mean here.

I think it is not a good idea to add some code for gnome-shell only. We need to
add some common way for resolving problems.

Could you use ibus-daemon --panel=disable to disable panel in gnome 3.0 desktop
session?

[fujiwara, 2011-04-04 06:51:12]

On 2011/04/02 15:13:31, Peng wrote:
> > > http://codereview.appspot.com/4279042/diff/1/ui/gtk/main.py#newcode97
> > > ui/gtk/main.py:97: stop_by_shell = False
> > > I think it is not good to has such argument. It is for gnome-shell only.
> > > 
> > > I suggest to add something like "not replace the current ui".
> > > 
> > > If it is true, it will check if there is a ibus ui, if it is, it will not
> > > replace it.
> > 
> > I don't understand what you mean here.
> 
> I think it is not a good idea to add some code for gnome-shell only. We need
to
> add some common way for resolving problems.
> 
> Could you use ibus-daemon --panel=disable to disable panel in gnome 3.0
desktop
> session?

OK, I understood you mean --panel=disable but I don't mean gtk-ui-panel is
always disabled.

gnome-session checks if the graphics supports GLX.
http://git.gnome.org/browse/gnome-session/tree/tools/gnome-session-check-acce...
If yes, it enables gnome-shell. If no (e.g. VM), it enables gnome-panel.

So currently my suggestion is 'shell' as the argument but not 'gnome-shell' as
you may not like it.

[Peng, 2011-04-04 13:14:43]

On 2011/04/04 06:51:12, fujiwara wrote:
> On 2011/04/02 15:13:31, Peng wrote:
> > > > http://codereview.appspot.com/4279042/diff/1/ui/gtk/main.py#newcode97
> > > > ui/gtk/main.py:97: stop_by_shell = False
> > > > I think it is not good to has such argument. It is for gnome-shell only.
> > > > 
> > > > I suggest to add something like "not replace the current ui".
> > > > 
> > > > If it is true, it will check if there is a ibus ui, if it is, it will
not
> > > > replace it.
> > > 
> > > I don't understand what you mean here.
> > 
> > I think it is not a good idea to add some code for gnome-shell only. We need
> to
> > add some common way for resolving problems.
> > 
> > Could you use ibus-daemon --panel=disable to disable panel in gnome 3.0
> desktop
> > session?
> 
> OK, I understood you mean --panel=disable but I don't mean gtk-ui-panel is
> always disabled.
> 
> gnome-session checks if the graphics supports GLX.
>
http://git.gnome.org/browse/gnome-session/tree/tools/gnome-session-check-acce...
> If yes, it enables gnome-shell. If no (e.g. VM), it enables gnome-panel.
> 
> So currently my suggestion is 'shell' as the argument but not 'gnome-shell' as
> you may not like it.

--panel=disable is for this kind use case. Could you find a way to test
gnome-shell in X init script? To decide if enable ibus default panel?

From another hand, I think the ibus-daemon already supports replacing bus name.
so it does not have any problem to start ibus default ui in gnome-shell desktop
session. gnome-shell could replace it later. Right?

[fujiwara, 2011-04-06 09:46:48]

On 2011/03/11 15:19:20, Peng wrote:
> http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c
> File bus/dbusimpl.c (right):
> 
> http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode200
> bus/dbusimpl.c:200: "    <method name='ListQueuedOwners'>"
> It is better to implement it as well.

OK, done.

> 
> http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode255
> bus/dbusimpl.c:255: g_assert (connection != NULL);
> g_assert(BUS_IS_CONNECTION(connection)) ?

Done.

> 
> http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode257
> bus/dbusimpl.c:257: owner = (BusConnectionOwner *) g_new0 (BusConnectionOwner,
> 1);
> How about use g_slice_new() and g_slice_free()

Done.

> 
> http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode300
> bus/dbusimpl.c:300: service = g_new0 (BusNameService, 1);
> g_slice_new() and g_slice_free()?

Done.

> 
> http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode338
> bus/dbusimpl.c:338: if (dbus) {
> I think dbus should not be NULL. right?
> So check it is not necessary?
> maybe just need g_assert().

Done.

> 
> http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode368
> bus/dbusimpl.c:368: if (dbus && service->owners == NULL) {
> Same with Line 338?

Done.

> 
> http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode372
> bus/dbusimpl.c:372: g_object_ref (owner->conn),
> Need g_object_ref()? Please double check it.

Done but I'll test this later as we talked off line.

> 
> http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode394
> bus/dbusimpl.c:394: if (dbus &&
> Same with Line 338

Done.

> 
> http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode399
> bus/dbusimpl.c:399: g_object_ref (owner->conn),
> need g_obejct_ref()? Could you double check it.

Done and same above.

> 
> http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode1002
> bus/dbusimpl.c:1002: static guint32 serial = 0;
> Could you change serial to object member, and make all functions share a same
> serial?

OK, I understood what you meant.
My understanding is, the signal is sent by connection but not dbusimpl so I
added the serial in the connection.

> 
> http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode1372
> bus/dbusimpl.c:1372: "");
> Is new owner always ""? If there is a connection are waiting the name, it will
> be the new owner.

Done.

> 
> http://codereview.appspot.com/4279042/diff/1/ibus/common.py
> File ibus/common.py (right):
> 
> http://codereview.appspot.com/4279042/diff/1/ibus/common.py#newcode56
> ibus/common.py:56: "is_running_gnome_shell",
> Do we need this?

OK, I'll be back again after we discuss NameLost/NameAcquired.

> 
> http://codereview.appspot.com/4279042/diff/1/src/ibustext.h
> File src/ibustext.h (right):
> 
> http://codereview.appspot.com/4279042/diff/1/src/ibustext.h#newcode179
> src/ibustext.h:179: gboolean         ibus_text_get_is_static           
> (IBusText       *text);
> js should does not need this function. Please skip it too.

Done.

> 
> http://codereview.appspot.com/4279042/diff/1/src/ibusutil.c
> File src/ibusutil.c (right):
> 
> http://codereview.appspot.com/4279042/diff/1/src/ibusutil.c#newcode100
> src/ibusutil.c:100: filename = g_build_filename ("/usr",
> "share/xml/iso-codes/iso_639.xml", NULL);
> It is better to not hard code "/usr"?

Done.

> 
> http://codereview.appspot.com/4279042/diff/1/src/ibusutil.c#newcode137
> src/ibusutil.c:137: ibus_is_running_gnome_shell (void)
> Is it necessary?

Same condition above.

> 
> http://codereview.appspot.com/4279042/diff/1/ui/gtk/main.py
> File ui/gtk/main.py (right):
> 
> http://codereview.appspot.com/4279042/diff/1/ui/gtk/main.py#newcode76
> ui/gtk/main.py:76: print "Got NameLost signal", name
> Maybe it is not good to quit when lost the name ownership.
> Instead we could hide ibus status icon, and show it again when it get the
> ownership back.

Done.

> 
> And if the behavior is configurable, it will be much better.
> 
> http://codereview.appspot.com/4279042/diff/1/ui/gtk/main.py#newcode97
> ui/gtk/main.py:97: stop_by_shell = False
> I think it is not good to has such argument. It is for gnome-shell only.
> 
> I suggest to add something like "not replace the current ui".
> 
> If it is true, it will check if there is a ibus ui, if it is, it will not
> replace it.

OK, I understood what you meant here after we talked off line. Thanks.
You'd suggest panel.py does not have to set ibus.BUS_NAME_FLAG_REPLACE_EXISTING
.
I thought ibus.BUS_NAME_FLAG_REPLACE_EXISTING by default.
Now I'm trying your suggestion with this Patch Set 2 but I have one problem.

If I run 'ibus-ui-gtk -r' after I run 'ibus-ui-gtk', the two icons are shown
because both receives NameAcquired signal.
I set g_dbus_message_set_destination() in bus_dbus_impl_name_acquired() but both
receives.

Also another concern is, even if I choose 'Restart' on panel GUI, the process is
not terminated.

[Peng, 2011-04-06 13:46:15]

On 2011/04/06 09:46:48, fujiwara wrote:
> On 2011/03/11 15:19:20, Peng wrote:
> > http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c
> > File bus/dbusimpl.c (right):
> > 
> > http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode200
> > bus/dbusimpl.c:200: "    <method name='ListQueuedOwners'>"
> > It is better to implement it as well.
> 
> OK, done.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode255
> > bus/dbusimpl.c:255: g_assert (connection != NULL);
> > g_assert(BUS_IS_CONNECTION(connection)) ?
> 
> Done.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode257
> > bus/dbusimpl.c:257: owner = (BusConnectionOwner *) g_new0
(BusConnectionOwner,
> > 1);
> > How about use g_slice_new() and g_slice_free()
> 
> Done.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode300
> > bus/dbusimpl.c:300: service = g_new0 (BusNameService, 1);
> > g_slice_new() and g_slice_free()?
> 
> Done.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode338
> > bus/dbusimpl.c:338: if (dbus) {
> > I think dbus should not be NULL. right?
> > So check it is not necessary?
> > maybe just need g_assert().
> 
> Done.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode368
> > bus/dbusimpl.c:368: if (dbus && service->owners == NULL) {
> > Same with Line 338?
> 
> Done.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode372
> > bus/dbusimpl.c:372: g_object_ref (owner->conn),
> > Need g_object_ref()? Please double check it.
> 
> Done but I'll test this later as we talked off line.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode394
> > bus/dbusimpl.c:394: if (dbus &&
> > Same with Line 338
> 
> Done.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode399
> > bus/dbusimpl.c:399: g_object_ref (owner->conn),
> > need g_obejct_ref()? Could you double check it.
> 
> Done and same above.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode1002
> > bus/dbusimpl.c:1002: static guint32 serial = 0;
> > Could you change serial to object member, and make all functions share a
same
> > serial?
> 
> OK, I understood what you meant.
> My understanding is, the signal is sent by connection but not dbusimpl so I
> added the serial in the connection.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/bus/dbusimpl.c#newcode1372
> > bus/dbusimpl.c:1372: "");
> > Is new owner always ""? If there is a connection are waiting the name, it
will
> > be the new owner.
> 
> Done.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/ibus/common.py
> > File ibus/common.py (right):
> > 
> > http://codereview.appspot.com/4279042/diff/1/ibus/common.py#newcode56
> > ibus/common.py:56: "is_running_gnome_shell",
> > Do we need this?
> 
> OK, I'll be back again after we discuss NameLost/NameAcquired.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/src/ibustext.h
> > File src/ibustext.h (right):
> > 
> > http://codereview.appspot.com/4279042/diff/1/src/ibustext.h#newcode179
> > src/ibustext.h:179: gboolean         ibus_text_get_is_static           
> > (IBusText       *text);
> > js should does not need this function. Please skip it too.
> 
> Done.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/src/ibusutil.c
> > File src/ibusutil.c (right):
> > 
> > http://codereview.appspot.com/4279042/diff/1/src/ibusutil.c#newcode100
> > src/ibusutil.c:100: filename = g_build_filename ("/usr",
> > "share/xml/iso-codes/iso_639.xml", NULL);
> > It is better to not hard code "/usr"?
> 
> Done.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/src/ibusutil.c#newcode137
> > src/ibusutil.c:137: ibus_is_running_gnome_shell (void)
> > Is it necessary?
> 
> Same condition above.
> 
> > 
> > http://codereview.appspot.com/4279042/diff/1/ui/gtk/main.py
> > File ui/gtk/main.py (right):
> > 
> > http://codereview.appspot.com/4279042/diff/1/ui/gtk/main.py#newcode76
> > ui/gtk/main.py:76: print "Got NameLost signal", name
> > Maybe it is not good to quit when lost the name ownership.
> > Instead we could hide ibus status icon, and show it again when it get the
> > ownership back.
> 
> Done.
> 
> > 
> > And if the behavior is configurable, it will be much better.
> > 
> > http://codereview.appspot.com/4279042/diff/1/ui/gtk/main.py#newcode97
> > ui/gtk/main.py:97: stop_by_shell = False
> > I think it is not good to has such argument. It is for gnome-shell only.
> > 
> > I suggest to add something like "not replace the current ui".
> > 
> > If it is true, it will check if there is a ibus ui, if it is, it will not
> > replace it.
> 
> OK, I understood what you meant here after we talked off line. Thanks.
> You'd suggest panel.py does not have to set
ibus.BUS_NAME_FLAG_REPLACE_EXISTING
> .
> I thought ibus.BUS_NAME_FLAG_REPLACE_EXISTING by default.
> Now I'm trying your suggestion with this Patch Set 2 but I have one problem.
> 
> If I run 'ibus-ui-gtk -r' after I run 'ibus-ui-gtk', the two icons are shown
> because both receives NameAcquired signal.
> I set g_dbus_message_set_destination() in bus_dbus_impl_name_acquired() but
both
> receives.

I think it should be a bug in somewhere. I will have you to check it.
> 
> Also another concern is, even if I choose 'Restart' on panel GUI, the process
is
> not terminated.

I should be OK, if users start some panels by self, it does not exit.

[Peng, 2011-04-06 13:46:20]

http://codereview.appspot.com/4279042/diff/12001/ui/gtk/main.py
File ui/gtk/main.py (right):

http://codereview.appspot.com/4279042/diff/12001/ui/gtk/main.py#newcode83
ui/gtk/main.py:83: print "Got NameAcquired signal", name
Remove print

http://codereview.appspot.com/4279042/diff/12001/ui/gtk/main.py#newcode87
ui/gtk/main.py:87: print "Got NameLost signal", name
Remove print

http://codereview.appspot.com/4279042/diff/12001/ui/gtk/main.py#newcode103
ui/gtk/main.py:103: print >> out, "-d, --daemonize        daemonize ibus"
Add help message for -r --replace

[Peng, 2011-04-07 19:32:58]

I created a new CL http://codereview.appspot.com/4368056/ based on this CL. It
refines some logic and fix some bugs.

On 2011/04/06 13:46:20, Peng wrote:
> http://codereview.appspot.com/4279042/diff/12001/ui/gtk/main.py
> File ui/gtk/main.py (right):
> 
> http://codereview.appspot.com/4279042/diff/12001/ui/gtk/main.py#newcode83
> ui/gtk/main.py:83: print "Got NameAcquired signal", name
> Remove print
> 
> http://codereview.appspot.com/4279042/diff/12001/ui/gtk/main.py#newcode87
> ui/gtk/main.py:87: print "Got NameLost signal", name
> Remove print
> 
> http://codereview.appspot.com/4279042/diff/12001/ui/gtk/main.py#newcode103
> ui/gtk/main.py:103: print >> out, "-d, --daemonize        daemonize ibus"
> Add help message for -r --replace

[Peng, 2011-04-18 21:05:14]

I think this CL could be closed. Please close it. Thanks.

On 2011/04/07 19:32:58, Peng wrote:
> I created a new CL http://codereview.appspot.com/4368056/ based on this CL. It
> refines some logic and fix some bugs.
> 
> On 2011/04/06 13:46:20, Peng wrote:
> > http://codereview.appspot.com/4279042/diff/12001/ui/gtk/main.py
> > File ui/gtk/main.py (right):
> > 
> > http://codereview.appspot.com/4279042/diff/12001/ui/gtk/main.py#newcode83
> > ui/gtk/main.py:83: print "Got NameAcquired signal", name
> > Remove print
> > 
> > http://codereview.appspot.com/4279042/diff/12001/ui/gtk/main.py#newcode87
> > ui/gtk/main.py:87: print "Got NameLost signal", name
> > Remove print
> > 
> > http://codereview.appspot.com/4279042/diff/12001/ui/gtk/main.py#newcode103
> > ui/gtk/main.py:103: print >> out, "-d, --daemonize        daemonize ibus"
> > Add help message for -r --replace