Initial implementation of libpyzy.

Initial implementation of libpyzy.

BUG=
TEST=

[hsumita, 2011-10-12 08:30:13]

Unfortunately it seems that the Patchset2 contains many changes, but there are
only a few changes.
Simply you can ignore the Patchset1 or Patchset2.

[Peng, 2011-10-17 15:02:11]

I think there are many works should be done, like:
* Replace all ibus-pinyin to libpyzy in source code and makefile
* Fix all license headers in all files to use LGPL.
* Maybe implement a script based test auto test system
* Maybe implement a c language binding, so the pure c program may use this
library
* Maybe remove libibus and glib dependences
* and etc

I think it is not reasonable to make all happen in one CL. I suggest we check in
this CL first, and implement or fix them in several small CLs.

http://codereview.appspot.com/5235062/diff/10016/COPYING
File COPYING (right):

http://codereview.appspot.com/5235062/diff/10016/COPYING#newcode2
COPYING:2: Version 2, June 1991
LGPL?

http://codereview.appspot.com/5235062/diff/10016/Makefile.am
File Makefile.am (right):

http://codereview.appspot.com/5235062/diff/10016/Makefile.am#newcode3
Makefile.am:3: # ibus-pinyin - The Chinese PinYin engine for IBus
Rename ibus-pinyin to libpyzy, and change the description.

http://codereview.appspot.com/5235062/diff/10016/autogen.sh
File autogen.sh (right):

http://codereview.appspot.com/5235062/diff/10016/autogen.sh#newcode7
autogen.sh:7: PKG_NAME="ibus-pinyin"
ditto

http://codereview.appspot.com/5235062/diff/10016/configure.ac
File configure.ac (right):

http://codereview.appspot.com/5235062/diff/10016/configure.ac#newcode3
configure.ac:3: # ibus-pinyin - The Chinese PinYin engine for IBus
ditto

http://codereview.appspot.com/5235062/diff/10016/src/Makefile.am.lib
File src/Makefile.am.lib (right):

http://codereview.appspot.com/5235062/diff/10016/src/Makefile.am.lib#newcode1
src/Makefile.am.lib:1: # vim:set noet ts=4:
what's this file for

http://codereview.appspot.com/5235062/diff/10016/src/Makefile.am.test
File src/Makefile.am.test (right):

http://codereview.appspot.com/5235062/diff/10016/src/Makefile.am.test#newcode1
src/Makefile.am.test:1: # vim:set noet ts=4:
what's this file for?

http://codereview.appspot.com/5235062/diff/10016/src/PyZyConfig.cc
File src/PyZyConfig.cc (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyConfig.cc#newcode29
src/PyZyConfig.cc:29: const guint PINYIN_DEFAULT_OPTION =
unisgned int?

http://codereview.appspot.com/5235062/diff/10016/src/PyZyConfig.cc#newcode80
src/PyZyConfig.cc:80: unsigned int Config::option (void) const                  
  { return m_impl->m_option; }
Those lines are too long.

http://codereview.appspot.com/5235062/diff/10016/src/PyZyInputContext.h
File src/PyZyInputContext.h (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyInputContext.h#newco...
src/PyZyInputContext.h:43: enum vkey_code {
VKeyCode

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.cc
File src/PyZyPhoneticContext.cc (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.cc#n...
src/PyZyPhoneticContext.cc:198: m_observer->lookupTableChanged ();
Suggest hide m_observer in base class. In here, we may call a member method of
base class to notify observer.

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.h
File src/PyZyPhoneticContext.h (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.h#ne...
src/PyZyPhoneticContext.h:74: gboolean selectCandidateInPage (guint i);
unsigned int

http://codereview.appspot.com/5235062/diff/10016/src/PyZyTest.cc
File src/PyZyTest.cc (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyTest.cc#newcode121
src/PyZyTest.cc:121: EXPECT_EQ (0, observer.commitTextCount ());
How about write all test cases in a text or scripts file?
Anyway, we may implement it later, it is not necessary done in the first CL.

[hsumita, 2011-10-18 03:33:45]

Thank you for your reviewing.

I will submit this CL, and make another CLs to fix these.

On 2011/10/17 15:02:11, Peng wrote:
> I think there are many works should be done, like:
> * Replace all ibus-pinyin to libpyzy in source code and makefile
> * Fix all license headers in all files to use LGPL.
> * Maybe implement a script based test auto test system
> * Maybe implement a c language binding, so the pure c program may use this
> library
> * Maybe remove libibus and glib dependences
> * and etc
> 
> I think it is not reasonable to make all happen in one CL. I suggest we check
in
> this CL first, and implement or fix them in several small CLs.
> 
> http://codereview.appspot.com/5235062/diff/10016/COPYING
> File COPYING (right):
> 
> http://codereview.appspot.com/5235062/diff/10016/COPYING#newcode2
> COPYING:2: Version 2, June 1991
> LGPL?
> 
> http://codereview.appspot.com/5235062/diff/10016/Makefile.am
> File Makefile.am (right):
> 
> http://codereview.appspot.com/5235062/diff/10016/Makefile.am#newcode3
> Makefile.am:3: # ibus-pinyin - The Chinese PinYin engine for IBus
> Rename ibus-pinyin to libpyzy, and change the description.
> 
> http://codereview.appspot.com/5235062/diff/10016/autogen.sh
> File autogen.sh (right):
> 
> http://codereview.appspot.com/5235062/diff/10016/autogen.sh#newcode7
> autogen.sh:7: PKG_NAME="ibus-pinyin"
> ditto
> 
> http://codereview.appspot.com/5235062/diff/10016/configure.ac
> File configure.ac (right):
> 
> http://codereview.appspot.com/5235062/diff/10016/configure.ac#newcode3
> configure.ac:3: # ibus-pinyin - The Chinese PinYin engine for IBus
> ditto
> 
> http://codereview.appspot.com/5235062/diff/10016/src/Makefile.am.lib
> File src/Makefile.am.lib (right):
> 
> http://codereview.appspot.com/5235062/diff/10016/src/Makefile.am.lib#newcode1
> src/Makefile.am.lib:1: # vim:set noet ts=4:
> what's this file for
> 
> http://codereview.appspot.com/5235062/diff/10016/src/Makefile.am.test
> File src/Makefile.am.test (right):
> 
> http://codereview.appspot.com/5235062/diff/10016/src/Makefile.am.test#newcode1
> src/Makefile.am.test:1: # vim:set noet ts=4:
> what's this file for?
> 
> http://codereview.appspot.com/5235062/diff/10016/src/PyZyConfig.cc
> File src/PyZyConfig.cc (right):
> 
> http://codereview.appspot.com/5235062/diff/10016/src/PyZyConfig.cc#newcode29
> src/PyZyConfig.cc:29: const guint PINYIN_DEFAULT_OPTION =
> unisgned int?
> 
> http://codereview.appspot.com/5235062/diff/10016/src/PyZyConfig.cc#newcode80
> src/PyZyConfig.cc:80: unsigned int Config::option (void) const                
 
>   { return m_impl->m_option; }
> Those lines are too long.
> 
> http://codereview.appspot.com/5235062/diff/10016/src/PyZyInputContext.h
> File src/PyZyInputContext.h (right):
> 
>
http://codereview.appspot.com/5235062/diff/10016/src/PyZyInputContext.h#newco...
> src/PyZyInputContext.h:43: enum vkey_code {
> VKeyCode
> 
> http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.cc
> File src/PyZyPhoneticContext.cc (right):
> 
>
http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.cc#n...
> src/PyZyPhoneticContext.cc:198: m_observer->lookupTableChanged ();
> Suggest hide m_observer in base class. In here, we may call a member method of
> base class to notify observer.
> 
> http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.h
> File src/PyZyPhoneticContext.h (right):
> 
>
http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.h#ne...
> src/PyZyPhoneticContext.h:74: gboolean selectCandidateInPage (guint i);
> unsigned int
> 
> http://codereview.appspot.com/5235062/diff/10016/src/PyZyTest.cc
> File src/PyZyTest.cc (right):
> 
> http://codereview.appspot.com/5235062/diff/10016/src/PyZyTest.cc#newcode121
> src/PyZyTest.cc:121: EXPECT_EQ (0, observer.commitTextCount ());
> How about write all test cases in a text or scripts file?
> Anyway, we may implement it later, it is not necessary done in the first CL.

[hsumita, 2011-10-18 09:45:42]

I fixed some issues by other CLs and added some comments for unfixed issues.

http://codereview.appspot.com/5235062/diff/10016/COPYING
File COPYING (right):

http://codereview.appspot.com/5235062/diff/10016/COPYING#newcode2
COPYING:2: Version 2, June 1991
Done.
It is replaced by http://codereview.appspot.com/5301043/

http://codereview.appspot.com/5235062/diff/10016/Makefile.am
File Makefile.am (right):

http://codereview.appspot.com/5235062/diff/10016/Makefile.am#newcode3
Makefile.am:3: # ibus-pinyin - The Chinese PinYin engine for IBus
Done.
It is fixed by http://codereview.appspot.com/5305041/

http://codereview.appspot.com/5235062/diff/10016/autogen.sh
File autogen.sh (right):

http://codereview.appspot.com/5235062/diff/10016/autogen.sh#newcode7
autogen.sh:7: PKG_NAME="ibus-pinyin"
Done.
It is fixed by http://codereview.appspot.com/5305041/

http://codereview.appspot.com/5235062/diff/10016/configure.ac
File configure.ac (right):

http://codereview.appspot.com/5235062/diff/10016/configure.ac#newcode3
configure.ac:3: # ibus-pinyin - The Chinese PinYin engine for IBus
Done.
It is fixed by http://codereview.appspot.com/5301043/

http://codereview.appspot.com/5235062/diff/10016/src/Makefile.am.lib
File src/Makefile.am.lib (right):

http://codereview.appspot.com/5235062/diff/10016/src/Makefile.am.lib#newcode1
src/Makefile.am.lib:1: # vim:set noet ts=4:
Done.
This file is removed by http://codereview.appspot.com/5305041/

http://codereview.appspot.com/5235062/diff/10016/src/Makefile.am.test
File src/Makefile.am.test (right):

http://codereview.appspot.com/5235062/diff/10016/src/Makefile.am.test#newcode1
src/Makefile.am.test:1: # vim:set noet ts=4:
Done.
This file is removed by http://codereview.appspot.com/5305041

http://codereview.appspot.com/5235062/diff/10016/src/PyZyConfig.cc
File src/PyZyConfig.cc (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyConfig.cc#newcode29
src/PyZyConfig.cc:29: const guint PINYIN_DEFAULT_OPTION =
Done.
It is fixed by http://codereview.appspot.com/5299042.

http://codereview.appspot.com/5235062/diff/10016/src/PyZyConfig.cc#newcode80
src/PyZyConfig.cc:80: unsigned int Config::option (void) const                  
  { return m_impl->m_option; }
Done.
It is fixed by http://codereview.appspot.com/5299042/

http://codereview.appspot.com/5235062/diff/10016/src/PyZyInputContext.h
File src/PyZyInputContext.h (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyInputContext.h#newco...
src/PyZyInputContext.h:43: enum vkey_code {
Done.
It is fixed by http://codereview.appspot.com/5299042.

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.cc
File src/PyZyPhoneticContext.cc (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.cc#n...
src/PyZyPhoneticContext.cc:198: m_observer->lookupTableChanged ();
Is this means that we implements lookupTableChanged (and other functions named
*Changed or commitText) to PyZyInputContext and call it from PhoneticContext (or
other context)?

It may not good because PyZyInputContext.h is used by libpyzy user. I prefer
that this header doesn't contain any information about an inside of libpyzy.

Virtually, PhoneticContext is base class of other *Context class (of course
except for InputContext) and InputContext is merely thin wrapper. How about that
we implement notifier on PhoneticContext and other *Context class call it?

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.h
File src/PyZyPhoneticContext.h (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.h#ne...
src/PyZyPhoneticContext.h:74: gboolean selectCandidateInPage (guint i);
Which did you meen that we replace all of variable types from g* to standard
types or we replace only these ?

I want to replace all of it since users of libpyzy using standard types.

On 2011/10/17 15:02:11, Peng wrote:
> unsigned int

http://codereview.appspot.com/5235062/diff/10016/src/PyZyTest.cc
File src/PyZyTest.cc (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyTest.cc#newcode121
src/PyZyTest.cc:121: EXPECT_EQ (0, observer.commitTextCount ());
I agree with the plan.
But I don't have enough time to implements it soon.
Sorry but give me time, please.

[Peng, 2011-10-18 14:55:29]

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.cc
File src/PyZyPhoneticContext.cc (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.cc#n...
src/PyZyPhoneticContext.cc:198: m_observer->lookupTableChanged ();
On 2011/10/18 09:45:42, hsumita wrote:
> Is this means that we implements lookupTableChanged (and other functions named
> *Changed or commitText) to PyZyInputContext and call it from PhoneticContext
(or
> other context)?
> 
> It may not good because PyZyInputContext.h is used by libpyzy user. I prefer
> that this header doesn't contain any information about an inside of libpyzy.
> 
> Virtually, PhoneticContext is base class of other *Context class (of course
> except for InputContext) and InputContext is merely thin wrapper. How about
that
> we implement notifier on PhoneticContext and other *Context class call it?

I agree it is better to keep PyZy::InputContext as a pure interface, and add
those notify functions PhoneticContext.

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.h
File src/PyZyPhoneticContext.h (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.h#ne...
src/PyZyPhoneticContext.h:74: gboolean selectCandidateInPage (guint i);
On 2011/10/18 09:45:42, hsumita wrote:
> Which did you meen that we replace all of variable types from g* to standard
> types or we replace only these ?
> 
> I want to replace all of it since users of libpyzy using standard types.
> 
> On 2011/10/17 15:02:11, Peng wrote:
> > unsigned int

Yes. Remove all g* from libpyzy, otherwise libpyzy will depend on glib. I think
you plan remove glib dependence, right?

http://codereview.appspot.com/5235062/diff/10016/src/PyZyTest.cc
File src/PyZyTest.cc (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyTest.cc#newcode121
src/PyZyTest.cc:121: EXPECT_EQ (0, observer.commitTextCount ());
On 2011/10/18 09:45:42, hsumita wrote:
> I agree with the plan.
> But I don't have enough time to implements it soon.
> Sorry but give me time, please.

I see. It is okay.

[hsumita, 2011-10-19 06:00:33]

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.cc
File src/PyZyPhoneticContext.cc (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.cc#n...
src/PyZyPhoneticContext.cc:198: m_observer->lookupTableChanged ();
Done.
It is fixed by http://codereview.appspot.com/5312042/

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.h
File src/PyZyPhoneticContext.h (right):

http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.h#ne...
src/PyZyPhoneticContext.h:74: gboolean selectCandidateInPage (guint i);
I think it is ok that libpyzy depend on glib, but it is not ok that a program
which depends on libpyzy should have a dependency on glib. In other words, I
want to write a program using libpyzy without including glib.h.

It increases in cost to removing all of glib dependencies from libpyzy. (There
are many functions like g_ucs4_to_utf8 which provides by glib)

What should we do? I have 2 plans.

- We use standard data types on files which related to PyZyInputContext.h and
PyZyConfig.h, and use g* data types on others. For example
// This function is used by libpyzy users. arguments and return value are
standard data type.
bool PhoneticContext::functionWhichIsUsedByLibpyzyUser (unsigned int foo)
{
  // using g* data type.
  guint bar = otherFunctions (foo);
  // return standard data type.
  return true;
}
// This function is not used by libpyzy user. So we use g* data type.
guint PhoneticContext::otherFunctions (guint foo)
{
  return 1;
}

- We always use standard data type.
void testFunction (void)
{
  const char *foo = "foo";
  // g_utf8_strlen takes const gchar * and returns glong.
  // But we always use standard data type.
  const long length = g_utf8_strlen(foo, -1);
}

What is your opinion?

[Peng, 2011-10-19 14:12:22]

On 2011/10/19 06:00:33, hsumita wrote:
> http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.cc
> File src/PyZyPhoneticContext.cc (right):
> 
>
http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.cc#n...
> src/PyZyPhoneticContext.cc:198: m_observer->lookupTableChanged ();
> Done.
> It is fixed by http://codereview.appspot.com/5312042/
> 
> http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.h
> File src/PyZyPhoneticContext.h (right):
> 
>
http://codereview.appspot.com/5235062/diff/10016/src/PyZyPhoneticContext.h#ne...
> src/PyZyPhoneticContext.h:74: gboolean selectCandidateInPage (guint i);
> I think it is ok that libpyzy depend on glib, but it is not ok that a program
> which depends on libpyzy should have a dependency on glib. In other words, I
> want to write a program using libpyzy without including glib.h.
> 
> It increases in cost to removing all of glib dependencies from libpyzy. (There
> are many functions like g_ucs4_to_utf8 which provides by glib)

I remember mozc team wants to use this library to create an engine for Chrome
OS, Linux, Windows and Mac. right? Glib is included in Chrome OS and Linux. But
for windows and Mac, probably you need do some extra works I guess. Anyway, we
may do it later, if we find it is necessary sometime. It should not be
difficult. I think glib is LGPL, we could copy some code from it.

> 
> What should we do? I have 2 plans.
> 
> - We use standard data types on files which related to PyZyInputContext.h and
> PyZyConfig.h, and use g* data types on others. For example
> // This function is used by libpyzy users. arguments and return value are
> standard data type.
> bool PhoneticContext::functionWhichIsUsedByLibpyzyUser (unsigned int foo)
> {
>   // using g* data type.
>   guint bar = otherFunctions (foo);
>   // return standard data type.
>   return true;
> }
> // This function is not used by libpyzy user. So we use g* data type.
> guint PhoneticContext::otherFunctions (guint foo)
> {
>   return 1;
> }
> 
> - We always use standard data type.
> void testFunction (void)
> {
>   const char *foo = "foo";
>   // g_utf8_strlen takes const gchar * and returns glong.
>   // But we always use standard data type.
>   const long length = g_utf8_strlen(foo, -1);
> }
> 
> What is your opinion?

[hsumita, 2011-10-20 01:47:46]

Yes, it is almost right.
Our first target of Chinese IME is ChromeOS.
If we can implements Chinese IME on Windows / Mac, of course we are very happy.
But it is more important for Mozc team to implementing libpyzy working on
ChromeOS/Linux.