Refactors Config to manages global/local config.

Refactors Config.


BUG=None
TEST=Manual

[hsumita1, 2012-05-09 06:19:41]

Hi Peng

I'm refactoring Config class of pyzy to integrate it into ibus-pinyin now, and I
want to your opinion how we implements it.

On ibus-pinyin, "mode_simp" is managed by PYPinyinProperties and others are
managed by PYConfig, so "mode_simp" is stored per window(local config) and
others are managed per process (global config).

On current implementation of pyzy, all of these config are global. It is OK on
ChromeOS, but not good on ibus-pinyin on Linux. To manage some config locally, I
have some ideas.

1. Creates new class to manage local config like ibus-pinyin
  pros) We can integrates pyzy into ibus-mozc naturally.
  cons) Developers (pyzy users) can't decide the scope of each config. (global
or local)
        Developers should handles 2 config class.
2. Make all config local.
  pros) Simple.
  cons) Developer should sync global config by own if needed.
3. Extend Config class to manages local and global config like
  pros) Developers can decide the scope of each config without managing global
config by own.
  cons) We should defines a lots of methods. (3 methods per config) 
        It may a little uncomfortable to access global variables through a
instance.

Which do you think better? or do you have another smart ideas?
Tentatively I implements a 3rd idea.

[Peng, 2012-05-09 15:16:37]

Local and global config (properties) are both OK for me. But the point is some
mode changes are temporarily. It is not necessary to save them in storage.

That's why I use two classes. One is for temp changes (properties which are
changed during inputting by hot keys or menu items); one is not for temp changes
(config which are changed by setup UI). And I prefer two classes solution.

https://codereview.appspot.com/6202056/diff/3003/src/PyZyConfig.h
File src/PyZyConfig.h (right):

https://codereview.appspot.com/6202056/diff/3003/src/PyZyConfig.h#newcode72
src/PyZyConfig.h:72: #define DOUBLE_PINYIN_KEYBOARD_LAST   (6)
why change it to 6? The last should be the last item's value?

[hsumita1, 2012-05-10 03:16:09]

On 2012/05/09 15:16:37, Peng wrote:
> Local and global config (properties) are both OK for me. But the point is some
> mode changes are temporarily. It is not necessary to save them in storage.
> 
Thank you for your oppinion!

I assumed that pyzy does not consider the life of config, and developers take a
responsibility about it. (Stores these on storage, or only on memory per config
item). Of course it is OK that ibus-pinyin has two classes. (Config and
PinyinProperties)

I concern that developer can't change the scope of config (local or global) if
we choice the two classes solution on pyzy. Perhaps there are developers who
want to make mode_simp global.

Any way, I will implements the two way solution. Thanks!

[Peng, 2012-05-10 14:06:45]

On 2012/05/10 03:16:09, hsumita1 wrote:
> On 2012/05/09 15:16:37, Peng wrote:
> > Local and global config (properties) are both OK for me. But the point is
some
> > mode changes are temporarily. It is not necessary to save them in storage.
> > 
> Thank you for your oppinion!
> 
> I assumed that pyzy does not consider the life of config, and developers take
a
> responsibility about it. (Stores these on storage, or only on memory per
config
> item). Of course it is OK that ibus-pinyin has two classes. (Config and
> PinyinProperties)
> 
> I concern that developer can't change the scope of config (local or global) if
> we choice the two classes solution on pyzy. Perhaps there are developers who
> want to make mode_simp global.
> 
> Any way, I will implements the two way solution. Thanks!

I sorry. So the PyZyConfig is different with Config in ibus-pinyin. In
ibus-pinyin, the major feature is loading settings from storage.

I am sorry. I forgot the PyZy does not load settings. In that case, I think the
Config is not necessary at all in PyZy. Each InputContext should has their own
properties which control input behavior. The pyzy library users are responsible
for loading settings and feeding them into each InputContexts. So users can
decide which setting is global or just one input context.

For example:
InputContext.{set,get}_option_1(..);
InputContext.{set,get}_option_2(..);

or 

InputContex.set_option(option_name, value) and ..
InputContex.get_option(option_name)

[hsumita1, 2012-05-11 07:46:22]

If we adopt this solution, I think we needs ConfigManager class like following
on ibus-mozc-pinyin and ibus-pinyin to implement global config.

-----------------
class ContextManager {
  static PyZy::Context createContext(...) {
    PyZy::Context *context = PyZy::InputContext::create(...);
    m_contexts.insert(context);
  }

  bool deleteContext(const InputContext *context) {
    return m_contexts.erase(context);
  }

  // Updates global config
  void setModeSimpGlobal(bool value) {
    for (set<PyZy::InputContext>::iterator it = ...) {
      it->setOptionModeSimp(value);
    }
  }

  // Implements setters for other global config.
  ...
private:
  // This class takes a ownership of contexts.
  static set<PyZy::InputContext *> m_contexts;
};
-----------------

Is this solution is OK? If yes, I will implements it next week.


BTW, how about following solution to access / modify config?
- Creates a config instance on pyzy client side codes.
- Register a config instance to InputContext instance on the constructor.
- Manipulates a local config through a config instance instead of accessors on
InputContext.
- Manipulates a global config through a ConfigManager class. (similar to
ContextManager described above)

I think it is better since config related codes on pyzy client only handles a
instance of PyZy::Config. This architecture is good for ibus-mozc-pinyin (and
maybe ibus-pinyin, too)

[hsumita1, 2012-05-11 10:48:43]

Sorry, I have forgotton to publish a comment...

https://codereview.appspot.com/6202056/diff/3003/src/PyZyConfig.h
File src/PyZyConfig.h (right):

https://codereview.appspot.com/6202056/diff/3003/src/PyZyConfig.h#newcode72
src/PyZyConfig.h:72: #define DOUBLE_PINYIN_KEYBOARD_LAST   (6)
To keep a consistency with ibus-pinyin. (see also a enum on
ibus-pinyin/src/PYPinyinEngine.h)

[Peng, 2012-05-11 15:06:43]

On 2012/05/11 07:46:22, hsumita1 wrote:
> If we adopt this solution, I think we needs ConfigManager class like following
> on ibus-mozc-pinyin and ibus-pinyin to implement global config.
> 
> -----------------
> class ContextManager {
>   static PyZy::Context createContext(...) {
>     PyZy::Context *context = PyZy::InputContext::create(...);
>     m_contexts.insert(context);
>   }
> 
>   bool deleteContext(const InputContext *context) {
>     return m_contexts.erase(context);
>   }
> 
>   // Updates global config
>   void setModeSimpGlobal(bool value) {
>     for (set<PyZy::InputContext>::iterator it = ...) {
>       it->setOptionModeSimp(value);
>     }
>   }
> 
>   // Implements setters for other global config.
>   ...
> private:
>   // This class takes a ownership of contexts.
>   static set<PyZy::InputContext *> m_contexts;
> };
> -----------------
> 
> Is this solution is OK? If yes, I will implements it next week.

I think this solution should work well. 

> 
> 
> BTW, how about following solution to access / modify config?
> - Creates a config instance on pyzy client side codes.
> - Register a config instance to InputContext instance on the constructor.
> - Manipulates a local config through a config instance instead of accessors on
> InputContext.
> - Manipulates a global config through a ConfigManager class. (similar to
> ContextManager described above)
> 
> I think it is better since config related codes on pyzy client only handles a
> instance of PyZy::Config. This architecture is good for ibus-mozc-pinyin (and
> maybe ibus-pinyin, too)

For this solution, I have several questions:

1. If all InputContexts share one config instance, how do you make some settings
local, and some global.
2. Glocal/Local settings should be controlled by library users. Does
InputContext need know which is local or global?

[Peng, 2012-05-11 15:06:51]

https://codereview.appspot.com/6202056/diff/3003/src/PyZyConfig.h
File src/PyZyConfig.h (right):

https://codereview.appspot.com/6202056/diff/3003/src/PyZyConfig.h#newcode72
src/PyZyConfig.h:72: #define DOUBLE_PINYIN_KEYBOARD_LAST   (6)
On 2012/05/11 10:48:43, hsumita1 wrote:
> To keep a consistency with ibus-pinyin. (see also a enum on
> ibus-pinyin/src/PYPinyinEngine.h)
I see. Thanks

[hsumita, 2012-05-14 12:49:03]

I assume a following usage.

PyZy::Config config1, config2;
// context1 has a own config
PyZy::InputContext *context1 = PyZy::InputContext::create(..., config1, ...);
// context2 and context3 has a shared config
PyZy::InputContext *context2 = PyZy::InputContext::create(..., config2, ...);
PyZy::InputContext *context3 = PyZy::InputContext::create(..., config2, ...);

Of course library user can determine the local / globa config with following
class.

class ConfigManager {
public:
  static PyZy::Config *create_instance();
  static void delete_instance(PyZy::Config *config);

  void set_local_config1(bool value);
  static void set_global_config1(bool value);
  ...

private:
  static vector<PyZy::Config *> m_config
};

[Peng, 2012-05-14 14:46:35]

On 2012/05/14 12:49:03, hsumita wrote:
> I assume a following usage.
> 
> PyZy::Config config1, config2;
> // context1 has a own config
> PyZy::InputContext *context1 = PyZy::InputContext::create(..., config1, ...);
> // context2 and context3 has a shared config
> PyZy::InputContext *context2 = PyZy::InputContext::create(..., config2, ...);
> PyZy::InputContext *context3 = PyZy::InputContext::create(..., config2, ...);
> 
> Of course library user can determine the local / globa config with following
> class.
> 
> class ConfigManager {
> public:
>   static PyZy::Config *create_instance();
>   static void delete_instance(PyZy::Config *config);
> 
>   void set_local_config1(bool value);
>   static void set_global_config1(bool value);
>   ...
> 
> private:
>   static vector<PyZy::Config *> m_config
> };

I believe ConfigManager class is not in libpyzy, and library user need to
inherit pyzy::Config for loading and saving settings. It can works. But as
library, it is not very user friendly. Users have to implement and provide
Config classes. Especially if it will be common library, it may be used by
variants projects (not only ibus). It is very likely be used by other script
languages. It is hard to inherit C++ classes by script languages.

[hsumita1, 2012-05-15 04:43:25]

Sorry for my poor description. I means that pyzy user implements a ConfigManager
to handle global config.

Anyway, I agree with your opinions. I will implement it soon.

[hsumita1, 2012-05-15 08:45:35]

patchset 5
- Append accessors on InputContext to manipulate config.
- Makes Config class to struct since it is no longer exported to pyzy users.
- Renames some config name.
  - option -> conversionOption (option is a little hard to understand)
  - bopomofoKeyboardMapping -> bopomofoSchema (to keep a consistency with
doublePinyinSchema)
- Moves constants from PyZyConfig.h into PyZyConst.h since
  - PyZyConfig.h is no longer exported to pyzy users.
  - It is a little hard to moves these into PyZyInputContext.h since it be a
cause of cyclic dependency on pyzy.

[hsumita1, 2012-05-15 08:48:29]

Sorry.
s/patchset 5/patchset 6/

[Peng, 2012-05-15 15:30:32]

https://codereview.appspot.com/6202056/diff/7003/src/PyZyInputContext.h
File src/PyZyInputContext.h (right):

https://codereview.appspot.com/6202056/diff/7003/src/PyZyInputContext.h#newco...
src/PyZyInputContext.h:116: virtual unsigned int getOptionConversionOption ()
const = 0;
Could you consider adding functions 
void setProperty(const string & name, const variant value);
variant getProperty(const string & name);

And implement a simple variant class like:

class variant {
   enum TYPE {
       TYPE_NULL =0,
       TYPE_BOOL, TYPE_INT, ...
   };

   union {
        bool bool_value;
        int  int_value;
        ...
   } value;
   
   variant(bool v);
   variant(int v);
   variant(...);
   
   operator bool();
   operator int ();
...
}

And then you don't need put all getters and setters in InputContext.
For example: {set,get}OptionBopomofoSchema() should be in BopomofoInputContext
only. And it is not necessary to be a public function.

[hsumita1, 2012-05-16 03:21:22]

https://codereview.appspot.com/6202056/diff/7003/src/PyZyInputContext.h
File src/PyZyInputContext.h (right):

https://codereview.appspot.com/6202056/diff/7003/src/PyZyInputContext.h#newco...
src/PyZyInputContext.h:116: virtual unsigned int getOptionConversionOption ()
const = 0;
Hmm... currently exported files for pyzy users are PyZyInputContext.h and
PyZyConst.h only, so pyzy user can't inherit BopomofoContext directly. I expect
that pyzy users creates a instance of BopomofoContext as a pointer of
InputContext through InputContext::create(...) and use it.

To keep the architecture, we should append getter / setter for all config on
InputContext and make these public. (It includes the method using variant class)
If we use a variant class and implement getter / setter using it, we can forbid
the access to bopomofoSchema through the instance of FullPinyinContext stored on
a pointer of InputContext.

[Peng, 2012-05-16 14:27:56]

On 2012/05/16 03:21:22, hsumita1 wrote:
> https://codereview.appspot.com/6202056/diff/7003/src/PyZyInputContext.h
> File src/PyZyInputContext.h (right):
> 
>
https://codereview.appspot.com/6202056/diff/7003/src/PyZyInputContext.h#newco...
> src/PyZyInputContext.h:116: virtual unsigned int getOptionConversionOption ()
> const = 0;
> Hmm... currently exported files for pyzy users are PyZyInputContext.h and
> PyZyConst.h only, so pyzy user can't inherit BopomofoContext directly. I
expect
> that pyzy users creates a instance of BopomofoContext as a pointer of
> InputContext through InputContext::create(...) and use it.

I see. {set,get}OptionBopomofoSchema() will be only used internal as helper
function.

> 
> To keep the architecture, we should append getter / setter for all config on
> InputContext and make these public. (It includes the method using variant
class)
> If we use a variant class and implement getter / setter using it, we can
forbid
> the access to bopomofoSchema through the instance of FullPinyinContext stored
on
> a pointer of InputContext.

Putting all setter & getter in PyZyInputContext (the public interface) is not
very flexible. Adding and removing a setting item may cause ABI broken. You need
to rebuild all apps use it. So it is better to re-factor it.

[hsumita1, 2012-05-17 07:41:03]

> Putting all setter & getter in PyZyInputContext (the public interface) is not
> very flexible. Adding and removing a setting item may cause ABI broken. You
need
> to rebuild all apps use it. So it is better to re-factor it.

Is re-factor means that we append setter / getter with variant class on
InputContext?
If so, I implements it on Patchset 8. I believe that using overload for
accessors is not good since it could be confusable on some situations (If there
are int and unsigned int properties), so I implements a type specified functions
like getUnsignedInt(). Could you review it?

[Peng, 2012-05-17 14:14:30]

lgtm, with several comments.

https://codereview.appspot.com/6202056/diff/4004/src/PyZyConfig.h
File src/PyZyConfig.h (right):

https://codereview.appspot.com/6202056/diff/4004/src/PyZyConfig.h#newcode41
src/PyZyConfig.h:41: unsigned int bopomofoSchema;
Because we switched to use setProperty & getProperty, bopomofoSchema and
doublePinyinSchema are not used in base IC classes. So I think this Config is
not useful anymore. I suggest just put member fields to each IC classes.

https://codereview.appspot.com/6202056/diff/4004/src/PyZyInputContext.h
File src/PyZyInputContext.h (right):

https://codereview.appspot.com/6202056/diff/4004/src/PyZyInputContext.h#newco...
src/PyZyInputContext.h:30: class Variant {
Use a separate .h and .cc files for this class?

[hsumita1, 2012-05-18 02:17:08]

Thanks a lot for your review!

https://codereview.appspot.com/6202056/diff/4004/src/PyZyConfig.h
File src/PyZyConfig.h (right):

https://codereview.appspot.com/6202056/diff/4004/src/PyZyConfig.h#newcode41
src/PyZyConfig.h:41: unsigned int bopomofoSchema;
Done.

https://codereview.appspot.com/6202056/diff/4004/src/PyZyInputContext.h
File src/PyZyInputContext.h (right):

https://codereview.appspot.com/6202056/diff/4004/src/PyZyInputContext.h#newco...
src/PyZyInputContext.h:30: class Variant {
On 2012/05/17 14:14:30, Peng wrote:
> Use a separate .h and .cc files for this class?

Done.