chewing / chewing-editor

Cross platform chewing user phrase editor
https://chewing.im/
GNU General Public License v2.0
31 stars 52 forks source link

Refactor the parameter type of add function #178

Closed jeffreyhc closed 8 years ago

jeffreyhc commented 8 years ago

Ref: https://github.com/chewing/chewing-editor/pull/149#discussion_r57522841

I refactor the add function parameter type and rename the slot add function in UserphraseModel to addWrapper for temporary, if there is a better idea please tell me.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling fb06faeac8aebb85eeda8c3933186d26b4002453 on jeffreyhc:refactor into 3bd4913706774bf289a7d274d44cfac453f338b6 on chewing:master.

jserv commented 8 years ago

@jeffreyhc : The rename to addWrapper violates the naming schema, which consists of lowercase verb and the target object begining with a capital letter. Your intention was originally "add" rather than adding "Wrapper" object to somewhere.

jeffreyhc commented 8 years ago

@jserv : Thanks for your advice! How about addPhrase? (I think this maybe more precise but this function name is same as a function in UserphraseView.)

jserv commented 8 years ago

I agree with @Chocobo1 that add method with access specifier 'public` should be straightforward.

jeffreyhc commented 8 years ago

@Chocobo1 @jserv I've fixed all you mentioned and modify my git commit message.

jserv commented 8 years ago
  1. The description "from private to public" is quite ambiguous for people to distinguish from phrases and dictionary storage. You should mention the access specifier.
  2. Please explain the statement 'because "QString" can "implicit sharing"'.
jeffreyhc commented 8 years ago

@jserv Does the "access specifier" means slot and signal?

jserv commented 8 years ago

Read C++ Programming language specification for the terminologies. http://en.cppreference.com/w/cpp/language/access

jeffreyhc commented 8 years ago

@jserv I think I don't get your point

The description "from private to public" is quite ambiguous for people to distinguish from phrases and dictionary storage.

What is "dictionary storage"? Why do people can't distinguish phrases and dictionary storage?

You should mention the access specifier.

I thought "from private to public" mentioned about the access specifier?

jserv commented 8 years ago

@jeffreyhc Because Chewing does have "private" dictionary! It is so-called user-defined phrase in the form of "hash", which is historically a terminology existing for more than 15 years. In the opposite side, Chewing has the system dictionary, which is "public".

jserv commented 8 years ago

Userphrase is kept in private storage for each IM instance, and the way to change its descriptions might result in the misunderstanding about its transition to public via shared networks.

jeffreyhc commented 8 years ago

@jserv I plan to modify my commit message like this:

Refactor the parameter type of add function

Remove "shared_ptr" type for the QString buffer phrase and bopomofo,
because "QString" can "implicit sharing" just do the same thing of
"shared_ptr". "Implicit sharing" makes ownership of an object can be
shared through pointers and the object is copied only if and when
the content is going to be changed, i.e., copy-on-write.
Moreover, in UserphraseModel, I remove the public add() which only
calls the private add() and the private add() is the function really 
add phrase to dictionary. Then modify the access specifier of the 
add() from private to public.

How do you think about the message?

jserv commented 8 years ago

@jeffreyhc : Do after review! Update frequently, and everyone can provides the feedback often.

jeffreyhc commented 8 years ago

@jserv OK, I update my commit.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling 25a00d26a36358c152a46db7d4d283ee44bda939 on jeffreyhc:refactor into 3bd4913706774bf289a7d274d44cfac453f338b6 on chewing:master.

jserv commented 8 years ago

Fix the syntax error in commit messages:

"QString" can "implicit sharing" just do the same thing of "shared_ptr"

jeffreyhc commented 8 years ago

How about this version?

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling 762b9333912459ede440fbfdf998b7603b40f328 on jeffreyhc:refactor into 3bd4913706774bf289a7d274d44cfac453f338b6 on chewing:master.

jserv commented 8 years ago

Don't use quotation marks to specify a common term or phrase such as "QString" in Git commit messages. Also, you don't have to inform others of the time when the revised patch was pushed since CI does this well.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 94.382% when pulling 06600d32649e81028df70d49fe97a537102b4759 on jeffreyhc:refactor into 3bd4913706774bf289a7d274d44cfac453f338b6 on chewing:master.