Issue 4200041: add English Editor (Closed)
|
Created:
13 years, 2 months ago by Peng Wu Modified:
13 years ago Reviewers:
shawn.p.huang Base URL:
git@github.com:epico/ibus-pinyin.git@master Visibility:
Public. |
Descriptionadd English Editor
to speed up English input
BUG=none
TEST=build fine
Patch Set 1 #
Total comments: 58
Patch Set 2 : fixes code style #Patch Set 3 : add in memory user database #
Total comments: 16
Patch Set 4 : fixes code styles and let english input mode configurable #
MessagesTotal messages: 11
http://codereview.appspot.com/4200041/diff/1/data/Makefile.am File data/Makefile.am (right): http://codereview.appspot.com/4200041/diff/1/data/Makefile.am#newcode38 data/Makefile.am:38: $(ENGLISH_DB): $(WORDLIST) WORDLIST is not define http://codereview.appspot.com/4200041/diff/1/data/Makefile.am#newcode39 data/Makefile.am:39: gawk -f english.awk $< |sqlite3 $@ This step is too slow. http://codereview.appspot.com/4200041/diff/1/data/english.awk File data/english.awk (right): http://codereview.appspot.com/4200041/diff/1/data/english.awk#newcode3 data/english.awk:3: BEGIN { Add below lines for better performance. # Begin a transaction print "BEGIN TRANSACTION;" http://codereview.appspot.com/4200041/diff/1/data/english.awk#newcode19 data/english.awk:19: print ".exit"; This line is not necessary. Please add: # Commit the transcation print "COMMIT;" http://codereview.appspot.com/4200041/diff/1/src/Makefile.am File src/Makefile.am (right): http://codereview.appspot.com/4200041/diff/1/src/Makefile.am#newcode67 src/Makefile.am:67: PYEngEditor.cc \ Please rename it to PYEnglishEditor.cc http://codereview.appspot.com/4200041/diff/1/src/Makefile.am#newcode109 src/Makefile.am:109: PYEngEditor.h \ Same http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc File src/PYEngEditor.cc (right): http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode5 src/PYEngEditor.cc:5: * Copyright (c) 2010 Peng Wu <alexepico@gmail.com> Please change to 2011 or 2010 - 2011 http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode22 src/PYEngEditor.cc:22: #include <assert.h> Please use g_assert http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode33 src/PYEngEditor.cc:33: #include "PYEngEditor.h" Please mv PYEngEditor.h to the first. And then: system headers And then ibus-pinyin headers http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode35 src/PYEngEditor.cc:35: static const char * SQL_CREATE_DB = It is not necessary to define those const strings. Maybe it is more clear to define them in function. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode45 src/PYEngEditor.cc:45: "SELECT word FROM ( SELECT * FROM english UNION ALL SELECT * FROM user.english) WHERE word LIKE \"%s%%\" GROUP BY word ORDER BY SUM(freq) DESC;"; Can we use ' instead of \"? It is more clear. And this line too long. Please separated it http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode58 src/PYEngEditor.cc:58: const float EnglishEditor::train_factor = 0.1; This value will be accessed out of this file? If not, it is better to remove it from EnglishEditor http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode63 src/PYEngEditor.cc:63: String m_sql; I prefer to put data at the end of class and then: constructors and destructor. public methods protected methods private methods private data http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode88 src/PYEngEditor.cc:88: bool isDatabaseExisted(const char * filename) { Make it private method I used gboolean instead of bool in whole project. So it is better to use same type, and return TRUE or FALSE http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode96 src/PYEngEditor.cc:96: sqlite3_close(tmp_db); Is it OK to close the db here? http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode107 src/PYEngEditor.cc:107: if ( result != SQLITE_ROW) Please remove space after '(' And others in file. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode112 src/PYEngEditor.cc:112: const char * version = (const char *) sqlite3_column_text(stmt, 0); Add space before '('. and fix other lines in file function call style: ret = func (arg1, arg2); http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode113 src/PYEngEditor.cc:113: if ( strcmp("1.2.0", version ) != 0) Remove space before ')' http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode138 src/PYEngEditor.cc:138: sqlite3_close(tmp_db); open failed, should close it? http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode194 src/PYEngEditor.cc:194: assert(result == SQLITE_OK); g_assert http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode233 src/PYEngEditor.cc:233: return executeSQL(m_sqlite); It is not good to write the user db every time. Please refer PYDatabase.cc. Database::loadUserDB (void) and Database::saveUserDB (void) http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode300 src/PYEngEditor.cc:300: switch (m_cursor) { Please use if to make it more readable. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode660 src/PYEngEditor.cc:660: #if 0 If it is not useful now, please remove it. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h File src/PYEngEditor.h (right): http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h#newcode5 src/PYEngEditor.h:5: * Copyright (c) 2010 Peng Wu <alexepico@gmail.com> Same http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h#newcode27 src/PYEngEditor.h:27: typedef struct sqlite3 sqlite3; I think you may remove it. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h#newcode78 src/PYEngEditor.h:78: static const float train_factor; Is it necessary to define a const static member? http://codereview.appspot.com/4200041/diff/1/src/PYExtEditor.cc File src/PYExtEditor.cc (right): http://codereview.appspot.com/4200041/diff/1/src/PYExtEditor.cc#newcode285 src/PYExtEditor.cc:285: if ( keyval != IBUS_Return ) Please refine the code style. if (a != b)
Sign in to reply to this message.
On 2011/02/23 15:59:25, Peng wrote: > http://codereview.appspot.com/4200041/diff/1/data/Makefile.am > File data/Makefile.am (right): > > http://codereview.appspot.com/4200041/diff/1/data/Makefile.am#newcode38 > data/Makefile.am:38: $(ENGLISH_DB): $(WORDLIST) > WORDLIST is not define > > Fixed. http://codereview.appspot.com/4200041/diff/1/data/Makefile.am#newcode39 > data/Makefile.am:39: gawk -f english.awk $< |sqlite3 $@ > This step is too slow. > Fixed. > http://codereview.appspot.com/4200041/diff/1/data/english.awk > File data/english.awk (right): > > http://codereview.appspot.com/4200041/diff/1/data/english.awk#newcode3 > data/english.awk:3: BEGIN { > Add below lines for better performance. > # Begin a transaction > print "BEGIN TRANSACTION;" > > Fixed. http://codereview.appspot.com/4200041/diff/1/data/english.awk#newcode19 > data/english.awk:19: print ".exit"; > This line is not necessary. > Please add: > # Commit the transcation > print "COMMIT;" > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/Makefile.am > File src/Makefile.am (right): > > http://codereview.appspot.com/4200041/diff/1/src/Makefile.am#newcode67 > src/Makefile.am:67: PYEngEditor.cc \ > Please rename it to PYEnglishEditor.cc > > Fixed. http://codereview.appspot.com/4200041/diff/1/src/Makefile.am#newcode109 > src/Makefile.am:109: PYEngEditor.h \ > Same > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc > File src/PYEngEditor.cc (right): > > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode5 > src/PYEngEditor.cc:5: * Copyright (c) 2010 Peng Wu <mailto:alexepico@gmail.com> > Please change to 2011 or 2010 - 2011 > > Fixed. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode22 > src/PYEngEditor.cc:22: #include <assert.h> > Please use g_assert > > Fixed. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode33 > src/PYEngEditor.cc:33: #include "PYEngEditor.h" > Please mv PYEngEditor.h to the first. > And then: > system headers > And then > ibus-pinyin headers > > Fixed. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode35 > src/PYEngEditor.cc:35: static const char * SQL_CREATE_DB = > It is not necessary to define those const strings. > Maybe it is more clear to define them in function. > > I just put the sql statements together to easily check the sql statements. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode45 > src/PYEngEditor.cc:45: "SELECT word FROM ( SELECT * FROM english UNION ALL > SELECT * FROM user.english) WHERE word LIKE \"%s%%\" GROUP BY word ORDER BY > SUM(freq) DESC;"; > Can we use ' instead of \"? It is more clear. > > And this line too long. Please separated it > > Fixed. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode58 > src/PYEngEditor.cc:58: const float EnglishEditor::train_factor = 0.1; > This value will be accessed out of this file? > If not, it is better to remove it from EnglishEditor > > Maybe it will be accessed later. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode63 > src/PYEngEditor.cc:63: String m_sql; > I prefer to put data at the end of class > and then: > constructors and destructor. > > public methods > protected methods > private methods > private data > > Fixed. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode88 > src/PYEngEditor.cc:88: bool isDatabaseExisted(const char * filename) { > Make it private method > This method maybe used later, and is part of interface. > I used gboolean instead of bool in whole project. So it is better to use same > type, and return TRUE or FALSE > > Fixed. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode96 > src/PYEngEditor.cc:96: sqlite3_close(tmp_db); > Is it OK to close the db here? > > Yes, as it is a tmp_db, not m_sqlite. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode107 > src/PYEngEditor.cc:107: if ( result != SQLITE_ROW) > Please remove space after '(' > And others in file. > > Fixed. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode112 > src/PYEngEditor.cc:112: const char * version = (const char *) > sqlite3_column_text(stmt, 0); > Add space before '('. and fix other lines in file > > function call style: > > ret = func (arg1, arg2); > > Fixed. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode113 > src/PYEngEditor.cc:113: if ( strcmp("1.2.0", version ) != 0) > Remove space before ')' > > Fixed. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode138 > src/PYEngEditor.cc:138: sqlite3_close(tmp_db); > open failed, should close it? > > Yes, according to the sqlite document. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode194 > src/PYEngEditor.cc:194: assert(result == SQLITE_OK); > g_assert > > Fixed. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode233 > src/PYEngEditor.cc:233: return executeSQL(m_sqlite); > It is not good to write the user db every time. Please refer PYDatabase.cc. > > Database::loadUserDB (void) > and > Database::saveUserDB (void) > > This English input mode is not used very frequently by users, and only update once after inputing a whole word. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode300 > src/PYEngEditor.cc:300: switch (m_cursor) { > Please use if to make it more readable. > > Referred to similar code in PYExtEditor. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode660 > src/PYEngEditor.cc:660: #if 0 > If it is not useful now, please remove it. > Test code to check English Database. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h > File src/PYEngEditor.h (right): > > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h#newcode5 > src/PYEngEditor.h:5: * Copyright (c) 2010 Peng Wu <mailto:alexepico@gmail.com> > Same > > Fixed. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h#newcode27 > src/PYEngEditor.h:27: typedef struct sqlite3 sqlite3; > I think you may remove it. > > Fixed. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h#newcode78 > src/PYEngEditor.h:78: static const float train_factor; > Is it necessary to define a const static member? > maybe accessed outside of English Editor. > http://codereview.appspot.com/4200041/diff/1/src/PYExtEditor.cc > File src/PYExtEditor.cc (right): > > http://codereview.appspot.com/4200041/diff/1/src/PYExtEditor.cc#newcode285 > src/PYExtEditor.cc:285: if ( keyval != IBUS_Return ) > Please refine the code style. > > if (a != b) Fixed.
Sign in to reply to this message.
Please reply each comment by click 'reply' or 'Done' link in code review tools, so that you reply will be show in the code. It will be more convenient for discussing. 'Done' means fixed. On 2011/03/17 03:25:43, alexepico wrote: > On 2011/02/23 15:59:25, Peng wrote: > > http://codereview.appspot.com/4200041/diff/1/data/Makefile.am > > File data/Makefile.am (right): > > > > http://codereview.appspot.com/4200041/diff/1/data/Makefile.am#newcode38 > > data/Makefile.am:38: $(ENGLISH_DB): $(WORDLIST) > > WORDLIST is not define > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/data/Makefile.am#newcode39 > > data/Makefile.am:39: gawk -f english.awk $< |sqlite3 $@ > > This step is too slow. > > > Fixed. > > http://codereview.appspot.com/4200041/diff/1/data/english.awk > > File data/english.awk (right): > > > > http://codereview.appspot.com/4200041/diff/1/data/english.awk#newcode3 > > data/english.awk:3: BEGIN { > > Add below lines for better performance. > > # Begin a transaction > > print "BEGIN TRANSACTION;" > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/data/english.awk#newcode19 > > data/english.awk:19: print ".exit"; > > This line is not necessary. > > Please add: > > # Commit the transcation > > print "COMMIT;" > > > Fixed. > > http://codereview.appspot.com/4200041/diff/1/src/Makefile.am > > File src/Makefile.am (right): > > > > http://codereview.appspot.com/4200041/diff/1/src/Makefile.am#newcode67 > > src/Makefile.am:67: PYEngEditor.cc \ > > Please rename it to PYEnglishEditor.cc > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/Makefile.am#newcode109 > > src/Makefile.am:109: PYEngEditor.h \ > > Same > > > Fixed. > > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc > > File src/PYEngEditor.cc (right): > > > > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode5 > > src/PYEngEditor.cc:5: * Copyright (c) 2010 Peng Wu > <mailto:alexepico@gmail.com> > > Please change to 2011 or 2010 - 2011 > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode22 > > src/PYEngEditor.cc:22: #include <assert.h> > > Please use g_assert > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode33 > > src/PYEngEditor.cc:33: #include "PYEngEditor.h" > > Please mv PYEngEditor.h to the first. > > And then: > > system headers > > And then > > ibus-pinyin headers > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode35 > > src/PYEngEditor.cc:35: static const char * SQL_CREATE_DB = > > It is not necessary to define those const strings. > > Maybe it is more clear to define them in function. > > > > > I just put the sql statements together to easily check the sql statements. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode45 > > src/PYEngEditor.cc:45: "SELECT word FROM ( SELECT * FROM english UNION ALL > > SELECT * FROM user.english) WHERE word LIKE \"%s%%\" GROUP BY word ORDER BY > > SUM(freq) DESC;"; > > Can we use ' instead of \"? It is more clear. > > > > And this line too long. Please separated it > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode58 > > src/PYEngEditor.cc:58: const float EnglishEditor::train_factor = 0.1; > > This value will be accessed out of this file? > > If not, it is better to remove it from EnglishEditor > > > > > Maybe it will be accessed later. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode63 > > src/PYEngEditor.cc:63: String m_sql; > > I prefer to put data at the end of class > > and then: > > constructors and destructor. > > > > public methods > > protected methods > > private methods > > private data > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode88 > > src/PYEngEditor.cc:88: bool isDatabaseExisted(const char * filename) { > > Make it private method > > > This method maybe used later, and is part of interface. > > I used gboolean instead of bool in whole project. So it is better to use same > > type, and return TRUE or FALSE > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode96 > > src/PYEngEditor.cc:96: sqlite3_close(tmp_db); > > Is it OK to close the db here? > > > > > Yes, as it is a tmp_db, not m_sqlite. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode107 > > src/PYEngEditor.cc:107: if ( result != SQLITE_ROW) > > Please remove space after '(' > > And others in file. > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode112 > > src/PYEngEditor.cc:112: const char * version = (const char *) > > sqlite3_column_text(stmt, 0); > > Add space before '('. and fix other lines in file > > > > function call style: > > > > ret = func (arg1, arg2); > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode113 > > src/PYEngEditor.cc:113: if ( strcmp("1.2.0", version ) != 0) > > Remove space before ')' > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode138 > > src/PYEngEditor.cc:138: sqlite3_close(tmp_db); > > open failed, should close it? > > > > > Yes, according to the sqlite document. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode194 > > src/PYEngEditor.cc:194: assert(result == SQLITE_OK); > > g_assert > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode233 > > src/PYEngEditor.cc:233: return executeSQL(m_sqlite); > > It is not good to write the user db every time. Please refer PYDatabase.cc. > > > > Database::loadUserDB (void) > > and > > Database::saveUserDB (void) > > > > > This English input mode is not used very frequently by users, and only > update once after inputing a whole word. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode300 > > src/PYEngEditor.cc:300: switch (m_cursor) { > > Please use if to make it more readable. > > > > > Referred to similar code in PYExtEditor. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode660 > > src/PYEngEditor.cc:660: #if 0 > > If it is not useful now, please remove it. > > > Test code to check English Database. > > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h > > File src/PYEngEditor.h (right): > > > > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h#newcode5 > > src/PYEngEditor.h:5: * Copyright (c) 2010 Peng Wu <mailto:alexepico@gmail.com> > > Same > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h#newcode27 > > src/PYEngEditor.h:27: typedef struct sqlite3 sqlite3; > > I think you may remove it. > > > > > Fixed. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h#newcode78 > > src/PYEngEditor.h:78: static const float train_factor; > > Is it necessary to define a const static member? > > > maybe accessed outside of English Editor. > > http://codereview.appspot.com/4200041/diff/1/src/PYExtEditor.cc > > File src/PYExtEditor.cc (right): > > > > http://codereview.appspot.com/4200041/diff/1/src/PYExtEditor.cc#newcode285 > > src/PYExtEditor.cc:285: if ( keyval != IBUS_Return ) > > Please refine the code style. > > > > if (a != b) > Fixed.
Sign in to reply to this message.
Hi, Here are the new comments, please review it. Thanks. http://codereview.appspot.com/4200041/diff/1/data/Makefile.am File data/Makefile.am (right): http://codereview.appspot.com/4200041/diff/1/data/Makefile.am#newcode38 data/Makefile.am:38: $(ENGLISH_DB): $(WORDLIST) On 2011/02/23 15:59:26, Peng wrote: > WORDLIST is not define Done. http://codereview.appspot.com/4200041/diff/1/data/Makefile.am#newcode39 data/Makefile.am:39: gawk -f english.awk $< |sqlite3 $@ On 2011/02/23 15:59:26, Peng wrote: > This step is too slow. Done. http://codereview.appspot.com/4200041/diff/1/data/english.awk File data/english.awk (right): http://codereview.appspot.com/4200041/diff/1/data/english.awk#newcode3 data/english.awk:3: BEGIN { On 2011/02/23 15:59:26, Peng wrote: > Add below lines for better performance. > # Begin a transaction > print "BEGIN TRANSACTION;" > Done. http://codereview.appspot.com/4200041/diff/1/data/english.awk#newcode19 data/english.awk:19: print ".exit"; On 2011/02/23 15:59:26, Peng wrote: > This line is not necessary. > Please add: > # Commit the transcation > print "COMMIT;" Done. http://codereview.appspot.com/4200041/diff/1/src/Makefile.am File src/Makefile.am (right): http://codereview.appspot.com/4200041/diff/1/src/Makefile.am#newcode67 src/Makefile.am:67: PYEngEditor.cc \ On 2011/02/23 15:59:26, Peng wrote: > Please rename it to PYEnglishEditor.cc Done. http://codereview.appspot.com/4200041/diff/1/src/Makefile.am#newcode109 src/Makefile.am:109: PYEngEditor.h \ On 2011/02/23 15:59:26, Peng wrote: > Same Done. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc File src/PYEngEditor.cc (right): http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode5 src/PYEngEditor.cc:5: * Copyright (c) 2010 Peng Wu <alexepico@gmail.com> On 2011/02/23 15:59:26, Peng wrote: > Please change to 2011 or 2010 - 2011 Done. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode22 src/PYEngEditor.cc:22: #include <assert.h> On 2011/02/23 15:59:26, Peng wrote: > Please use g_assert Done. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode33 src/PYEngEditor.cc:33: #include "PYEngEditor.h" On 2011/02/23 15:59:26, Peng wrote: > Please mv PYEngEditor.h to the first. > And then: > system headers > And then > ibus-pinyin headers Done. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode35 src/PYEngEditor.cc:35: static const char * SQL_CREATE_DB = On 2011/02/23 15:59:26, Peng wrote: > It is not necessary to define those const strings. > Maybe it is more clear to define them in function. I just put the sql statements together to easily check the sql statements. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode45 src/PYEngEditor.cc:45: "SELECT word FROM ( SELECT * FROM english UNION ALL SELECT * FROM user.english) WHERE word LIKE \"%s%%\" GROUP BY word ORDER BY SUM(freq) DESC;"; On 2011/02/23 15:59:26, Peng wrote: > Can we use ' instead of \"? It is more clear. > > And this line too long. Please separated it Done. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode58 src/PYEngEditor.cc:58: const float EnglishEditor::train_factor = 0.1; On 2011/02/23 15:59:26, Peng wrote: > This value will be accessed out of this file? > If not, it is better to remove it from EnglishEditor Maybe it will be accessed later. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode63 src/PYEngEditor.cc:63: String m_sql; On 2011/02/23 15:59:26, Peng wrote: > I prefer to put data at the end of class > and then: > constructors and destructor. > > public methods > protected methods > private methods > private data Done. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode88 src/PYEngEditor.cc:88: bool isDatabaseExisted(const char * filename) { On 2011/02/23 15:59:26, Peng wrote: > Make it private method > This method may be used later, and is part of interface. > I used gboolean instead of bool in whole project. So it is better to use same > type, and return TRUE or FALSE Done. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode96 src/PYEngEditor.cc:96: sqlite3_close(tmp_db); On 2011/02/23 15:59:26, Peng wrote: > Is it OK to close the db here? Yes, as it is a tmp_db, not m_sqlite. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode107 src/PYEngEditor.cc:107: if ( result != SQLITE_ROW) On 2011/02/23 15:59:26, Peng wrote: > Please remove space after '(' > And others in file. Done. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode112 src/PYEngEditor.cc:112: const char * version = (const char *) sqlite3_column_text(stmt, 0); On 2011/02/23 15:59:26, Peng wrote: > Add space before '('. and fix other lines in file > > function call style: > > ret = func (arg1, arg2); Done. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode113 src/PYEngEditor.cc:113: if ( strcmp("1.2.0", version ) != 0) On 2011/02/23 15:59:26, Peng wrote: > Remove space before ')' Done. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode138 src/PYEngEditor.cc:138: sqlite3_close(tmp_db); On 2011/02/23 15:59:26, Peng wrote: > open failed, should close it? Yes, according to the sqlite document. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode194 src/PYEngEditor.cc:194: assert(result == SQLITE_OK); On 2011/02/23 15:59:26, Peng wrote: > g_assert Done. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode233 src/PYEngEditor.cc:233: return executeSQL(m_sqlite); On 2011/02/23 15:59:26, Peng wrote: > It is not good to write the user db every time. Please refer PYDatabase.cc. > > Database::loadUserDB (void) > and > Database::saveUserDB (void) This English input mode is not used frequently by users, and only update once after inputing a whole word. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode300 src/PYEngEditor.cc:300: switch (m_cursor) { On 2011/02/23 15:59:26, Peng wrote: > Please use if to make it more readable. Referred to similar code in PYExtEditor. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode660 src/PYEngEditor.cc:660: #if 0 On 2011/02/23 15:59:26, Peng wrote: > If it is not useful now, please remove it. Test code to check English Database. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h File src/PYEngEditor.h (right): http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h#newcode5 src/PYEngEditor.h:5: * Copyright (c) 2010 Peng Wu <alexepico@gmail.com> On 2011/02/23 15:59:26, Peng wrote: > Same Done. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h#newcode27 src/PYEngEditor.h:27: typedef struct sqlite3 sqlite3; On 2011/02/23 15:59:26, Peng wrote: > I think you may remove it. Done. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h#newcode78 src/PYEngEditor.h:78: static const float train_factor; On 2011/02/23 15:59:26, Peng wrote: > Is it necessary to define a const static member? maybe accessed outside of English Editor. http://codereview.appspot.com/4200041/diff/1/src/PYExtEditor.cc File src/PYExtEditor.cc (right): http://codereview.appspot.com/4200041/diff/1/src/PYExtEditor.cc#newcode285 src/PYExtEditor.cc:285: if ( keyval != IBUS_Return ) On 2011/02/23 15:59:26, Peng wrote: > Please refine the code style. > > if (a != b) Done.
Sign in to reply to this message.
http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc File src/PYEngEditor.cc (right): http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode35 src/PYEngEditor.cc:35: static const char * SQL_CREATE_DB = On 2011/03/18 02:37:07, alexepico wrote: > On 2011/02/23 15:59:26, Peng wrote: > > It is not necessary to define those const strings. > > Maybe it is more clear to define them in function. > > I just put the sql statements together to easily check the sql statements. I think embed those SQL stmts in code may be more readable. Especially, when I read the code, it is easy to check what the SQL stmt is. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode138 src/PYEngEditor.cc:138: sqlite3_close(tmp_db); On 2011/03/18 02:37:07, alexepico wrote: > On 2011/02/23 15:59:26, Peng wrote: > > open failed, should close it? > > Yes, according to the sqlite document. Really. I think it is not reasonable. Please double check this. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode233 src/PYEngEditor.cc:233: return executeSQL(m_sqlite); On 2011/03/18 02:37:07, alexepico wrote: > On 2011/02/23 15:59:26, Peng wrote: > > It is not good to write the user db every time. Please refer PYDatabase.cc. > > > > Database::loadUserDB (void) > > and > > Database::saveUserDB (void) > > This English input mode is not used frequently by users, and only update once > after inputing a whole word. If it is that case, I suggest removing use user db. http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h File src/PYEngEditor.h (right): http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h#newcode78 src/PYEngEditor.h:78: static const float train_factor; On 2011/03/18 02:37:07, alexepico wrote: > On 2011/02/23 15:59:26, Peng wrote: > > Is it necessary to define a const static member? > > maybe accessed outside of English Editor. Why need it out side of EnglishEditor?
Sign in to reply to this message.
On 2011/03/19 04:19:26, Peng wrote: > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc > File src/PYEngEditor.cc (right): > > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode35 > src/PYEngEditor.cc:35: static const char * SQL_CREATE_DB = > On 2011/03/18 02:37:07, alexepico wrote: > > On 2011/02/23 15:59:26, Peng wrote: > > > It is not necessary to define those const strings. > > > Maybe it is more clear to define them in function. > > > > I just put the sql statements together to easily check the sql statements. > > I think embed those SQL stmts in code may be more readable. > Especially, when I read the code, it is easy to check what the SQL stmt is. > In the past, I used this to do cross check sql statements. As ibus-pinyin uses different code style, put sql statements in function body now. Done. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode138 > src/PYEngEditor.cc:138: sqlite3_close(tmp_db); > On 2011/03/18 02:37:07, alexepico wrote: > > On 2011/02/23 15:59:26, Peng wrote: > > > open failed, should close it? > > > > Yes, according to the sqlite document. > > Really. I think it is not reasonable. Please double check this. > From sqlite documentation: Whether or not an error occurs when it is opened, resources associated with the database connection handle should be released by passing it to sqlite3_close() when it is no longer required. > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.cc#newcode233 > src/PYEngEditor.cc:233: return executeSQL(m_sqlite); > On 2011/03/18 02:37:07, alexepico wrote: > > On 2011/02/23 15:59:26, Peng wrote: > > > It is not good to write the user db every time. Please refer PYDatabase.cc. > > > > > > Database::loadUserDB (void) > > > and > > > Database::saveUserDB (void) > > > > This English input mode is not used frequently by users, and only update once > > after inputing a whole word. > > If it is that case, I suggest removing use user db. loadUserDB/saveUserDB is added. Done. > > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h > File src/PYEngEditor.h (right): > > http://codereview.appspot.com/4200041/diff/1/src/PYEngEditor.h#newcode78 > src/PYEngEditor.h:78: static const float train_factor; > On 2011/03/18 02:37:07, alexepico wrote: > > On 2011/02/23 15:59:26, Peng wrote: > > > Is it necessary to define a const static member? > > > > maybe accessed outside of English Editor. > > Why need it out side of EnglishEditor? I changed it to a private member variable, maybe modifiable in the future. Done.
Sign in to reply to this message.
http://codereview.appspot.com/4200041/diff/17004/data/Makefile.am File data/Makefile.am (right): http://codereview.appspot.com/4200041/diff/17004/data/Makefile.am#newcode42 data/Makefile.am:42: gawk -f english.awk $< |sqlite3 $@ Use $(AWK) http://codereview.appspot.com/4200041/diff/17004/src/Makefile.am File src/Makefile.am (right): http://codereview.appspot.com/4200041/diff/17004/src/Makefile.am#newcode67 src/Makefile.am:67: PYEnglishEditor.cc \ Could you make it configurable? like lua extension. Thanks. http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc File src/PYEnglishEditor.cc (right): http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... src/PYEnglishEditor.cc:68: sqlite3 * tmp_db = NULL; Please remove space between * and tmp_db. http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... src/PYEnglishEditor.cc:71: sqlite3_close (tmp_db); I checked the source code of sqlit3. If open is failed, you should not call close. Whether or not an error occurs when it is opened, resources associated with the database connection handle should be released by passing it to sqlite3_close() when it is no longer required. I think the API document means when a database is opened already, you should close it, event if some error happens. http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... src/PYEnglishEditor.cc:76: sqlite3_stmt * stmt = NULL; same http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... src/PYEnglishEditor.cc:129: const char * SQL_CREATE_DB = It is not necessary to use a const char * here. Please m_sql = "" directly. http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... src/PYEnglishEditor.cc:135: if (!executeSQL (tmp_db)) { tmp_db? http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... src/PYEnglishEditor.cc:335: EnglishDatabase * self = static_cast<EnglishDatabase *> (data); Please remove space between * and self. Please also fix same style issues in source code too.
Sign in to reply to this message.
Please review it again. http://codereview.appspot.com/4200041/diff/17004/data/Makefile.am File data/Makefile.am (right): http://codereview.appspot.com/4200041/diff/17004/data/Makefile.am#newcode42 data/Makefile.am:42: gawk -f english.awk $< |sqlite3 $@ On 2011/04/19 14:31:06, Peng wrote: > Use $(AWK) Done. http://codereview.appspot.com/4200041/diff/17004/src/Makefile.am File src/Makefile.am (right): http://codereview.appspot.com/4200041/diff/17004/src/Makefile.am#newcode67 src/Makefile.am:67: PYEnglishEditor.cc \ On 2011/04/19 14:31:06, Peng wrote: > Could you make it configurable? like lua extension. Thanks. Done. http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc File src/PYEnglishEditor.cc (right): http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... src/PYEnglishEditor.cc:68: sqlite3 * tmp_db = NULL; On 2011/04/19 14:31:06, Peng wrote: > Please remove space between * and tmp_db. Done. http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... src/PYEnglishEditor.cc:71: sqlite3_close (tmp_db); On 2011/04/19 14:31:06, Peng wrote: > I checked the source code of sqlit3. If open is failed, you should not call > close. > > Whether or not an error occurs when it is opened, resources associated with the > database connection handle should be released by passing it to sqlite3_close() > when it is no longer required. > > I think the API document means when a database is opened already, you should > close it, event if some error happens. > Done. http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... src/PYEnglishEditor.cc:76: sqlite3_stmt * stmt = NULL; On 2011/04/19 14:31:06, Peng wrote: > same Done. http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... src/PYEnglishEditor.cc:129: const char * SQL_CREATE_DB = On 2011/04/19 14:31:06, Peng wrote: > It is not necessary to use a const char * here. > Please m_sql = "" directly. Done. http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... src/PYEnglishEditor.cc:135: if (!executeSQL (tmp_db)) { On 2011/04/19 14:31:06, Peng wrote: > tmp_db? Yes. In createDatabase, it doesn't modify m_sqlite, just create a database on disk. http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... src/PYEnglishEditor.cc:335: EnglishDatabase * self = static_cast<EnglishDatabase *> (data); On 2011/04/19 14:31:06, Peng wrote: > Please remove space between * and self. > > Please also fix same style issues in source code too. Done.
Sign in to reply to this message.
Seems the configure.ac was not loaded. Please upload it again. Thanks. On 2011/04/22 05:18:21, alexepico wrote: > Please review it again. > > http://codereview.appspot.com/4200041/diff/17004/data/Makefile.am > File data/Makefile.am (right): > > http://codereview.appspot.com/4200041/diff/17004/data/Makefile.am#newcode42 > data/Makefile.am:42: gawk -f english.awk $< |sqlite3 $@ > On 2011/04/19 14:31:06, Peng wrote: > > Use $(AWK) > > Done. > > http://codereview.appspot.com/4200041/diff/17004/src/Makefile.am > File src/Makefile.am (right): > > http://codereview.appspot.com/4200041/diff/17004/src/Makefile.am#newcode67 > src/Makefile.am:67: PYEnglishEditor.cc \ > On 2011/04/19 14:31:06, Peng wrote: > > Could you make it configurable? like lua extension. Thanks. > > Done. > > http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc > File src/PYEnglishEditor.cc (right): > > http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... > src/PYEnglishEditor.cc:68: sqlite3 * tmp_db = NULL; > On 2011/04/19 14:31:06, Peng wrote: > > Please remove space between * and tmp_db. > > Done. > > http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... > src/PYEnglishEditor.cc:71: sqlite3_close (tmp_db); > On 2011/04/19 14:31:06, Peng wrote: > > I checked the source code of sqlit3. If open is failed, you should not call > > close. > > > > Whether or not an error occurs when it is opened, resources associated with > the > > database connection handle should be released by passing it to sqlite3_close() > > when it is no longer required. > > > > I think the API document means when a database is opened already, you should > > close it, event if some error happens. > > > > Done. > > http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... > src/PYEnglishEditor.cc:76: sqlite3_stmt * stmt = NULL; > On 2011/04/19 14:31:06, Peng wrote: > > same > > Done. > > http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... > src/PYEnglishEditor.cc:129: const char * SQL_CREATE_DB = > On 2011/04/19 14:31:06, Peng wrote: > > It is not necessary to use a const char * here. > > Please m_sql = "" directly. > > Done. > > http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... > src/PYEnglishEditor.cc:135: if (!executeSQL (tmp_db)) { > On 2011/04/19 14:31:06, Peng wrote: > > tmp_db? > > Yes. In createDatabase, it doesn't modify m_sqlite, just create a database on > disk. > > http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... > src/PYEnglishEditor.cc:335: EnglishDatabase * self = static_cast<EnglishDatabase > *> (data); > On 2011/04/19 14:31:06, Peng wrote: > > Please remove space between * and self. > > > > Please also fix same style issues in source code too. > > Done.
Sign in to reply to this message.
LGTM. I fixed several style issues. and committed it. Please close this CL. Thanks. On 2011/04/22 11:15:47, Peng wrote: > Seems the configure.ac was not loaded. Please upload it again. > Thanks. > > On 2011/04/22 05:18:21, alexepico wrote: > > Please review it again. > > > > http://codereview.appspot.com/4200041/diff/17004/data/Makefile.am > > File data/Makefile.am (right): > > > > http://codereview.appspot.com/4200041/diff/17004/data/Makefile.am#newcode42 > > data/Makefile.am:42: gawk -f english.awk $< |sqlite3 $@ > > On 2011/04/19 14:31:06, Peng wrote: > > > Use $(AWK) > > > > Done. > > > > http://codereview.appspot.com/4200041/diff/17004/src/Makefile.am > > File src/Makefile.am (right): > > > > http://codereview.appspot.com/4200041/diff/17004/src/Makefile.am#newcode67 > > src/Makefile.am:67: PYEnglishEditor.cc \ > > On 2011/04/19 14:31:06, Peng wrote: > > > Could you make it configurable? like lua extension. Thanks. > > > > Done. > > > > http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc > > File src/PYEnglishEditor.cc (right): > > > > > http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... > > src/PYEnglishEditor.cc:68: sqlite3 * tmp_db = NULL; > > On 2011/04/19 14:31:06, Peng wrote: > > > Please remove space between * and tmp_db. > > > > Done. > > > > > http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... > > src/PYEnglishEditor.cc:71: sqlite3_close (tmp_db); > > On 2011/04/19 14:31:06, Peng wrote: > > > I checked the source code of sqlit3. If open is failed, you should not call > > > close. > > > > > > Whether or not an error occurs when it is opened, resources associated with > > the > > > database connection handle should be released by passing it to > sqlite3_close() > > > when it is no longer required. > > > > > > I think the API document means when a database is opened already, you should > > > close it, event if some error happens. > > > > > > > Done. > > > > > http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... > > src/PYEnglishEditor.cc:76: sqlite3_stmt * stmt = NULL; > > On 2011/04/19 14:31:06, Peng wrote: > > > same > > > > Done. > > > > > http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... > > src/PYEnglishEditor.cc:129: const char * SQL_CREATE_DB = > > On 2011/04/19 14:31:06, Peng wrote: > > > It is not necessary to use a const char * here. > > > Please m_sql = "" directly. > > > > Done. > > > > > http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... > > src/PYEnglishEditor.cc:135: if (!executeSQL (tmp_db)) { > > On 2011/04/19 14:31:06, Peng wrote: > > > tmp_db? > > > > Yes. In createDatabase, it doesn't modify m_sqlite, just create a database on > > disk. > > > > > http://codereview.appspot.com/4200041/diff/17004/src/PYEnglishEditor.cc#newco... > > src/PYEnglishEditor.cc:335: EnglishDatabase * self = > static_cast<EnglishDatabase > > *> (data); > > On 2011/04/19 14:31:06, Peng wrote: > > > Please remove space between * and self. > > > > > > Please also fix same style issues in source code too. > > > > Done.
Sign in to reply to this message.
On 2011/04/22 12:22:41, Peng wrote: > LGTM. > > I fixed several style issues. and committed it. Please close this CL. Thanks. > Thanks for reviewing.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
