Issue 4844041: Support selection text retrival. (Closed)
|
Created:
12 years, 10 months ago by nona1 Modified:
12 years, 10 months ago CC:
kinaba Base URL:
git://github.com/ibus/ibus.git@master Visibility:
Public. |
DescriptionSupport selection text retrival.
This patch enable us to get selection text on client application.
Currently only GtkTextView widget can get them in gtk application.
BUG=None
TEST=manually done.(By gedit text editor)
Patch Set 1 #
Total comments: 16
Patch Set 2 : Selection handling make be same as surrounding text. #Patch Set 3 : Add argument selection range in surrounding text #
Total comments: 48
Patch Set 4 : Introduce anchor position and fix code styles. #
Total comments: 8
Patch Set 5 : Update python scripts #
Total comments: 6
Patch Set 6 : Update python interface signature. #
MessagesTotal messages: 21
Dear Peng and yusuke, I am Seigo Nonaka. I'd like to do a code review about ibus's new feature. I'd like to implement getting a selection text from client. I believe that such function enable us to support "back to preedit from committed string" or other innovative features. Thank you.
Sign in to reply to this message.
(+ueno-san who implemented GetSurroundingText for ibus) Hi Nona, Thank you for the patch. First, though I'm sure that the feature is important for Japanese IMEs like ibus-mozc, I think it'd be better to elaborate why the existing feature (i.e. surrounding text retrieval) is not sufficient for you. Please update the CL description if you agree. Some questions and suggestions below: 1. capability Probably you should add new CAP_XXX since clients other than Gtk+ (e.g. Chrome, Qt) do not support the feature yet. 2. use case How does ibus-mozc use the new API? I guess your plan would be something like this: 1) a user selects a committed text (e.g. by shift+arrow). Nothing happens in im-ibus.so at this point. 2) a user presses "reconvert selected text" key (e.g. ctrl+backspace) 3) the key press is sent to ibus-mozc 4) ibus-mozc tries to retrieve the selection range _asynchronously_ in its process_key_event handler. 5) ibus-mozc receives the selection range, and starts the reconversion process. I think there'd be at least 4 issues in this scenario: - In step 4), ibus-mozc can't determine the correct return value of process_key_event (consumed or not consumed). For example, when no text is selected, the function should return "not consumed", but it's not possible since the selection range retrieval is asynchronous. - There's no easy way for ibus-mozc to wait for the event 5). Polling? - What happens if other important event such as focus in, focus out, and key press comes between 4) and 5)? - What happens if the event 5) never comes? This could be fixed by adding a new CAP tho. 3. idea for improvement I think it'd be better to set the selection range in the same way as surrounding text. How about implementing the new feature like this? - Add selection_range_start and selection_range_end parameters to the SetSurroundingText IPC. - When a client (e.g. im-ibus) calls SetSurroundingText, the client tries to get selection range using platform dependent API, and sends it to engine along with the surrounding text. Then, ibus-mozc could be implemented like this: 1) ibus-mozc starts. It tells ibus-daemon that the engine needs surrounding text (and selection range). 2) a user selects a committed text (e.g. shift+arrow). nothing happens in im-ibus.so at this point. 3) a user presses "reconvert selected text" key (e.g. ctrl+backspace) 4) the selection range (and a surrounding text) is sent to ibus-mozc 5) the key press is sent to ibus-mozc 6) ibus-mozc can start the reconversion process immediately since it already knows the selection range. (This is just an idea, and I'm not so familiar with GetSurroundingText. Please wait for comments from other reviewers.) http://codereview.appspot.com/4844041/diff/1/bus/engineproxy.h File bus/engineproxy.h (right): http://codereview.appspot.com/4844041/diff/1/bus/engineproxy.h#newcode252 bus/engineproxy.h:252: IBusText *text); please align http://codereview.appspot.com/4844041/diff/1/bus/inputcontext.c File bus/inputcontext.c (right): http://codereview.appspot.com/4844041/diff/1/bus/inputcontext.c#newcode1814 bus/inputcontext.c:1814: static void add function comment http://codereview.appspot.com/4844041/diff/1/bus/inputcontext.c#newcode1818 bus/inputcontext.c:1818: bus_input_context_emit_signal (context, add the g_assert checks as well as the context->enabled check? http://codereview.appspot.com/4844041/diff/1/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): http://codereview.appspot.com/4844041/diff/1/client/gtk2/ibusimcontext.c#newc... client/gtk2/ibusimcontext.c:1376: IBusText *text = ibus_text_new_from_string(""); ibus_text_new_from_static_string http://codereview.appspot.com/4844041/diff/1/src/ibusengine.c File src/ibusengine.c (right): http://codereview.appspot.com/4844041/diff/1/src/ibusengine.c#newcode729 src/ibusengine.c:729: engine_signals[SET_SELECTION_TEXT] = comment is missing http://codereview.appspot.com/4844041/diff/1/src/ibusengine.h File src/ibusengine.h (right): http://codereview.appspot.com/4844041/diff/1/src/ibusengine.h#newcode155 src/ibusengine.h:155: gpointer pdummy[5]; [4] http://codereview.appspot.com/4844041/diff/1/src/ibusengine.h#newcode437 src/ibusengine.h:437: remove this line http://codereview.appspot.com/4844041/diff/1/src/ibusinputcontext.c File src/ibusinputcontext.c (right): http://codereview.appspot.com/4844041/diff/1/src/ibusinputcontext.c#newcode459 src/ibusinputcontext.c:459: context_signals[GET_SELECTION_TEXT] = comment
Sign in to reply to this message.
I agree with Yusuke to improve SurroundingText instead of adding new IPC. I also embedded some comments. > 3. idea for improvement > > I think it'd be better to set the selection range in the same way as surrounding > text. How about implementing the new feature like this? > > - Add selection_range_start and selection_range_end parameters to the > SetSurroundingText IPC. > - When a client (e.g. im-ibus) calls SetSurroundingText, the client tries to get > selection range using platform dependent API, and sends it to engine along with > the surrounding text. > > Then, ibus-mozc could be implemented like this: > > 1) ibus-mozc starts. It tells ibus-daemon that the engine needs surrounding text > (and selection range). ibus-mozc tells ibus-daemon and application that the engine needs surrounding text, and then applications will send any surrounding text (include selection range) changes to engine via ibus-daemon. The engine need keep a copy of surrounding text information which will be used later. > 2) a user selects a committed text (e.g. shift+arrow). nothing happens in > im-ibus.so at this point. > 3) a user presses "reconvert selected text" key (e.g. ctrl+backspace) > 4) the selection range (and a surrounding text) is sent to ibus-mozc This step is not necessary, because the engine should has surrounding text already. > 5) the key press is sent to ibus-mozc > 6) ibus-mozc can start the reconversion process immediately since it already > knows the selection range.
Sign in to reply to this message.
Thank you for reviewing! I also agree with Yusuke-san's opinion basically. However I want to change API/IPC designing. We can choose two type of change to treat selection text. 1) Adding new arguments into set/get_surrounding_text without any additional dbus method. 2) Adding new dbus method and new API like get/set_selection_text. I want to choose 2), because there are two reasons. First, I think that selection text and surrounding text are independent. For example, in case of using GNOME Terminal, we can make a selection any place on the window. And we can also input any characters into terminal with remaining the selection area. In such case, selection area does not exist near surrounding text. (Unfortunately, this patch can not be enable us to retrieve selection on GNOME Terminal. Because GNOME terminal is not GtkTextView widget) Second, it is difficult to implement Yusuke-san's suggestion on GTK2/immodule. In my understanidng, surrounding-text is automatically sent by application. So that, we can not expand "surrounding" area which is not expected to be large enough to cover selection area. In addition, cursor position is also sent by application and it is relative offset from head of surrounding text buffer. However, getting selection information from TextBuffer, we can only get gloabl(whole text) offset. It is impossible to convert from global offset to relative offset generally. So that, I want to make additional API/IPC to treat selected text, start offset and end offset(this is duplicated information but convenient) as same as surrounding text. And also I think this IPC/API communication cost is smaller than surrounding text. Could you tell me any adovise about this API/IPC desiging? PS: I already implemented with 2) API/IPC design and confirmed to get selection text successfully on GtkTextView widget(include GEdit). However the code is still dirty, I will refine it and upload soon. Thank you. Seigo.
Sign in to reply to this message.
I don't know the use cases of reconvert very well. For my understanding, the reconvert feature does not only include getting selection text, but also include editing and replacing selection text. And it should be only work with an editable selection range. And IME has to use surrounding related APIs to change text buffer around input cursor. So probably integrate selection range with surrounding text is a easier way. In gtk, surrounding text is the current line (paragraph) of text in most gtk widgets. I think paragraph should be a reasonable maximum rang for reconvert. If the selection range includes two or more paragraphs, the IME could only reconvert the union range of the surrounding and selection ranges. I think probably it is more reasonable. For IPC/API design, ibus does not have a reliable way to retrieve information from application synchronously. Probably you have to use similar way of surrounding text - application notifies IME of all changes of selection. So I think it is not necessary to create a new IPCs. Peng On 2011/08/02 18:43:37, nona1 wrote: > Thank you for reviewing! > > I also agree with Yusuke-san's opinion basically. > However I want to change API/IPC designing. > > We can choose two type of change to treat selection text. > 1) Adding new arguments into set/get_surrounding_text without any additional > dbus method. > 2) Adding new dbus method and new API like get/set_selection_text. > > I want to choose 2), because there are two reasons. > > First, I think that selection text and surrounding text are independent. > For example, in case of using GNOME Terminal, we can make a selection any place > on the window. > And we can also input any characters into terminal with remaining the selection > area. > In such case, selection area does not exist near surrounding text. > (Unfortunately, this patch can not be enable us to retrieve selection on GNOME > Terminal. Because GNOME terminal is not GtkTextView widget) > > Second, it is difficult to implement Yusuke-san's suggestion on GTK2/immodule. > In my understanidng, surrounding-text is automatically sent by application. > So that, we can not expand "surrounding" area which is not expected to be large > enough to cover selection area. > In addition, cursor position is also sent by application and it is relative > offset from head of surrounding text buffer. However, getting selection > information from TextBuffer, we can only get gloabl(whole text) offset. It is > impossible to convert from global offset to relative offset generally. > > So that, I want to make additional API/IPC to treat selected text, start offset > and end offset(this is duplicated information but convenient) as same as > surrounding text. And also I think this IPC/API communication cost is smaller > than surrounding text. > > > Could you tell me any adovise about this API/IPC desiging? > > PS: > I already implemented with 2) API/IPC design and confirmed to get selection text > successfully on GtkTextView widget(include GEdit). > However the code is still dirty, I will refine it and upload soon. > > Thank you. > > Seigo.
Sign in to reply to this message.
Ah... I think too much to be general. As you mentioned, it is enough to be limit within surrounding text. I will fix them and re-upload soon. Thank you! Seigo.
Sign in to reply to this message.
I update patch set, but I still have probrems around API designing.
(Sorry for long reply)
1. Overview of this patch
Sorry this patch is still under constricting, but I want to confirm my policy
is good for ibus commiters. The rest of the job is adding capability for
selection text.
This patch still add apis(get/set_selection_text) due to following
problem(Please see 2).
However the retrieval algorightm is different from the past, it is same as the
surrounding text processing(that is, this patch fits asynchronous design).
2. The problem of combining surrounding text api(GTK).
The problem is we cannot identify the selection area from the surrounding text.
The reason is that we cannot know where selection area is on the surrounding
text.
The GTK application only give us the surrounding text and the cursor position,
and also give us the selection text and the selection start point and end point.
The problem is that the former cursor position is relative position of
surrounding text, and the latter start/end point is the absolute positions of
the whole text.
So we cannot get the relative selection position of surrounding text from given
information.
At first I imagine that I can solve this problem with substring matching, but
it cannot solve this problem. For example, if the whole text are composed by
"a", the surrounding text and selection text are also contained by "a", like
follows.
(surrounding text, cursor_position) = ("aaaaaaaaaaa", 4)
(selection text, start, end) = ("aaa", 43, 46)
In this case, the substring match cannot detect selection area.
Therefore, I think that combining the surrounding handling and selection
handling is not good way.
3. What I want to do by IBus.
Let me explain why I want to add this featrue. At first, I want to implement
the feature called "reconversion".
The typical reconversion works as follows. (Following steps are Japanese as
example, but it can easily apply the other CJK language).
1. Input strings as usual and commit string completely. (e.g. 寒さを侵して)
2. Select committed string(Usually it is wrongly committed string.) (e.g.
寒さを[侵して], [] is selection）
3. Call "Reconversion" function by clikking toolbar or keyboard shortcut. Then
selection become preedit string. (e.g 寒さを{おかして}, {} is preedit)
4. Re-select the candidate from lookup table, and commit. (e.g. 寒さを冒して)
To implement above feature , it is required the functionality of getting/setting
selection text.
And also I think this functinality is provide other good features. For example,
looking up dictionary from selected text, opening browser search result page
with selected text, etc...
So that, I want to add this functionality to IBus.
http://codereview.appspot.com/4844041/diff/1/bus/engineproxy.h
File bus/engineproxy.h (right):
http://codereview.appspot.com/4844041/diff/1/bus/engineproxy.h#newcode252
bus/engineproxy.h:252: IBusText *text);
On 2011/08/02 09:05:56, Yusuke Sato wrote:
> please align
Done.
http://codereview.appspot.com/4844041/diff/1/bus/inputcontext.c
File bus/inputcontext.c (right):
http://codereview.appspot.com/4844041/diff/1/bus/inputcontext.c#newcode1814
bus/inputcontext.c:1814: static void
On 2011/08/02 09:05:56, Yusuke Sato wrote:
> add function comment
I removed this function.
http://codereview.appspot.com/4844041/diff/1/bus/inputcontext.c#newcode1818
bus/inputcontext.c:1818: bus_input_context_emit_signal (context,
On 2011/08/02 09:05:56, Yusuke Sato wrote:
> add the g_assert checks as well as the context->enabled check?
Sorry I removed this function. Thank you for reviewing!
http://codereview.appspot.com/4844041/diff/1/client/gtk2/ibusimcontext.c
File client/gtk2/ibusimcontext.c (right):
http://codereview.appspot.com/4844041/diff/1/client/gtk2/ibusimcontext.c#newc...
client/gtk2/ibusimcontext.c:1376: IBusText *text =
ibus_text_new_from_string("");
On 2011/08/02 09:05:56, Yusuke Sato wrote:
> ibus_text_new_from_static_string
Done.
http://codereview.appspot.com/4844041/diff/1/src/ibusengine.c
File src/ibusengine.c (right):
http://codereview.appspot.com/4844041/diff/1/src/ibusengine.c#newcode729
src/ibusengine.c:729: engine_signals[SET_SELECTION_TEXT] =
On 2011/08/02 09:05:56, Yusuke Sato wrote:
> comment is missing
Done.
http://codereview.appspot.com/4844041/diff/1/src/ibusengine.h
File src/ibusengine.h (right):
http://codereview.appspot.com/4844041/diff/1/src/ibusengine.h#newcode155
src/ibusengine.h:155: gpointer pdummy[5];
On 2011/08/02 09:05:56, Yusuke Sato wrote:
> [4]
Done.
http://codereview.appspot.com/4844041/diff/1/src/ibusengine.h#newcode437
src/ibusengine.h:437:
On 2011/08/02 09:05:56, Yusuke Sato wrote:
> remove this line
Done.
http://codereview.appspot.com/4844041/diff/1/src/ibusinputcontext.c
File src/ibusinputcontext.c (right):
http://codereview.appspot.com/4844041/diff/1/src/ibusinputcontext.c#newcode459
src/ibusinputcontext.c:459: context_signals[GET_SELECTION_TEXT] =
On 2011/08/02 09:05:56, Yusuke Sato wrote:
> comment
Sorry I removed this signal. Thank you!.
Sign in to reply to this message.
On 2011/08/07 15:58:02, nona1 wrote:
> I update patch set, but I still have probrems around API designing.
> (Sorry for long reply)
>
> 1. Overview of this patch
> Sorry this patch is still under constricting, but I want to confirm my policy
> is good for ibus commiters. The rest of the job is adding capability for
> selection text.
> This patch still add apis(get/set_selection_text) due to following
> problem(Please see 2).
> However the retrieval algorightm is different from the past, it is same as the
> surrounding text processing(that is, this patch fits asynchronous design).
>
> 2. The problem of combining surrounding text api(GTK).
> The problem is we cannot identify the selection area from the surrounding
text.
> The reason is that we cannot know where selection area is on the surrounding
> text.
> The GTK application only give us the surrounding text and the cursor position,
> and also give us the selection text and the selection start point and end
point.
> The problem is that the former cursor position is relative position of
> surrounding text, and the latter start/end point is the absolute positions of
> the whole text.
> So we cannot get the relative selection position of surrounding text from
given
> information.
> At first I imagine that I can solve this problem with substring matching, but
> it cannot solve this problem. For example, if the whole text are composed by
> "a", the surrounding text and selection text are also contained by "a", like
> follows.
> (surrounding text, cursor_position) = ("aaaaaaaaaaa", 4)
> (selection text, start, end) = ("aaa", 43, 46)
> In this case, the substring match cannot detect selection area.
> Therefore, I think that combining the surrounding handling and selection
> handling is not good way.
Did you try get the cursor position from GtkTextBuffer directly? I think you
already get the selection from GtkTextBuffer, you could also get the current
cursor information from it too.
>
> 3. What I want to do by IBus.
> Let me explain why I want to add this featrue. At first, I want to implement
> the feature called "reconversion".
> The typical reconversion works as follows. (Following steps are Japanese as
> example, but it can easily apply the other CJK language).
> 1. Input strings as usual and commit string completely. (e.g. 寒さを侵して)
> 2. Select committed string(Usually it is wrongly committed string.) (e.g.
> 寒さを[侵して], [] is selection）
> 3. Call "Reconversion" function by clikking toolbar or keyboard shortcut. Then
> selection become preedit string. (e.g 寒さを{おかして}, {} is preedit)
> 4. Re-select the candidate from lookup table, and commit. (e.g. 寒さを冒して)
> To implement above feature , it is required the functionality of
getting/setting
> selection text.
> And also I think this functinality is provide other good features. For
example,
> looking up dictionary from selected text, opening browser search result page
> with selected text, etc...
> So that, I want to add this functionality to IBus.
IBTW, I think you could use X ipc to get the selection. Below is related
document. http://developer.gnome.org/gtk3/stable/gtk3-Selections.html
>
> http://codereview.appspot.com/4844041/diff/1/bus/engineproxy.h
> File bus/engineproxy.h (right):
>
> http://codereview.appspot.com/4844041/diff/1/bus/engineproxy.h#newcode252
> bus/engineproxy.h:252: IBusText *text);
> On 2011/08/02 09:05:56, Yusuke Sato wrote:
> > please align
>
> Done.
>
> http://codereview.appspot.com/4844041/diff/1/bus/inputcontext.c
> File bus/inputcontext.c (right):
>
> http://codereview.appspot.com/4844041/diff/1/bus/inputcontext.c#newcode1814
> bus/inputcontext.c:1814: static void
> On 2011/08/02 09:05:56, Yusuke Sato wrote:
> > add function comment
>
> I removed this function.
>
> http://codereview.appspot.com/4844041/diff/1/bus/inputcontext.c#newcode1818
> bus/inputcontext.c:1818: bus_input_context_emit_signal (context,
> On 2011/08/02 09:05:56, Yusuke Sato wrote:
> > add the g_assert checks as well as the context->enabled check?
> Sorry I removed this function. Thank you for reviewing!
>
> http://codereview.appspot.com/4844041/diff/1/client/gtk2/ibusimcontext.c
> File client/gtk2/ibusimcontext.c (right):
>
>
http://codereview.appspot.com/4844041/diff/1/client/gtk2/ibusimcontext.c#newc...
> client/gtk2/ibusimcontext.c:1376: IBusText *text =
> ibus_text_new_from_string("");
> On 2011/08/02 09:05:56, Yusuke Sato wrote:
> > ibus_text_new_from_static_string
>
> Done.
>
> http://codereview.appspot.com/4844041/diff/1/src/ibusengine.c
> File src/ibusengine.c (right):
>
> http://codereview.appspot.com/4844041/diff/1/src/ibusengine.c#newcode729
> src/ibusengine.c:729: engine_signals[SET_SELECTION_TEXT] =
> On 2011/08/02 09:05:56, Yusuke Sato wrote:
> > comment is missing
>
> Done.
>
> http://codereview.appspot.com/4844041/diff/1/src/ibusengine.h
> File src/ibusengine.h (right):
>
> http://codereview.appspot.com/4844041/diff/1/src/ibusengine.h#newcode155
> src/ibusengine.h:155: gpointer pdummy[5];
> On 2011/08/02 09:05:56, Yusuke Sato wrote:
> > [4]
>
> Done.
>
> http://codereview.appspot.com/4844041/diff/1/src/ibusengine.h#newcode437
> src/ibusengine.h:437:
> On 2011/08/02 09:05:56, Yusuke Sato wrote:
> > remove this line
>
> Done.
>
> http://codereview.appspot.com/4844041/diff/1/src/ibusinputcontext.c
> File src/ibusinputcontext.c (right):
>
> http://codereview.appspot.com/4844041/diff/1/src/ibusinputcontext.c#newcode459
> src/ibusinputcontext.c:459: context_signals[GET_SELECTION_TEXT] =
> On 2011/08/02 09:05:56, Yusuke Sato wrote:
> > comment
>
> Sorry I removed this signal. Thank you!.
Sign in to reply to this message.
> Did you try get the cursor position from GtkTextBuffer directly? I think you > already get the selection from GtkTextBuffer, you could also get the current > cursor information from it too. Thank you! I resolved related problem with getting current position directly. I updated patch set. Please review again. > IBTW, I think you could use X ipc to get the selection. Below is related > document. http://developer.gnome.org/gtk3/stable/gtk3-Selections.html I'm grateful to you for your advice. But I'm sorry I cannot clearly understand what you intend. I don't think using IPC to get selection is not a good idea, because I think it is overfitting.
Sign in to reply to this message.
mostly LG http://codereview.appspot.com/4844041/diff/15001/bus/engineproxy.c File bus/engineproxy.c (right): http://codereview.appspot.com/4844041/diff/15001/bus/engineproxy.c#newcode363 bus/engineproxy.c:363: engine->selection_start_pos = 0; If I understand correctly, 362-364 are not necessary. the structure is automatically memzero'ed. http://codereview.appspot.com/4844041/diff/15001/bus/engineproxy.c#newcode1071 bus/engineproxy.c:1071: cursor_pos != engine->surrounding_cursor_pos) { engine->selection_start_pos != selection_start_pos || engine->selection_end_pos != selection_end_pos) { would be missing. http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:954: if(ibusimcontext->client_window == NULL) { space after 'if' http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:955: *start = *end = *cursor = 0; how about doing this initialization at the beginning of the function and remove 955, 961, 967, and 975? http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:958: gdk_window_get_user_data(ibusimcontext->client_window, (gpointer)&widget); space before '(' http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:960: if (!GTK_IS_TEXT_VIEW(widget)){ space http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:961: *start = *end = *cursor = 0; ditto http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:965: GtkTextView *text_view = GTK_TEXT_VIEW(widget); space http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:966: GtkTextBuffer *buffer = gtk_text_view_get_buffer(text_view); space http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:968: if (!gtk_text_buffer_get_has_selection(buffer)) { space http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:969: *start = *end = *cursor = 0; ditto http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:974: if (!gtk_text_buffer_get_selection_bounds(buffer, &start_iter, &end_iter)) { space http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:975: *start = *end = *cursor = 0; ditto http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:987: g_return_if_fail (*start <= *end); in this case (start is larger than end), i think we should return with *start = *end = *cursor = 0. move this line to line 982? http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:1027: if (start_pos < 0 || end_pos > len) { integer underflow. this does not make sense since these variables are unsigned. something like this would be better: if (start_pos < relative_origin || end_pos < relative_origin) { ... } start_pos -= relative_origin; end_pos -= relative_origin; if (end_pos > len) { ... } http://codereview.appspot.com/4844041/diff/15001/src/ibusengine.c File src/ibusengine.c (right): http://codereview.appspot.com/4844041/diff/15001/src/ibusengine.c#newcode725 src/ibusengine.c:725: G_TYPE_UINT); G_TYPE_UINT); should be G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT); http://codereview.appspot.com/4844041/diff/15001/src/ibusengine.c#newcode737 src/ibusengine.c:737: engine->priv->surrounding_cursor_pos = 0; 737-739 not necessary? http://codereview.appspot.com/4844041/diff/15001/src/ibusengine.c#newcode1408 src/ibusengine.c:1408: gboolean signal_only = (text == NULL); const http://codereview.appspot.com/4844041/diff/15001/src/ibusengine.c#newcode1421 src/ibusengine.c:1421: if (!signal_only) { why you removed cursor_pos check? is it safe? http://codereview.appspot.com/4844041/diff/15001/src/ibusengine.h File src/ibusengine.h (left): http://codereview.appspot.com/4844041/diff/15001/src/ibusengine.h#oldcode150 src/ibusengine.h:150: preserve this line if you have no reason to remove it. http://codereview.appspot.com/4844041/diff/15001/src/ibusinputcontext.c File src/ibusinputcontext.c (right): http://codereview.appspot.com/4844041/diff/15001/src/ibusinputcontext.c#newco... src/ibusinputcontext.c:471: priv->surrounding_cursor_pos = 0; remove 471-473? http://codereview.appspot.com/4844041/diff/15001/src/ibusinputcontext.c#newco... src/ibusinputcontext.c:1070: cursor_pos != priv->surrounding_cursor_pos) { ditto. should check selection_{start,end}_pos in priv. http://codereview.appspot.com/4844041/diff/15001/src/ibusinputcontext.h File src/ibusinputcontext.h (left): http://codereview.appspot.com/4844041/diff/15001/src/ibusinputcontext.h#oldco... src/ibusinputcontext.h:507: preserve the vertical space.
Sign in to reply to this message.
http://codereview.appspot.com/4844041/diff/15001/bus/engineproxy.c File bus/engineproxy.c (right): http://codereview.appspot.com/4844041/diff/15001/bus/engineproxy.c#newcode1064 bus/engineproxy.c:1064: guint selection_end_pos) IMHO, the cursor_pos should always be end position of selection. So probably we don't need add two arguments to the set surrounding text. Qt supports selection for IME. It uses Qt::ImCursorPosition & Qt::ImAnchorPosition for the selection range. Maybe we could use similar definition. http://doc.qt.nokia.com/latest/qt.html#InputMethodQuery-enum
Sign in to reply to this message.
http://codereview.appspot.com/4844041/diff/15001/bus/engineproxy.c File bus/engineproxy.c (right): http://codereview.appspot.com/4844041/diff/15001/bus/engineproxy.c#newcode363 bus/engineproxy.c:363: engine->selection_start_pos = 0; On 2011/08/10 12:05:21, Yusuke Sato wrote: > If I understand correctly, 362-364 are not necessary. the structure is > automatically memzero'ed. Done. http://codereview.appspot.com/4844041/diff/15001/bus/engineproxy.c#newcode1064 bus/engineproxy.c:1064: guint selection_end_pos) I got it! Done! On 2011/08/10 15:24:57, Peng wrote: > IMHO, the cursor_pos should always be end position of selection. So probably we > don't need add two arguments to the set surrounding text. > > Qt supports selection for IME. It uses Qt::ImCursorPosition & > Qt::ImAnchorPosition for the selection range. Maybe we could use similar > definition. > > http://doc.qt.nokia.com/latest/qt.html#InputMethodQuery-enum http://codereview.appspot.com/4844041/diff/15001/bus/engineproxy.c#newcode1071 bus/engineproxy.c:1071: cursor_pos != engine->surrounding_cursor_pos) { On 2011/08/10 12:05:21, Yusuke Sato wrote: > engine->selection_start_pos != selection_start_pos || > engine->selection_end_pos != selection_end_pos) { > > would be missing. > Done. http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:954: if(ibusimcontext->client_window == NULL) { On 2011/08/10 12:05:21, Yusuke Sato wrote: > space after 'if' Done. http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:955: *start = *end = *cursor = 0; On 2011/08/10 12:05:21, Yusuke Sato wrote: > how about doing this initialization at the beginning of the function and remove > 955, 961, 967, and 975? Done. http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:958: gdk_window_get_user_data(ibusimcontext->client_window, (gpointer)&widget); On 2011/08/10 12:05:21, Yusuke Sato wrote: > space before '(' Done. http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:960: if (!GTK_IS_TEXT_VIEW(widget)){ On 2011/08/10 12:05:21, Yusuke Sato wrote: > space Done. http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:961: *start = *end = *cursor = 0; On 2011/08/10 12:05:21, Yusuke Sato wrote: > ditto Done. http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:965: GtkTextView *text_view = GTK_TEXT_VIEW(widget); On 2011/08/10 12:05:21, Yusuke Sato wrote: > space Done. http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:966: GtkTextBuffer *buffer = gtk_text_view_get_buffer(text_view); On 2011/08/10 12:05:21, Yusuke Sato wrote: > space Done. http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:968: if (!gtk_text_buffer_get_has_selection(buffer)) { On 2011/08/10 12:05:21, Yusuke Sato wrote: > space Done. http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:969: *start = *end = *cursor = 0; On 2011/08/10 12:05:21, Yusuke Sato wrote: > ditto Done. http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:974: if (!gtk_text_buffer_get_selection_bounds(buffer, &start_iter, &end_iter)) { On 2011/08/10 12:05:21, Yusuke Sato wrote: > space Done. http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:975: *start = *end = *cursor = 0; On 2011/08/10 12:05:21, Yusuke Sato wrote: > ditto Done. http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:987: g_return_if_fail (*start <= *end); Sorry, for my understanding this validation never fail, because end_pos is always larger than start_pos. On 2011/08/10 12:05:21, Yusuke Sato wrote: > in this case (start is larger than end), i think we should return with *start = > *end = *cursor = 0. > > move this line to line 982? http://codereview.appspot.com/4844041/diff/15001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:1027: if (start_pos < 0 || end_pos > len) { On 2011/08/10 12:05:21, Yusuke Sato wrote: > integer underflow. this does not make sense since these variables are unsigned. > > something like this would be better: > > if (start_pos < relative_origin || > end_pos < relative_origin) { > ... > } > start_pos -= relative_origin; > end_pos -= relative_origin; > if (end_pos > len) { > ... > } > Done. http://codereview.appspot.com/4844041/diff/15001/src/ibusengine.c File src/ibusengine.c (right): http://codereview.appspot.com/4844041/diff/15001/src/ibusengine.c#newcode725 src/ibusengine.c:725: G_TYPE_UINT); On 2011/08/10 12:05:21, Yusuke Sato wrote: > G_TYPE_UINT); > > should be > > G_TYPE_UINT, > G_TYPE_UINT, > G_TYPE_UINT); > > > Done. http://codereview.appspot.com/4844041/diff/15001/src/ibusengine.c#newcode737 src/ibusengine.c:737: engine->priv->surrounding_cursor_pos = 0; On 2011/08/10 12:05:21, Yusuke Sato wrote: > 737-739 not necessary? Done. http://codereview.appspot.com/4844041/diff/15001/src/ibusengine.c#newcode1408 src/ibusengine.c:1408: gboolean signal_only = (text == NULL); On 2011/08/10 12:05:21, Yusuke Sato wrote: > const Done. http://codereview.appspot.com/4844041/diff/15001/src/ibusengine.c#newcode1421 src/ibusengine.c:1421: if (!signal_only) { On 2011/08/10 12:05:21, Yusuke Sato wrote: > why you removed cursor_pos check? is it safe? Yes, if signal_only is false, above g_return_if_fail functions guarantee arguments are not NULL. Above g_return_if_fail functions only pass all arguments are NULL or all arguments are not NULL. http://codereview.appspot.com/4844041/diff/15001/src/ibusengine.h File src/ibusengine.h (left): http://codereview.appspot.com/4844041/diff/15001/src/ibusengine.h#oldcode150 src/ibusengine.h:150: On 2011/08/10 12:05:21, Yusuke Sato wrote: > preserve this line if you have no reason to remove it. Sorry, I forget to revert this line. http://codereview.appspot.com/4844041/diff/15001/src/ibusinputcontext.c File src/ibusinputcontext.c (right): http://codereview.appspot.com/4844041/diff/15001/src/ibusinputcontext.c#newco... src/ibusinputcontext.c:471: priv->surrounding_cursor_pos = 0; On 2011/08/10 12:05:21, Yusuke Sato wrote: > remove 471-473? Done. http://codereview.appspot.com/4844041/diff/15001/src/ibusinputcontext.c#newco... src/ibusinputcontext.c:1070: cursor_pos != priv->surrounding_cursor_pos) { On 2011/08/10 12:05:21, Yusuke Sato wrote: > ditto. should check selection_{start,end}_pos in priv. Done. http://codereview.appspot.com/4844041/diff/15001/src/ibusinputcontext.h File src/ibusinputcontext.h (left): http://codereview.appspot.com/4844041/diff/15001/src/ibusinputcontext.h#oldco... src/ibusinputcontext.h:507: On 2011/08/10 12:05:21, Yusuke Sato wrote: > preserve the vertical space. Done.
Sign in to reply to this message.
Please update interfaces in python code too. Hi Daiki Ueno, This CL will modify some surrounding text related interfaces. If it is landed, I think you need update some code in ibus-m17n. Do you think it is OK? Thanks. http://codereview.appspot.com/4844041/diff/21001/src/ibusengine.c File src/ibusengine.c (right): http://codereview.appspot.com/4844041/diff/21001/src/ibusengine.c#newcode221 src/ibusengine.c:221: " <arg direction='in' type='u' name='selection_anchor_pos' />" I prefer anchor_pos, could you please rename all selection_anchor_pos to it? Thanks. http://codereview.appspot.com/4844041/diff/21001/src/ibusengine.c#newcode705 src/ibusengine.c:705: * @engine: An IBusEngine. The document does not have signal arguments. Please add them.
Sign in to reply to this message.
On 2011/08/10 18:32:41, Peng wrote: > This CL will modify some surrounding text related interfaces. If it is landed, I > think you need update some code in ibus-m17n. Do you think it is OK? Fine with me.
Sign in to reply to this message.
I added a couple of minor comments. https://codereview.appspot.com/4844041/diff/21001/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): https://codereview.appspot.com/4844041/diff/21001/client/gtk2/ibusimcontext.c... client/gtk2/ibusimcontext.c:959: gdk_window_get_user_data (ibusimcontext->client_window, (gpointer)&widget); gpointer -> gpointer *, I guess. https://codereview.appspot.com/4844041/diff/21001/src/ibusmarshalers.list File src/ibusmarshalers.list (right): https://codereview.appspot.com/4844041/diff/21001/src/ibusmarshalers.list#new... src/ibusmarshalers.list:16: VOID:OBJECT,UINT This line is no longer needed.
Sign in to reply to this message.
Thank you for reviewing! http://codereview.appspot.com/4844041/diff/21001/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): http://codereview.appspot.com/4844041/diff/21001/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:959: gdk_window_get_user_data (ibusimcontext->client_window, (gpointer)&widget); Yes, you are right. FIxed. On 2011/08/11 07:38:37, Daiki Ueno wrote: > gpointer -> gpointer *, I guess. http://codereview.appspot.com/4844041/diff/21001/src/ibusengine.c File src/ibusengine.c (right): http://codereview.appspot.com/4844041/diff/21001/src/ibusengine.c#newcode221 src/ibusengine.c:221: " <arg direction='in' type='u' name='selection_anchor_pos' />" On 2011/08/10 18:32:41, Peng wrote: > I prefer anchor_pos, could you please rename all selection_anchor_pos to it? > Thanks. Done except object member variable because of following precedent(and I also think it is good). http://codereview.appspot.com/4844041/diff/21001/src/ibusengine.c#newcode705 src/ibusengine.c:705: * @engine: An IBusEngine. On 2011/08/10 18:32:41, Peng wrote: > The document does not have signal arguments. Please add them. Done. http://codereview.appspot.com/4844041/diff/21001/src/ibusmarshalers.list File src/ibusmarshalers.list (right): http://codereview.appspot.com/4844041/diff/21001/src/ibusmarshalers.list#newc... src/ibusmarshalers.list:16: VOID:OBJECT,UINT On 2011/08/11 07:38:37, Daiki Ueno wrote: > This line is no longer needed. Done.
Sign in to reply to this message.
Sorry I wrongly upload with debug code. If you already reviewed, I'm really sorry. Patch set 5 is clean and up-to-date. Thank you. On 2011/08/11 08:08:17, nona1 wrote: > Thank you for reviewing! > > http://codereview.appspot.com/4844041/diff/21001/client/gtk2/ibusimcontext.c > File client/gtk2/ibusimcontext.c (right): > > http://codereview.appspot.com/4844041/diff/21001/client/gtk2/ibusimcontext.c#... > client/gtk2/ibusimcontext.c:959: gdk_window_get_user_data > (ibusimcontext->client_window, (gpointer)&widget); > Yes, you are right. > FIxed. > On 2011/08/11 07:38:37, Daiki Ueno wrote: > > gpointer -> gpointer *, I guess. > > http://codereview.appspot.com/4844041/diff/21001/src/ibusengine.c > File src/ibusengine.c (right): > > http://codereview.appspot.com/4844041/diff/21001/src/ibusengine.c#newcode221 > src/ibusengine.c:221: " <arg direction='in' type='u' > name='selection_anchor_pos' />" > On 2011/08/10 18:32:41, Peng wrote: > > I prefer anchor_pos, could you please rename all selection_anchor_pos to it? > > Thanks. > > Done except object member variable because of following precedent(and I also > think it is good). > > http://codereview.appspot.com/4844041/diff/21001/src/ibusengine.c#newcode705 > src/ibusengine.c:705: * @engine: An IBusEngine. > On 2011/08/10 18:32:41, Peng wrote: > > The document does not have signal arguments. Please add them. > > Done. > > http://codereview.appspot.com/4844041/diff/21001/src/ibusmarshalers.list > File src/ibusmarshalers.list (right): > > http://codereview.appspot.com/4844041/diff/21001/src/ibusmarshalers.list#newc... > src/ibusmarshalers.list:16: VOID:OBJECT,UINT > On 2011/08/11 07:38:37, Daiki Ueno wrote: > > This line is no longer needed. > > Done.
Sign in to reply to this message.
lgtm with some comments http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iengine.py File ibus/interface/iengine.py (right): http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iengine.py#ne... ibus/interface/iengine.py:53: @method(in_signature="vu") update in_signature http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iinputcontext.py File ibus/interface/iinputcontext.py (right): http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iinputcontext... ibus/interface/iinputcontext.py:52: @method(in_signature="vu") same http://codereview.appspot.com/4844041/diff/32001/src/ibusengine.c File src/ibusengine.c (right): http://codereview.appspot.com/4844041/diff/32001/src/ibusengine.c#newcode1146 src/ibusengine.c:1146: // g_debug ("set-surrounding-text ('%s', %d)", text->text, cursor_pos); could update it as well? http://codereview.appspot.com/4844041/diff/32001/src/ibusengine.c#newcode1427 src/ibusengine.c:1427: // g_debug ("get-surrounding-text ('%s', %d)", (*text)->text, *cursor_pos); same
Sign in to reply to this message.
Thank you! http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iengine.py File ibus/interface/iengine.py (right): http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iengine.py#ne... ibus/interface/iengine.py:53: @method(in_signature="vu") On 2011/08/11 11:09:57, Peng wrote: > update in_signature Done. http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iinputcontext.py File ibus/interface/iinputcontext.py (right): http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iinputcontext... ibus/interface/iinputcontext.py:52: @method(in_signature="vu") On 2011/08/11 11:09:57, Peng wrote: > same Done.
Sign in to reply to this message.
On 2011/08/11 11:18:41, nona1 wrote: > Thank you! > > http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iengine.py > File ibus/interface/iengine.py (right): > > http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iengine.py#ne... > ibus/interface/iengine.py:53: @method(in_signature="vu") > On 2011/08/11 11:09:57, Peng wrote: > > update in_signature > > Done. > > http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iinputcontext.py > File ibus/interface/iinputcontext.py (right): > > http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iinputcontext... > ibus/interface/iinputcontext.py:52: @method(in_signature="vu") > On 2011/08/11 11:09:57, Peng wrote: > > same > > Done. LGTM. nice work.
Sign in to reply to this message.
On 2011/08/11 15:01:30, Yusuke Sato wrote: > On 2011/08/11 11:18:41, nona1 wrote: > > Thank you! > > > > http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iengine.py > > File ibus/interface/iengine.py (right): > > > > > http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iengine.py#ne... > > ibus/interface/iengine.py:53: @method(in_signature="vu") > > On 2011/08/11 11:09:57, Peng wrote: > > > update in_signature > > > > Done. > > > > > http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iinputcontext.py > > File ibus/interface/iinputcontext.py (right): > > > > > http://codereview.appspot.com/4844041/diff/32001/ibus/interface/iinputcontext... > > ibus/interface/iinputcontext.py:52: @method(in_signature="vu") > > On 2011/08/11 11:09:57, Peng wrote: > > > same > > > > Done. > > LGTM. nice work. Great work. I have pushed this CL on your behalf. Please close this CL. Thanks.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
