|
|
DescriptionAdd some comments about registry
BUG=none
TEST=built fine
Patch Set 1 #
Total comments: 17
Patch Set 2 : Refine comments #Patch Set 3 : Update comments #Patch Set 4 : Update #MessagesTotal messages: 8
Thank you for adding comments! http://codereview.appspot.com/2159046/diff/1/bus/ibusimpl.c File bus/ibusimpl.c (right): http://codereview.appspot.com/2159046/diff/1/bus/ibusimpl.c#newcode614 bus/ibusimpl.c:614: /* start monitor of changes of registry */ Let's write comments in a good manner (start with a capital letter and give it a period at the end): Start monitor of changes of registry. http://codereview.appspot.com/2159046/diff/1/bus/registry.c File bus/registry.c (right): http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode76 bus/registry.c:76: #ifdef G_THREADS_ENABLED Let's explain something about threads, like: If threads are enabled, we'll create a thread to monitor changes in IME XML files and related files, so users can use newly installed IMEs immediately. Note that we don't use GFileMonitor for monitoring as we need to monitor not only XML files but also other related files that can be scattered in many places. Monitoring these files using GFileMonitor would be complicated, hence we use a thread instead. http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode138 bus/registry.c:138: g_cond_signal (registry->cond); Let's add something like: // Raise a signal so that the loop in _check_changes() terminates immediately. http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode496 bus/registry.c:496: /* Wait for the condition changed or timeout */ A period is missing. http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode499 bus/registry.c:499: * and other related files. If any file is changed, we will emit a changed related files -> related files specified in <observed-paths> in the XML files. http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode501 bus/registry.c:501: * It is for finding install/uninstall/upgrade of IMEs. On Linux desktop, Can you make it less than 80 chars? http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode503 bus/registry.c:503: * ibus need a restart. Would be nice to explain why use g_cond_timed_wait rather than sleep(), like: Note that we use g_cond_timed_wait() here rather than sleep()/nanosleep() so that the loop can terminate immediately when the registry object is destroyed. See also comments in bus_registry_destroy(). http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode504 bus/registry.c:504: */ IMHO, // style comments would be easier to type and read. Besides, we don't need the extra "*/" line.
Sign in to reply to this message.
Update comments.
Sign in to reply to this message.
http://codereview.appspot.com/2159046/diff/1/bus/ibusimpl.c File bus/ibusimpl.c (right): http://codereview.appspot.com/2159046/diff/1/bus/ibusimpl.c#newcode614 bus/ibusimpl.c:614: /* start monitor of changes of registry */ On 2010/09/10 05:51:46, satorux wrote: > Let's write comments in a good manner (start with a capital letter and give it a > period at the end): > > Start monitor of changes of registry. > Done. http://codereview.appspot.com/2159046/diff/1/bus/registry.c File bus/registry.c (right): http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode76 bus/registry.c:76: #ifdef G_THREADS_ENABLED On 2010/09/10 05:51:46, satorux wrote: > Let's explain something about threads, like: > > If threads are enabled, we'll create a thread to monitor changes in IME XML > files and related files, so users can use newly installed IMEs immediately. > > Note that we don't use GFileMonitor for monitoring as we need to monitor not > only XML files but also other related files that can be scattered in many > places. Monitoring these files using GFileMonitor would be complicated, hence we > use a thread instead. Done. http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode138 bus/registry.c:138: g_cond_signal (registry->cond); On 2010/09/10 05:51:46, satorux wrote: > Let's add something like: > > // Raise a signal so that the loop in _check_changes() terminates immediately. Done. http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode496 bus/registry.c:496: /* Wait for the condition changed or timeout */ On 2010/09/10 05:51:46, satorux wrote: > A period is missing. Done. http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode499 bus/registry.c:499: * and other related files. If any file is changed, we will emit a changed On 2010/09/10 05:51:46, satorux wrote: > related files -> related files specified in <observed-paths> in the XML files. Done. http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode501 bus/registry.c:501: * It is for finding install/uninstall/upgrade of IMEs. On Linux desktop, On 2010/09/10 05:51:46, satorux wrote: > Can you make it less than 80 chars? Done. http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode503 bus/registry.c:503: * ibus need a restart. On 2010/09/10 05:51:46, satorux wrote: > Would be nice to explain why use g_cond_timed_wait rather than sleep(), like: > > > Note that we use g_cond_timed_wait() here rather than sleep()/nanosleep() so > that the loop can terminate immediately when the registry object is destroyed. > See also comments in bus_registry_destroy(). Done. http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode504 bus/registry.c:504: */ On 2010/09/10 05:51:46, satorux wrote: > IMHO, // style comments would be easier to type and read. Besides, we don't need > the extra "*/" line. I know it is a suggestion in google C++ coding style. I was work close with gnome and Linux kernel before. So ibus code style looks like gnome + kernel. :) For me, /* */ is looks better. But both are OK.
Sign in to reply to this message.
LGTM, but see below. http://codereview.appspot.com/2159046/diff/1/bus/registry.c File bus/registry.c (right): http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode503 bus/registry.c:503: * ibus need a restart. On 2010/09/10 09:37:49, penghuang wrote: > On 2010/09/10 05:51:46, satorux wrote: > > Would be nice to explain why use g_cond_timed_wait rather than sleep(), like: > > > > > > Note that we use g_cond_timed_wait() here rather than sleep()/nanosleep() so > > that the loop can terminate immediately when the registry object is destroyed. > > See also comments in bus_registry_destroy(). > > Done. My comment about sleep()/nanosleep() was ignored, which is OK if you think it's worthless. In this case, please say so, rather than hitting "Done" button...
Sign in to reply to this message.
Could you update the BUG and TEST lines, like: BUG=none TEST=built fine Or you can remove these by "Edit issue" button. On 2010/09/10 10:36:10, satorux wrote: > LGTM, but see below. > > http://codereview.appspot.com/2159046/diff/1/bus/registry.c > File bus/registry.c (right): > > http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode503 > bus/registry.c:503: * ibus need a restart. > On 2010/09/10 09:37:49, penghuang wrote: > > On 2010/09/10 05:51:46, satorux wrote: > > > Would be nice to explain why use g_cond_timed_wait rather than sleep(), > like: > > > > > > > > > Note that we use g_cond_timed_wait() here rather than sleep()/nanosleep() so > > > that the loop can terminate immediately when the registry object is > destroyed. > > > See also comments in bus_registry_destroy(). > > > > Done. > > My comment about sleep()/nanosleep() was ignored, which is OK if you think it's > worthless. In this case, please say so, rather than hitting "Done" button...
Sign in to reply to this message.
On 2010/09/10 10:37:33, satorux wrote: > Could you update the BUG and TEST lines, like: > > BUG=none > TEST=built fine > > Or you can remove these by "Edit issue" button. > > > On 2010/09/10 10:36:10, satorux wrote: > > LGTM, but see below. > > > > http://codereview.appspot.com/2159046/diff/1/bus/registry.c > > File bus/registry.c (right): > > > > http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode503 > > bus/registry.c:503: * ibus need a restart. > > On 2010/09/10 09:37:49, penghuang wrote: > > > On 2010/09/10 05:51:46, satorux wrote: > > > > Would be nice to explain why use g_cond_timed_wait rather than sleep(), > > like: > > > > > > > > > > > > Note that we use g_cond_timed_wait() here rather than sleep()/nanosleep() > so > > > > that the loop can terminate immediately when the registry object is > > destroyed. > > > > See also comments in bus_registry_destroy(). > > > > > > Done. > > > > My comment about sleep()/nanosleep() was ignored, which is OK if you think > it's > > worthless. In this case, please say so, rather than hitting "Done" button... I added comments about sleep() but is in 514 line.
Sign in to reply to this message.
On 2010/09/10 10:58:23, penghuang wrote: > On 2010/09/10 10:37:33, satorux wrote: > > Could you update the BUG and TEST lines, like: > > > > BUG=none > > TEST=built fine > > > > Or you can remove these by "Edit issue" button. > > > > > > On 2010/09/10 10:36:10, satorux wrote: > > > LGTM, but see below. > > > > > > http://codereview.appspot.com/2159046/diff/1/bus/registry.c > > > File bus/registry.c (right): > > > > > > http://codereview.appspot.com/2159046/diff/1/bus/registry.c#newcode503 > > > bus/registry.c:503: * ibus need a restart. > > > On 2010/09/10 09:37:49, penghuang wrote: > > > > On 2010/09/10 05:51:46, satorux wrote: > > > > > Would be nice to explain why use g_cond_timed_wait rather than sleep(), > > > like: > > > > > > > > > > > > > > > Note that we use g_cond_timed_wait() here rather than > sleep()/nanosleep() > > so > > > > > that the loop can terminate immediately when the registry object is > > > destroyed. > > > > > See also comments in bus_registry_destroy(). > > > > > > > > Done. > > > > > > My comment about sleep()/nanosleep() was ignored, which is OK if you think > > it's > > > worthless. In this case, please say so, rather than hitting "Done" button... > > I added comments about sleep() but is in 514 line. Oops, I missed. Sorry about that!
Sign in to reply to this message.
|