Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2506)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 4902051: Use GVariant as attachment for IBusSerializable. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 8 months ago by Peng
Modified:
12 years, 8 months ago
Reviewers:
Yusuke Sato, horo1
Base URL:
git@github.com:ibus/ibus.git@master
Visibility:
Public.

Description

Use GVariant as attachment for IBusSerializable. BUG=None TEST=Linux desktop

Patch Set 1 #

Total comments: 6

Patch Set 2 : Fix review issues #

Total comments: 19

Patch Set 3 : Fix review issues #

Unified diffs Side-by-side diffs Delta from patch set Stats (+98 lines, -206 lines) Patch
M src/ibusserializable.h View 1 2 2 chunks +40 lines, -41 lines 0 comments Download
M src/ibusserializable.c View 5 chunks +26 lines, -150 lines 0 comments Download
M src/tests/ibus-serializable.c View 1 2 2 chunks +32 lines, -15 lines 0 comments Download

Messages

Total messages: 6
Peng
12 years, 8 months ago (2011-08-18 17:46:27 UTC) #1
horo1
Thank you for the cl! http://codereview.appspot.com/4902051/diff/1/src/tests/ibus-serializable.c File src/tests/ibus-serializable.c (right): http://codereview.appspot.com/4902051/diff/1/src/tests/ibus-serializable.c#newcode151 src/tests/ibus-serializable.c:151: IBusSerializable *object = (IBusSerializable ...
12 years, 8 months ago (2011-08-19 01:14:29 UTC) #2
Peng
http://codereview.appspot.com/4902051/diff/1/src/tests/ibus-serializable.c File src/tests/ibus-serializable.c (right): http://codereview.appspot.com/4902051/diff/1/src/tests/ibus-serializable.c#newcode151 src/tests/ibus-serializable.c:151: IBusSerializable *object = (IBusSerializable *) ibus_serializable_deserialize (variant); On 2011/08/19 ...
12 years, 8 months ago (2011-08-19 01:47:49 UTC) #3
Yusuke Sato
http://codereview.appspot.com/4902051/diff/1/src/tests/ibus-serializable.c File src/tests/ibus-serializable.c (right): http://codereview.appspot.com/4902051/diff/1/src/tests/ibus-serializable.c#newcode134 src/tests/ibus-serializable.c:134: IBusText *text = ibus_text_new_from_string ("main text"); nit: _from_static_string would ...
12 years, 8 months ago (2011-08-19 03:54:18 UTC) #4
Peng
http://codereview.appspot.com/4902051/diff/1/src/tests/ibus-serializable.c File src/tests/ibus-serializable.c (right): http://codereview.appspot.com/4902051/diff/1/src/tests/ibus-serializable.c#newcode134 src/tests/ibus-serializable.c:134: IBusText *text = ibus_text_new_from_string ("main text"); On 2011/08/19 03:54:19, ...
12 years, 8 months ago (2011-08-19 04:34:47 UTC) #5
Yusuke Sato
12 years, 8 months ago (2011-08-19 06:11:10 UTC) #6 
lgtm, thx

On 2011/08/19 04:34:47, Peng wrote:
> http://codereview.appspot.com/4902051/diff/1/src/tests/ibus-serializable.c
> File src/tests/ibus-serializable.c (right):
> 
>
http://codereview.appspot.com/4902051/diff/1/src/tests/ibus-serializable.c#ne...
> src/tests/ibus-serializable.c:134: IBusText *text =  ibus_text_new_from_string
> ("main text");
> On 2011/08/19 03:54:19, Yusuke Sato wrote:
> > nit: _from_static_string would be better.
> 
> Done.
> 
>
http://codereview.appspot.com/4902051/diff/1/src/tests/ibus-serializable.c#ne...
> src/tests/ibus-serializable.c:179: g_variant_type_info_assert_no_infos ();
> On 2011/08/19 03:54:19, Yusuke Sato wrote:
> > don't we have to unref the text object here?
> 
> Yes. Otherwise g_variant_type_info_assert_no_infos() will gives some errors.
> Because some resource used by GVariant are not released.
> 
> http://codereview.appspot.com/4902051/diff/4001/src/ibusserializable.h
> File src/ibusserializable.h (right):
> 
>
http://codereview.appspot.com/4902051/diff/4001/src/ibusserializable.h#newcod...
> src/ibusserializable.h:130: * @iter: An IBusMessageIter.
> On 2011/08/19 03:54:19, Yusuke Sato wrote:
> > should be @builder
> 
> Done.
> 
>
http://codereview.appspot.com/4902051/diff/4001/src/ibusserializable.h#newcod...
> src/ibusserializable.h:144: * @iter: An IBusMessageIter.
> On 2011/08/19 03:54:19, Yusuke Sato wrote:
> > @variant
> 
> Done.
> 
>
http://codereview.appspot.com/4902051/diff/4001/src/ibusserializable.h#newcod...
> src/ibusserializable.h:210: * @object: An IBusSerializable.
> On 2011/08/19 03:54:19, Yusuke Sato wrote:
> > ditto
> 
> Done.
> 
>
http://codereview.appspot.com/4902051/diff/4001/src/ibusserializable.h#newcod...
> src/ibusserializable.h:222: * @object: An IBusSerializable.
> On 2011/08/19 03:54:19, Yusuke Sato wrote:
> > ditto
> 
> Done.
> 
>
http://codereview.appspot.com/4902051/diff/4001/src/ibusserializable.h#newcod...
> src/ibusserializable.h:234: * @object: An IBusSerializable.
> On 2011/08/19 03:54:19, Yusuke Sato wrote:
> > ditto
> 
> Done.
> 
>
http://codereview.appspot.com/4902051/diff/4001/src/ibusserializable.h#newcod...
> src/ibusserializable.h:246: * @object: An IBusSerializable.
> On 2011/08/19 03:54:19, Yusuke Sato wrote:
> > ditto
> 
> Done.
> 
>
http://codereview.appspot.com/4902051/diff/4001/src/ibusserializable.h#newcod...
> src/ibusserializable.h:247: * @iter: An IBusMessageIter.
> On 2011/08/19 03:54:19, Yusuke Sato wrote:
> > remove this line
> 
> Done.
> 
>
http://codereview.appspot.com/4902051/diff/4001/src/ibusserializable.h#newcod...
> src/ibusserializable.h:248: * @returns: TRUE if succeed; FALSE otherwise.
> On 2011/08/19 03:54:19, Yusuke Sato wrote:
> > this line seems to be incorrect. pls fix.
> 
> Done.
> 
>
http://codereview.appspot.com/4902051/diff/4001/src/ibusserializable.h#newcod...
> src/ibusserializable.h:259: * @iter: An IBusMessageIter.
> On 2011/08/19 03:54:19, Yusuke Sato wrote:
> > ditto
> 
> Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b