Closed
Bug 353776
Opened 18 years ago
Closed 15 years ago
need the surrounding text support for some language input
Categories
(Core :: Widget: Gtk, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a4
People
(Reporter: akira, Assigned: thep)
References
(Blocks 1 open bug, )
Details
(Keywords: intl)
Attachments
(2 files, 6 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; ja; rv:1.8.0.6) Gecko/20060808 Fedora/1.5.0.6-2.fc5 Firefox/1.5.0.6 pango-text Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; ja; rv:1.8.0.6) Gecko/20060808 Fedora/1.5.0.6-2.fc5 Firefox/1.5.0.6 pango-text Right now firefox/thunderbird doesn't deal with Input Method properly that uses the surrounding text signals from GTK+. then the unexpected characters appears in the text areas. Reproducible: Always Steps to Reproduce: 1.try to input the characters through Input Method that uses the surrounding text signals, such as scim-sinhala and scim-m17n. 2. 3. Actual Results: broken characters appears Expected Results: should shows the proper characters.
Reporter | ||
Comment 1•18 years ago
|
||
Reporter | ||
Updated•18 years ago
|
Version: unspecified → 1.5.0.x Branch
Assignee | ||
Comment 2•17 years ago
|
||
This bug applies to Thai XIM, too. (Try with XMODIFIERS="@im=BasicCheck" under th_TH locale.) I try to test your patch, but it seems to need some update to apply to current trunk.
Comment 3•17 years ago
|
||
BugAThon Thailand on Firefox 3 Beta 5pre, Mac OS X and Windows also has this problem. On Mac OS X, this behavior is consistent with native apps. On Linux, depends on IM system the user used. On Linux, the user can notice this very explicitly, visually. On Mac OS X, the user can notice this as well, but the difference is very marginal, have to carefully spot it. On Windows, the user cannot notice this at all, visually.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: intl
OS: Linux → All
Version: 1.5.0.x Branch → Trunk
Assignee | ||
Comment 4•17 years ago
|
||
(In reply to comment #3) > On Linux, the user can notice this very explicitly, visually. > On Mac OS X, the user can notice this as well, but the difference is very > marginal, have to carefully spot it. > On Windows, the user cannot notice this at all, visually. Please be more specific on how it's visually noticed. By means of special text rendering or what?
Assignee | ||
Comment 5•15 years ago
|
||
I'd like this bug to be fixed, as it's one of the most requested feature for Thai users, next to line wrapping. The fix would obviously benefit related languages like Lao, Sinhala as well. So, I've updated Akira's patch to apply to trunk again.
Assignee | ||
Comment 6•15 years ago
|
||
My previous patch has a flaw which broke multi-line text area. It returned the whole text buffer like what was done in Akira's original patch, but I chose the API for the cursor position that returned the offset in the current node (paragraph), instead of relative to the whole buffer: +NS_IMETHODIMP +nsPlaintextEditor::RetrieveSurrounding(nsTextEventReply* aReply) +{ + nsresult result; + + nsCOMPtr<nsISelection> selection; + result = GetSelection(getter_AddRefs(selection)); + if (NS_FAILED(result)) + return result; + + OutputToString(NS_LITERAL_STRING("text/plain"), + nsIDocumentEncoder::OutputLFLineBreak | + nsIDocumentEncoder::OutputBodyOnly | + nsIDocumentEncoder::OutputRaw, + aReply->theText); + + nsCOMPtr<nsIDOMNode> startNode; + result = GetStartNodeAndOffset(selection, + address_of(startNode), + &aReply->cursorPos); + + return result; +} So, I've adjusted it to also retrive text of current node, instead of the whole buffer: +NS_IMETHODIMP +nsPlaintextEditor::RetrieveSurrounding(nsTextEventReply* aReply) +{ + nsresult result; + + nsCOMPtr<nsISelection> selection; + result = GetSelection(getter_AddRefs(selection)); + if (NS_FAILED(result)) + return result; + + nsCOMPtr<nsIDOMNode> startNode; + result = GetStartNodeAndOffset(selection, + address_of(startNode), + &aReply->cursorPos); + if (NS_FAILED(result)) + return result; + + result = startNode->GetNodeValue(aReply->theText); + + return result; +} Now it works in multi-line text area, as well as multi-line editable HTML.
Attachment #387614 -
Attachment is obsolete: true
Assignee | ||
Comment 7•15 years ago
|
||
Comment on attachment 389088 [details] [diff] [review] Updated patch, fixing multi-line case As I've tested the patch for a while, can I request for a review?
Attachment #389088 -
Flags: review?(peterv)
Comment 8•15 years ago
|
||
Adding Masayuki Nakano, Mozilla's i18n expert.
Comment 9•15 years ago
|
||
The approach isn't good, we should use query content event to get the surrounding text. And also you can use set selection event and content command event to delete the text. So, you should be able to fix this bug by changing only nsWindow.
Comment 10•15 years ago
|
||
Comment on attachment 389088 [details] [diff] [review] Updated patch, fixing multi-line case -'ing per comment 9.
Attachment #389088 -
Flags: review?(peterv) → review-
Assignee | ||
Comment 11•15 years ago
|
||
Thanks for the advice. Now I'm getting back to it. I've got this first patch so far. It's not complete yet. Retrieving surrounding works, but deleting does not. From my investigation, it fails in nsWindow::IMEDeleteText(). NS_SELECTION_SET event returns an error, with these debug messages: ---8<--- WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /home/thep/vcs/mozilla_hg/mozilla-central-imsurr2/content/events/src/nsContentEventHandler.cpp, line 883 WARNING: NS_ENSURE_TRUE(selectionEvent.mSucceeded) failed: file /home/thep/vcs/mozilla_hg/mozilla-central-imsurr2/widget/src/gtk2/nsWindow.cpp, line 7081 ---8<--- So, it fails in nsContentEventHandler::OnSelectionEvent(), on return from nsIMEStateManager::GetFocusSelectionAndRoot(). Checking nsIMEStateManager::GetFocusSelectionAndRoot(), I find it fails because nsIMEStateManager::sTextStateObserver == NULL, that is, there is no text currently gets the focus. How does this happen? Do I miss some step? Note that I have also tried inserting this code before dispatching the NS_SELECTION_SET event, but it doesn't help: ---8<--- nsWindow *containerWindow = GetContainerWindow(); if (!gFocusWindow && containerWindow) { containerWindow->DispatchActivateEvent(); } ---8<---
Comment 12•15 years ago
|
||
Note I'll separate the IME code from nsWindow, see bug 520732. Please wait the fix, I'm testing the patch now. (In reply to comment #11) > From my investigation, it fails in nsWindow::IMEDeleteText(). NS_SELECTION_SET > event returns an error, with these debug messages: Maybe, it's a bug of I found in bug 528396. Cannot fix the bug by the patch of bug 528396?
Assignee | ||
Comment 13•15 years ago
|
||
Yes, the bug is gone with your patch in bug 528396. Now surrounding characters are deleted when requested. However, the amount it deletes is not what expected. With select-and-delete scheme, text is deleted cluster-wise, rather than character-wise as expected by input methods. For normal text editing, cluster-wise deletion is expected (see bug 157546). But for IME, the correction can be combining mark substitution or alike, which needs finer access than cluster.
Comment 14•15 years ago
|
||
Ah, it needs to extend the nsContentHandler and the related events.
Assignee | ||
Comment 15•15 years ago
|
||
OK. In this patch, I have added 'mExpandToClusterBoundary' member to nsSelectionEvent, with default value set to true to prevent brekage on other code. Now it works with Thai context-sensitive IME. Note that the patch is to be applied on top of your patch in bug 528396 (attachment 415380 [details] [diff] [review]).
Attachment #417894 -
Attachment is obsolete: true
Comment 16•15 years ago
|
||
bug 520732 and bug 528396 are fixed. you can create the patch right now.
Assignee | ||
Comment 17•15 years ago
|
||
Patch updated according to the GtkIMModule split. Please review.
Attachment #389088 -
Attachment is obsolete: true
Attachment #418540 -
Attachment is obsolete: true
Attachment #434167 -
Flags: review?(masayuki)
Comment 18•15 years ago
|
||
Please make OnRetrieveSurroundingNative() and OnDeleteSurroundingNative() methods and you should process in there. That frees you and other developers from |aModule->|. And in them, check aContext is same as the result of GetContext(). If they are not matched, you should do nothing and just return false. Please define the stack variable near the first used line. The C style definition could cause unused variable. Please add PR_LOG()s to the new methods like other methods. It helps us to debugging without the problematic IME. > +nsresult > +nsGtkIMModule::GetText(PRUnichar*& aText, PRInt32& aLength, PRInt32& aCursorPos) Use nsAString& instead of aText and aLength. And the caller should send nsAutoString instance to this method. > + aCursorPos = querySelectedTextEvent.mReply.mOffset; mOffset is unsigned, but aCursorPos is signed. You need to check whether (mOffset <= PR_INT32MAX) or not. > + queryTextContentEvent.InitForQueryTextContent( > + 0, aCursorPos + querySelectedTextEvent.mReply.mString.Length() + 32 > + ); Length() is PRUint32, so, you need to check here too. And I'm not sure whether this is correct or not. If you should return an entire paragraph which has cursor, you can look the example in the code for Windows. See nsIMM32Handler::HandleDocumentFeed(): http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsIMM32Handler.cpp#1070 Note that if the focused editor is HTML editor, the text contents are too big. So, if you change the behavior to such, GetText() should be GetCurrentParagraph() or something. > +gboolean > +nsGtkIMModule::OnRetrieveSurroundingCallback(GtkIMContext *aContext, > + nsGtkIMModule *aModule) > + utf8_str[wbytes] = 0; > + p = utf8_str; > + while (cursorPos > 0 && *p != 0) { > + p = g_utf8_next_char(p); > + cursorPos--; > + } How about this? > utf8_str_until_cursor = > g_utf16_to_utf8((const gunichar2 *)uniStr, cursor, NULL, &cursorInUTF8, NULL); Otherwise, look ok. I have a worry. Do Undo/Redo commands work expectantly for you? NS_CONTENT_COMMAND_DELETE might separate a transaction as two ones. If it's so, it should be another bug. And I guess it's not important.
Assignee | ||
Comment 19•15 years ago
|
||
(In reply to comment #18) > Please make OnRetrieveSurroundingNative() and OnDeleteSurroundingNative() > methods and you should process in there. That frees you and other developers > from |aModule->|. > > And in them, check aContext is same as the result of GetContext(). If they are > not matched, you should do nothing and just return false. > > Please define the stack variable near the first used line. The C style > definition could cause unused variable. > > Please add PR_LOG()s to the new methods like other methods. It helps us to > debugging without the problematic IME. OK. All are done. > > +nsresult > > +nsGtkIMModule::GetText(PRUnichar*& aText, PRInt32& aLength, PRInt32& aCursorPos) > > Use nsAString& instead of aText and aLength. And the caller should send > nsAutoString instance to this method. Also done. > > + aCursorPos = querySelectedTextEvent.mReply.mOffset; > > mOffset is unsigned, but aCursorPos is signed. You need to check whether > (mOffset <= PR_INT32MAX) or not. I make aCursorPos unsigned instead. It can be. > > + queryTextContentEvent.InitForQueryTextContent( > > + 0, aCursorPos + querySelectedTextEvent.mReply.mString.Length() + 32 > > + ); > > Length() is PRUint32, so, you need to check here too. All are made unsigned anyway. > And I'm not sure whether this is correct or not. If you should return an entire > paragraph which has cursor, you can look the example in the code for Windows. > See nsIMM32Handler::HandleDocumentFeed(): > http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsIMM32Handler.cpp#1070 > > Note that if the focused editor is HTML editor, the text contents are too big. > > So, if you change the behavior to such, GetText() should be > GetCurrentParagraph() or something. OK. I've imitated the code in GtkIMModule, and tested it. > > +gboolean > > +nsGtkIMModule::OnRetrieveSurroundingCallback(GtkIMContext *aContext, > > + nsGtkIMModule *aModule) > > > + utf8_str[wbytes] = 0; > > + p = utf8_str; > > + while (cursorPos > 0 && *p != 0) { > > + p = g_utf8_next_char(p); > > + cursorPos--; > > + } > > How about this? > > > utf8_str_until_cursor = > > g_utf16_to_utf8((const gunichar2 *)uniStr, cursor, NULL, &cursorInUTF8, NULL); I use this instead: gtk_im_context_set_surrounding( aContext, utf8_str, wbytes, g_utf8_offset_to_pointer(utf8_str, cursorPos) - utf8_str ); > Otherwise, look ok. I have a worry. Do Undo/Redo commands work expectantly for > you? NS_CONTENT_COMMAND_DELETE might separate a transaction as two ones. If > it's so, it should be another bug. And I guess it's not important. I'm not sure about this. Never thought about it before. For me, it's OK. But users may think differently. Updated patch is coming soon.
Assignee | ||
Comment 20•15 years ago
|
||
Updated patch. Please review again. Thanks.
Attachment #434167 -
Attachment is obsolete: true
Attachment #434238 -
Flags: review?(masayuki)
Attachment #434167 -
Flags: review?(masayuki)
Comment 21•15 years ago
|
||
> + if (NS_FAILED(GetCurrentParagraph(uniStr, cursorPos))) > + return FALSE; Use {}. > + gtk_im_context_set_surrounding( > + aContext, utf8_str, wbytes, > + g_utf8_offset_to_pointer(utf8_str, cursorPos) - utf8_str > + ); gtk_im_context_set_surrounding(aContext, utf8_str, wbytes, g_utf8_offset_to_pointer(utf8_str, cursorPos) - utf8_str); > + return DeleteText(aOffset, (PRUint32)aNChars) == NS_OK; If the result is NS_ERROR_* by NS_ENSURE_*(), any logs are not recorded, please calls PR_LOG() here if DeleteText() is failed. So, if (NS_SUCCEEDED(DeleteText(...))) { return TRUE; } PR_LOG(...); return FALSE;
Assignee | ||
Comment 22•15 years ago
|
||
(In reply to comment #21) > > + if (NS_FAILED(GetCurrentParagraph(uniStr, cursorPos))) > > + return FALSE; > > Use {}. I found {} omissions for the case of single 'return' elsewhere in the source tree. But I'm following GtkIMModule style as suggested BTW. Other two are also corrected as suggested.
Attachment #434238 -
Attachment is obsolete: true
Attachment #434474 -
Flags: review?(masayuki)
Attachment #434238 -
Flags: review?(masayuki)
Comment 23•15 years ago
|
||
Comment on attachment 434474 [details] [diff] [review] Updated patch as commented Great! Thank you for your work. Can you land the patch to tree? Or should I do it? If so, I'll do it tonight (JST). (In reply to comment #22) > Created an attachment (id=434474) [details] > Updated patch as commented > > (In reply to comment #21) > > > + if (NS_FAILED(GetCurrentParagraph(uniStr, cursorPos))) > > > + return FALSE; > > > > Use {}. > > I found {} omissions for the case of single 'return' elsewhere in the source > tree. But I'm following GtkIMModule style as suggested BTW. The rule was changed, so, old code doesn't have the {} in many points, see https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Control_Structures
Attachment #434474 -
Flags: review?(masayuki) → review+
Updated•15 years ago
|
Assignee: nobody → thep
Component: General → Widget: Gtk
Product: Firefox → Core
QA Contact: general → gtk
Target Milestone: --- → mozilla1.9.3a4
Assignee | ||
Comment 24•15 years ago
|
||
(In reply to comment #23) > Great! Thank you for your work. Can you land the patch to tree? Or should I do > it? If so, I'll do it tonight (JST). I don't have the commit right. So, please do it. Thanks. > (In reply to comment #22) > > I found {} omissions for the case of single 'return' elsewhere in the source > > tree. But I'm following GtkIMModule style as suggested BTW. > > The rule was changed, so, old code doesn't have the {} in many points, see > https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Control_Structures Yes, I had also checked this page before deciding to follow your suggestion at last. Thanks a lot for your reviews!
Comment 25•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3d350995be5d landed, thank you.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 26•15 years ago
|
||
Theppitak, Masayuki, thank you for your good collaboration and contributions here. Once we have nightly builds for testing, could we have the Thai people on this bug please promote testing of this patch via blogs in Thailand, etc. so we can have a wide group of Thai users to test this before it goes into Firefox?
Assignee | ||
Comment 27•15 years ago
|
||
I've just blogged about this (in Thai): http://thep.blogspot.com/2010/03/mozilla-ime-surrounding-text-support.html Another related bug is Bug 425900. It should already be gone. Needs verification as well. (In fact, it should have been marked as a duplicate of this bug in the first place, but I didn't have the permission to do so.)
Comment 28•15 years ago
|
||
tested with 20100329 nightly build on Mac OS, looks like the patch doesn't integrated yet.
Comment 29•15 years ago
|
||
(In reply to comment #28) > tested with 20100329 nightly build on Mac OS, > looks like the patch doesn't integrated yet. The patch is only for Linux. If you have found a bug on Mac, please file a new bug.
Updated•15 years ago
|
OS: All → Linux
Comment 30•14 years ago
|
||
Arthit, does the problem still occur on Mac for you? If so, can you file a bug for it?
You need to log in
before you can comment on or make changes to this bug.
Description
•