Load the system registry cache prior to the user one.

Load the system registry cache prior to the user one.

gnome-settings-daemon runs ibus-daemon after the user configures
any input method engines and causes a delay to show the engines
on UI because no cache exists for ibus-daemon.
The system cache can avoid the user timing.

[fujiwara, 2013-06-25 08:13:08]

I updated ibus command to have read-cache and write-cache sub commands.

[Peng, 2013-06-25 18:06:53]

https://codereview.appspot.com/10364043/diff/1/bus/registry.c
File bus/registry.c (right):

https://codereview.appspot.com/10364043/diff/1/bus/registry.c#newcode137
bus/registry.c:137: */
I think system cache should not be writable by normal users. What does it mean
'generate it'?

https://codereview.appspot.com/10364043/diff/6001/bus/ibus-daemon.1.in
File bus/ibus-daemon.1.in (right):

https://codereview.appspot.com/10364043/diff/6001/bus/ibus-daemon.1.in#newcode60
bus/ibus-daemon.1.in:60: auto, refresh, none is available.
This part is difficult to review? Could you please attach the diff of man
ibus-daemon output here as a codereview comment?

https://codereview.appspot.com/10364043/diff/6001/bus/registry.c
File bus/registry.c (right):

https://codereview.appspot.com/10364043/diff/6001/bus/registry.c#newcode64
bus/registry.c:64: (BusRegistry        *registry);
I think it can be in one line now.

https://codereview.appspot.com/10364043/diff/6001/bus/registry.c#newcode91
bus/registry.c:91: IBusRegistry *iregistry = IBUS_REGISTRY (registry);
Maybe rename it to parent?

https://codereview.appspot.com/10364043/diff/6001/bus/registry.c#newcode127
bus/registry.c:127: p_head = ibus_registry_get_components (iregistry);
rename it to components and declare it here. Maybe it make code more readable?

https://codereview.appspot.com/10364043/diff/6001/src/ibusregistry.c
File src/ibusregistry.c (right):

https://codereview.appspot.com/10364043/diff/6001/src/ibusregistry.c#newcode39
src/ibusregistry.c:39: static void         ibus_registry_destroy
put it in single line?

https://codereview.appspot.com/10364043/diff/6001/tools/main.vala
File tools/main.vala (right):

https://codereview.appspot.com/10364043/diff/6001/tools/main.vala#newcode240
tools/main.vala:240: if (argv.length > 1 && argv[1] == "system") {
--system ?

[Peng, 2013-06-25 18:09:02]

+Daiki as reviewer. As this CL is related to CL
https://codereview.appspot.com/9963043/

[Daiki Ueno, 2013-06-26 06:22:53]

Well, overall, the separation of src/ibusregistry.c from bus/registry.c looks
not very clear.  Particularly I wonder why BusRegistry derives from
IBusRegistry, though no virtual method of IBusRegistry is overridden in
BusRegistry.

If I were you I'd restructure the code something like.

* add an IBusRegistry instance member of BusRegistry, instead of the parent
instance

  struct BusRegistry
  {
    IBusObject parent;

    IBusRegistry *registry;

    /* list of BusComponent */
    GList *components;

    ...
  };

* make ibus_registry_load / ibus_registry_load_in_dir as a constructor

  IBusRegistry *ibus_registry_load (const gchar *dirname);

  static void
  bus_registry_init (BusRegistry *registry)
  {
    ...
    registry->registry = ibus_registry_load (...);
    ...
  }

* replace ibus_registry_remove_all() calls with destroy and creation

  ibus_object_destroy (IBUS_OBJECT (registry->registry));
  registry->registry = ibus_registry_load (...);

Some minor comments are below.

https://codereview.appspot.com/10364043/diff/6001/bus/registry.c
File bus/registry.c (right):

https://codereview.appspot.com/10364043/diff/6001/bus/registry.c#newcode118
bus/registry.c:118: if (ibus_registry_load_cache (iregistry, TRUE) == FALSE ||
Can't this be replaced by calling "ibus write-cache" command (either calling
here or from /etc/xdg/autostart/)?

https://codereview.appspot.com/10364043/diff/6001/src/ibusregistry.h
File src/ibusregistry.h (right):

https://codereview.appspot.com/10364043/diff/6001/src/ibusregistry.h#newcode141
src/ibusregistry.h:141: * @returns: (transfer none) (element-type
IBusComponent):
(transfer none)?  The implementation returns g_list_copy
(registry->priv->components).

https://codereview.appspot.com/10364043/diff/6001/src/ibusregistry.h#newcode154
src/ibusregistry.h:154: IBusComponent   *ibus_registry_lookup_component_by_name
I don't think this is so useful as a public API.  Users can easily traverse the
list.

https://codereview.appspot.com/10364043/diff/6001/src/ibusregistry.h#newcode160
src/ibusregistry.h:160: * @returns: (transfer none) (element-type
IBusObservedPath):
(transfer none)?

https://codereview.appspot.com/10364043/diff/6001/tools/main.vala
File tools/main.vala (right):

https://codereview.appspot.com/10364043/diff/6001/tools/main.vala#newcode23
tools/main.vala:23: extern const string IBUS_CACHE_DIR;
Maybe good to define this in config.vapi.

[fujiwara, 2013-06-27 19:11:26]

OK, the member of IBusRegistry makes sense.
I updated it.

Since both ibus_registry_load and ibus_registry_load_cache is needed, probably I
think these APIs are still needed as ibus_registry_new_with_load and
ibus_registry_new_with_load_cache also can be provided.

https://codereview.appspot.com/10364043/diff/6001/bus/registry.c
File bus/registry.c (right):

https://codereview.appspot.com/10364043/diff/6001/bus/registry.c#newcode118
bus/registry.c:118: if (ibus_registry_load_cache (iregistry, TRUE) == FALSE ||
On 2013/06/26 06:22:53, Daiki Ueno wrote:
> Can't this be replaced by calling "ibus write-cache" command (either calling
> here or from /etc/xdg/autostart/)?

Probably I don't think calling the external command is better.
I also think having two desktop files of ibus-daemon and ibus is not better
because I think which program runs faster.

https://codereview.appspot.com/10364043/diff/6001/src/ibusregistry.h
File src/ibusregistry.h (right):

https://codereview.appspot.com/10364043/diff/6001/src/ibusregistry.h#newcode141
src/ibusregistry.h:141: * @returns: (transfer none) (element-type
IBusComponent):
On 2013/06/26 06:22:53, Daiki Ueno wrote:
> (transfer none)?  The implementation returns g_list_copy
> (registry->priv->components).

g_list_copy is not enough for the transfer without ref.
Now I just removed g_list_copy.

https://codereview.appspot.com/10364043/diff/6001/src/ibusregistry.h#newcode154
src/ibusregistry.h:154: IBusComponent   *ibus_registry_lookup_component_by_name
On 2013/06/26 06:22:53, Daiki Ueno wrote:
> I don't think this is so useful as a public API.  Users can easily traverse
the
> list.

Done.

https://codereview.appspot.com/10364043/diff/6001/tools/main.vala
File tools/main.vala (right):

https://codereview.appspot.com/10364043/diff/6001/tools/main.vala#newcode23
tools/main.vala:23: extern const string IBUS_CACHE_DIR;
On 2013/06/26 06:22:53, Daiki Ueno wrote:
> Maybe good to define this in config.vapi.

The default $localstatedir is '${prefix}/var' so I think it's handled in
Makefile instead of config.h .

[fujiwara, 2013-06-27 19:28:42]

Thinking about 'ibus write-cache --system', I think some installers need to
modify the different system directory, e.g. /a/var/cache/ibus/bus/registy.xml
and /a/usr/share/ibus/component.
Now I added 'ibus write-cache --file=FILE' option for the custom path.
$IBUS_COMPONENT_PATH also can be available?

https://codereview.appspot.com/10364043/diff/1/bus/registry.c
File bus/registry.c (right):

https://codereview.appspot.com/10364043/diff/1/bus/registry.c#newcode137
bus/registry.c:137: */
On 2013/06/25 18:06:54, Peng wrote:
> I think system cache should not be writable by normal users. What does it mean
> 'generate it'?

You referred the old patch.

https://codereview.appspot.com/10364043/diff/6001/bus/ibus-daemon.1.in
File bus/ibus-daemon.1.in (right):

https://codereview.appspot.com/10364043/diff/6001/bus/ibus-daemon.1.in#newcode60
bus/ibus-daemon.1.in:60: auto, refresh, none is available.
On 2013/06/25 18:06:54, Peng wrote:
> This part is difficult to review? Could you please attach the diff of man
> ibus-daemon output here as a codereview comment?

I just replaced "--re-scan" with "--cache=mode".

ibus/bus/ibus-daemon.1.in
--t, --re-scan
- force to re-scan components, and re-create registry cache. 
+-t, --cache=mode [default is auto]
+ force to re-scan components, and re-create registry cache. auto, refresh, none
is available.

ibus-1.5.2/tools/ibus.1.in
+read-cache [--system|--file=FILE\]
+Show the content of the user registry cache if --system is not given. Show the
content of the system registry cache if --system is given. Show the content of
the custom registry cache FILE if --file=FILE is given.
+
+write-cache [--system|--file=FILE] [--force]
+Save the user registry cache if --system is not given. Save the system registry
cache if --system is given. Save the custom registry cache FILE if --file=FILE
is given. Save the cache forcibly even if it exists when --force is given.

https://codereview.appspot.com/10364043/diff/6001/bus/registry.c
File bus/registry.c (right):

https://codereview.appspot.com/10364043/diff/6001/bus/registry.c#newcode64
bus/registry.c:64: (BusRegistry        *registry);
On 2013/06/25 18:06:54, Peng wrote:
> I think it can be in one line now.

Done.

https://codereview.appspot.com/10364043/diff/6001/bus/registry.c#newcode91
bus/registry.c:91: IBusRegistry *iregistry = IBUS_REGISTRY (registry);
On 2013/06/25 18:06:54, Peng wrote:
> Maybe rename it to parent?

I modified this in the latest and now it's not parent.

https://codereview.appspot.com/10364043/diff/6001/src/ibusregistry.c
File src/ibusregistry.c (right):

https://codereview.appspot.com/10364043/diff/6001/src/ibusregistry.c#newcode39
src/ibusregistry.c:39: static void         ibus_registry_destroy
On 2013/06/25 18:06:54, Peng wrote:
> put it in single line?

Done.

https://codereview.appspot.com/10364043/diff/6001/tools/main.vala
File tools/main.vala (right):

https://codereview.appspot.com/10364043/diff/6001/tools/main.vala#newcode240
tools/main.vala:240: if (argv.length > 1 && argv[1] == "system") {
On 2013/06/25 18:06:54, Peng wrote:
> --system ?

I used GOptionEntry.
It cannot be applied to ibus main help because GOptionEntry does not support sub
commands and it seems vala does not have APIs of sub commands.

[Peng, 2013-06-28 14:45:22]

https://codereview.appspot.com/10364043/diff/1/bus/registry.c
File bus/registry.c (right):

https://codereview.appspot.com/10364043/diff/1/bus/registry.c#newcode137
bus/registry.c:137: */
On 2013/06/27 19:28:43, fujiwara wrote:
> On 2013/06/25 18:06:54, Peng wrote:
> > I think system cache should not be writable by normal users. What does it
mean
> > 'generate it'?
> 
> You referred the old patch.

I mean you should not let ibus-daemon to save the register to system cache
folder.

https://codereview.appspot.com/10364043/diff/19001/bus/registry.c
File bus/registry.c (right):

https://codereview.appspot.com/10364043/diff/19001/bus/registry.c#newcode39
bus/registry.c:39: struct _BusRegistry {
Looks like this class is not necessary any more. We could just move several
function info ibusimpl class? It will save a lot for code for creating a class.

https://codereview.appspot.com/10364043/diff/19001/bus/registry.c#newcode134
bus/registry.c:134: p = ibus_registry_get_components (iregistry);
You may move this line into for(;;)

https://codereview.appspot.com/10364043/diff/19001/bus/registry.c#newcode160
bus/registry.c:160: }
Does not need free p returned from ibus_registry_get_components()?

https://codereview.appspot.com/10364043/diff/19001/tools/main.vala
File tools/main.vala (right):

https://codereview.appspot.com/10364043/diff/19001/tools/main.vala#newcode250
tools/main.vala:250: print ("%s\n", contents);
Maybe it is better to verify the contents first to make sure it is in right
format, and then print the output from IBusRegister class.

https://codereview.appspot.com/10364043/diff/19001/tools/main.vala#newcode263
tools/main.vala:263: { "force", 0, 0, OptionArg.NONE, out force,
I think the force options is not necessary. It should always overwrite the file.

https://codereview.appspot.com/10364043/diff/19001/tools/main.vala#newcode354
tools/main.vala:354: new_argv[0] = "%s %s".printf(program_name, new_argv[0]);
Don't understand this. Could you please explain it?

[fujiwara, 2013-07-04 17:04:27]

https://codereview.appspot.com/10364043/diff/1/bus/registry.c
File bus/registry.c (right):

https://codereview.appspot.com/10364043/diff/1/bus/registry.c#newcode137
bus/registry.c:137: */
On 2013/06/28 14:45:22, Peng wrote:
> On 2013/06/27 19:28:43, fujiwara wrote:
> > On 2013/06/25 18:06:54, Peng wrote:
> > > I think system cache should not be writable by normal users. What does it
> mean
> > > 'generate it'?
> > 
> > You referred the old patch.
> 
> I mean you should not let ibus-daemon to save the register to system cache
> folder.

I meant the newer patch does not save the system cache.

https://codereview.appspot.com/10364043/diff/19001/bus/registry.c
File bus/registry.c (right):

https://codereview.appspot.com/10364043/diff/19001/bus/registry.c#newcode39
bus/registry.c:39: struct _BusRegistry {
On 2013/06/28 14:45:22, Peng wrote:
> Looks like this class is not necessary any more. We could just move several
> function info ibusimpl class? It will save a lot for code for creating a
class.

OK, ibusimpl is updated.

https://codereview.appspot.com/10364043/diff/19001/bus/registry.c#newcode160
bus/registry.c:160: }
On 2013/06/28 14:45:22, Peng wrote:
> Does not need free p returned from ibus_registry_get_components()?

OK, now container annotation is added.

https://codereview.appspot.com/10364043/diff/19001/tools/main.vala
File tools/main.vala (right):

https://codereview.appspot.com/10364043/diff/19001/tools/main.vala#newcode250
tools/main.vala:250: print ("%s\n", contents);
On 2013/06/28 14:45:22, Peng wrote:
> Maybe it is better to verify the contents first to make sure it is in right
> format, and then print the output from IBusRegister class.

I added ibus_registry_output().

https://codereview.appspot.com/10364043/diff/19001/tools/main.vala#newcode263
tools/main.vala:263: { "force", 0, 0, OptionArg.NONE, out force,
On 2013/06/28 14:45:22, Peng wrote:
> I think the force options is not necessary. It should always overwrite the
file.

Done.

https://codereview.appspot.com/10364043/diff/19001/tools/main.vala#newcode354
tools/main.vala:354: new_argv[0] = "%s %s".printf(program_name, new_argv[0]);
On 2013/06/28 14:45:22, Peng wrote:
> Don't understand this. Could you please explain it?

ibus read-cache --help shows: "read-cache [OPTIONS]".
I changed it to "ibus read-cache [OPTIONS]".

[Daiki Ueno, 2013-07-05 07:32:38]

If you remove bus/registry.[ch], you also need to remove them from
bus/Makefile.am.  Otherwise ibus-daemon does not build.

Anyway it seems becoming better, though I'm not sure if the new API is optimal.
 
Some minor comments below.

https://codereview.appspot.com/10364043/diff/6001/tools/main.vala
File tools/main.vala (right):

https://codereview.appspot.com/10364043/diff/6001/tools/main.vala#newcode23
tools/main.vala:23: extern const string IBUS_CACHE_DIR;
On 2013/06/27 19:11:27, fujiwara wrote:
> On 2013/06/26 06:22:53, Daiki Ueno wrote:
> > Maybe good to define this in config.vapi.
> 
> The default $localstatedir is '${prefix}/var' so I think it's handled in
> Makefile instead of config.h .

It is not necessarily defined in config.h.  Macros specified with -D in Makefile
are also accessible from config.vapi (see BINDIR).  Anyway, the latest CL is not
using this, so it could be simply removed.

https://codereview.appspot.com/10364043/diff/19001/tools/main.vala
File tools/main.vala (right):

https://codereview.appspot.com/10364043/diff/19001/tools/main.vala#newcode354
tools/main.vala:354: new_argv[0] = "%s %s".printf(program_name, new_argv[0]);
On 2013/07/04 17:04:28, fujiwara wrote:
> On 2013/06/28 14:45:22, Peng wrote:
> > Don't understand this. Could you please explain it?
> 
> ibus read-cache --help shows: "read-cache [OPTIONS]".
> I changed it to "ibus read-cache [OPTIONS]".

Maybe using Environment.set_prgname() makes it a bit clearer.

https://codereview.appspot.com/10364043/diff/34001/bus/ibusimpl.h
File bus/ibusimpl.h (right):

https://codereview.appspot.com/10364043/diff/34001/bus/ibusimpl.h#newcode81
bus/ibusimpl.h:81: IBusRegistry    *bus_ibus_impl_get_registry        
(BusIBusImpl        *ibus);
This function is no longer used by any other code.

https://codereview.appspot.com/10364043/diff/34001/src/ibusregistry.h
File src/ibusregistry.h (right):

https://codereview.appspot.com/10364043/diff/34001/src/ibusregistry.h#newcode119
src/ibusregistry.h:119: * Load the user or system registry cache.
Missing the description of return value, and use #IBusRegistry and %TRUE
consistently.

https://codereview.appspot.com/10364043/diff/34001/src/ibusregistry.h#newcode129
src/ibusregistry.h:129: * Load the registry cache @filename.
Ditto.

https://codereview.appspot.com/10364043/diff/34001/src/ibusregistry.h#newcode139
src/ibusregistry.h:139: * Save the registry in a user directory or system
directory.
Ditto.

https://codereview.appspot.com/10364043/diff/34001/src/ibusregistry.h#newcode170
src/ibusregistry.h:170: * Check if the registry is updated.
Missing parameter and return value description.

https://codereview.appspot.com/10364043/diff/34001/src/ibusregistry.h#newcode180
src/ibusregistry.h:180: */
Missing function description.

https://codereview.appspot.com/10364043/diff/34001/src/ibusregistry.h#newcode187
src/ibusregistry.h:187: * a list of #IBusObservedPath objects. Should not be
freed.
Ditto.

[fujiwara, 2013-07-05 13:24:38]

On 2013/07/05 07:32:38, Daiki Ueno wrote:
> If you remove bus/registry.[ch], you also need to remove them from
> bus/Makefile.am.  Otherwise ibus-daemon does not build.
> 

Forgot to upload the Makefile.am.

[Peng, 2013-07-08 14:05:30]

https://codereview.appspot.com/10364043/diff/38001/bus/ibusimpl.c
File bus/ibusimpl.c (right):

https://codereview.appspot.com/10364043/diff/38001/bus/ibusimpl.c#newcode65
bus/ibusimpl.c:65: IBusRegistry    *registry;
Add one blank line please

https://codereview.appspot.com/10364043/diff/38001/bus/ibusimpl.c#newcode68
bus/ibusimpl.c:68: GList *components;
same

https://codereview.appspot.com/10364043/diff/38001/bus/ibusimpl.c#newcode72
bus/ibusimpl.c:72: gboolean registry_changed;
same

https://codereview.appspot.com/10364043/diff/38001/bus/ibusimpl.c#newcode107
bus/ibusimpl.c:107: static void     bus_ibus_impl_destroy  (BusIBusImpl       
*ibus);
The '(' is not align with others

https://codereview.appspot.com/10364043/diff/38001/bus/ibusimpl.c#newcode2046
bus/ibusimpl.c:2046: * Start the monitor thread.
Seems there is no thread anymore?

https://codereview.appspot.com/10364043/diff/38001/bus/ibusimpl.c#newcode2049
bus/ibusimpl.c:2049: bus_ibus_impl_start_monitor_changes (BusIBusImpl *ibus)
How about move this feature to IBusRegistry class?

https://codereview.appspot.com/10364043/diff/38001/src/tests/Makefile.am
File src/tests/Makefile.am (right):

https://codereview.appspot.com/10364043/diff/38001/src/tests/Makefile.am#newc...
src/tests/Makefile.am:51: ibus-registry \
Align \

https://codereview.appspot.com/10364043/diff/38001/tools/main.vala
File tools/main.vala (right):

https://codereview.appspot.com/10364043/diff/38001/tools/main.vala#newcode28
tools/main.vala:28: string cache_file = null;
Make those variable as local value?

[fujiwara, 2013-07-09 01:41:54]

https://codereview.appspot.com/10364043/diff/38001/bus/ibusimpl.c
File bus/ibusimpl.c (right):

https://codereview.appspot.com/10364043/diff/38001/bus/ibusimpl.c#newcode65
bus/ibusimpl.c:65: IBusRegistry    *registry;
On 2013/07/08 14:05:31, Peng wrote:
> Add one blank line please

Done.

https://codereview.appspot.com/10364043/diff/38001/bus/ibusimpl.c#newcode107
bus/ibusimpl.c:107: static void     bus_ibus_impl_destroy  (BusIBusImpl       
*ibus);
On 2013/07/08 14:05:31, Peng wrote:
> The '(' is not align with others

Done.

https://codereview.appspot.com/10364043/diff/38001/bus/ibusimpl.c#newcode2046
bus/ibusimpl.c:2046: * Start the monitor thread.
On 2013/07/08 14:05:31, Peng wrote:
> Seems there is no thread anymore?

I changed the description.

https://codereview.appspot.com/10364043/diff/38001/bus/ibusimpl.c#newcode2049
bus/ibusimpl.c:2049: bus_ibus_impl_start_monitor_changes (BusIBusImpl *ibus)
On 2013/07/08 14:05:31, Peng wrote:
> How about move this feature to IBusRegistry class?

Done.

https://codereview.appspot.com/10364043/diff/38001/src/tests/Makefile.am
File src/tests/Makefile.am (right):

https://codereview.appspot.com/10364043/diff/38001/src/tests/Makefile.am#newc...
src/tests/Makefile.am:51: ibus-registry \
On 2013/07/08 14:05:31, Peng wrote:
> Align \

Done.

https://codereview.appspot.com/10364043/diff/38001/tools/main.vala
File tools/main.vala (right):

https://codereview.appspot.com/10364043/diff/38001/tools/main.vala#newcode28
tools/main.vala:28: string cache_file = null;
On 2013/07/08 14:05:31, Peng wrote:
> Make those variable as local value?

It seems vala need the global value for OptionEntry.
If I moved those variables into local, the either compile error was caused:

main.vala:209.35-209.35: error: Value must be constant
    const OptionEntry[] options = {

or

main.vala:210.9-210.9: error: Expected array element, got array initializer list
        { "system", 0, 0, OptionArg.NONE, out is_system,

[Peng, 2013-07-09 03:18:46]

lgtm with one comment

https://codereview.appspot.com/10364043/diff/61001/src/ibusregistry.c
File src/ibusregistry.c (right):

https://codereview.appspot.com/10364043/diff/61001/src/ibusregistry.c#newcode73
src/ibusregistry.c:73: _signals[CHANGED] =
It is better to add some document here.

[Daiki Ueno, 2013-07-09 03:32:41]

You didn't answer the most part of comment #10.  Maybe intentionally.  Anyway,
it is still failing to build:

make[3]: Entering directory `/home/ueno/devel/ibus/bus'
make[3]: *** No rule to make target `test-registry.c', needed by
`test_registry-test-registry.o'.  Stop.

Also, have you looked at the generated gtk-doc?  IBusRegistry is not mentioned
in ibus-docs.sgml.in.

https://codereview.appspot.com/10364043/diff/61001/src/ibusregistry.h
File src/ibusregistry.h (right):

https://codereview.appspot.com/10364043/diff/61001/src/ibusregistry.h#newcode88
src/ibusregistry.h:88: * New a IBusRegistry
Still missing @returns.  Also #IBusRegistry.  Please check others carefully.

https://codereview.appspot.com/10364043/diff/61001/src/ibusregistry.h#newcode152
src/ibusregistry.h:152: * @filename: The file path of the registroy cache
s/registroy/registry/

https://codereview.appspot.com/10364043/diff/61001/src/ibusregistry.h#newcode188
src/ibusregistry.h:188: * The caller has to call g_list_free for the returned
list.
g_list_free().

https://codereview.appspot.com/10364043/diff/61001/tools/main.vala
File tools/main.vala (right):

https://codereview.appspot.com/10364043/diff/61001/tools/main.vala#newcode23
tools/main.vala:23: extern const string IBUS_CACHE_DIR;
Why is it still needed?

[fujiwara, 2013-07-09 15:31:51]

On 2013/07/09 03:32:41, Daiki Ueno wrote:
> You didn't answer the most part of comment #10.  Maybe intentionally.  Anyway,
> it is still failing to build:
> 
> make[3]: Entering directory `/home/ueno/devel/ibus/bus'
> make[3]: *** No rule to make target `test-registry.c', needed by
> `test_registry-test-registry.o'.  Stop.

Thanks. It seems I modified Makefile.am.orig by mistake.

> Also, have you looked at the generated gtk-doc?  IBusRegistry is not mentioned
> in ibus-docs.sgml.in.

Done.

> 
> https://codereview.appspot.com/10364043/diff/61001/src/ibusregistry.h
> File src/ibusregistry.h (right):
> 
>
https://codereview.appspot.com/10364043/diff/61001/src/ibusregistry.h#newcode88
> src/ibusregistry.h:88: * New a IBusRegistry
> Still missing @returns.  Also #IBusRegistry.  Please check others carefully.

Done.
I think #IBusRegistry is nice to have.

> https://codereview.appspot.com/10364043/diff/61001/tools/main.vala
> File tools/main.vala (right):
> 
> https://codereview.appspot.com/10364043/diff/61001/tools/main.vala#newcode23
> tools/main.vala:23: extern const string IBUS_CACHE_DIR;
> Why is it still needed?

Done.

[fujiwara, 2013-07-09 15:33:07]

https://codereview.appspot.com/10364043/diff/61001/src/ibusregistry.c
File src/ibusregistry.c (right):

https://codereview.appspot.com/10364043/diff/61001/src/ibusregistry.c#newcode73
src/ibusregistry.c:73: _signals[CHANGED] =
On 2013/07/09 03:18:47, Peng wrote:
> It is better to add some document here.

Done.

[Peng, 2013-07-09 16:04:24]

On 2013/07/09 15:33:07, fujiwara wrote:
> https://codereview.appspot.com/10364043/diff/61001/src/ibusregistry.c
> File src/ibusregistry.c (right):
> 
>
https://codereview.appspot.com/10364043/diff/61001/src/ibusregistry.c#newcode73
> src/ibusregistry.c:73: _signals[CHANGED] =
> On 2013/07/09 03:18:47, Peng wrote:
> > It is better to add some document here.
> 
> Done.

BTW, Please try generate the gtk-doc, and make sure the doc includes the new API
correctly.

[Daiki Ueno, 2013-07-11 02:08:51]

Why you can't wait for all the reviewers' go.  LGTM anyway.