Re: [PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)
On Thu, Jul 03, 2014 at 04:45:42PM +0800, Jactry Zeng wrote:
index 6f2d579..2553b50 100644 --- a/dlls/riched20/richole.c +++ b/dlls/riched20/richole.c @@ -783,11 +783,31 @@ static HRESULT WINAPI ITextSelection_fnInvoke( static HRESULT WINAPI ITextSelection_fnGetText(ITextSelection *me, BSTR *pbstr) { ITextSelectionImpl *This = impl_from_ITextSelection(me); + ME_Cursor *start = NULL, *end = NULL; + int nChars; + if (!This->reOle) return CO_E_RELEASED; + TRACE("%p\n", pbstr); + if (!pbstr) + return E_INVALIDARG;
- FIXME("not implemented\n"); - return E_NOTIMPL; + ME_GetSelection(This->reOle->editor, &start, &end); + nChars = ME_GetCursorOfs(end) - ME_GetCursorOfs(start); + if (!nChars) + { + *pbstr = NULL; + return S_OK; + } + + *pbstr = SysAllocStringLen(NULL, nChars); + if (!*pbstr) + return E_OUTOFMEMORY; + /* FIXME: a '\r' should be appended at the end of a story */ + ME_GetTextW(This->reOle->editor, *pbstr, nChars, start, nChars, 0);
Actually, perhaps you could fix the FIXME. GetTextW already has the ability to add \r\n, so extending this to add just \r shouldn't be too difficult. Also, in the tests, you leak the BSTRs. Huw.
Hello Huw, 2014-07-03 17:34 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
+ if (!*pbstr) + return E_OUTOFMEMORY; + /* FIXME: a '\r' should be appended at the end of a story */ + ME_GetTextW(This->reOle->editor, *pbstr, nChars, start, nChars, 0);
Actually, perhaps you could fix the FIXME. GetTextW already has the ability to add \r\n, so extending this to add just \r shouldn't be too difficult.
Thanks for your review first! A few weeks ago, I have made some tests about this problem. And it seem is an unimplement feature of Rich edit control and should not be fixed in ME_GetTextW or ITextSeletction::GetText simply: 1. end-of-story is different from end-of-paragraph
From document of ITextDocument::GetStoryCount: Rich edit controls have only one story.[1] So ‘story’ may is an unit like 'paragraph' in Rich edit control. We may need add a MERF_ENDSTORY to implement it.[2]
2. What does the end-of-story looks like? When we run the wordpad with native riched20, we can use ‘ctrl + shitf + →’ to select the ‘space’ at the end of the Rich edit control. But we cannot set the cursor behind it.(a picture here[3]) And wine’s riched20 didn’t implement this ‘space’. This 'space' may is the end-of-story. A crash will happen when we copy the ‘space’ from a native Rich edit control to a builtin one.(backtrace.txt[4]) BTW, there is a know bug of EM_SETSEL: cannot select the last character of Rich edit control.[5] 3.
From the last paragraph of this blog: http://blogs.msdn.com/b/murrays/archive/2008/11/22/paragraphs-and-paragraph-... It is sure there is a final EOP in a Rich edit control.
There seems to be every indication that we should implement the final EOP of Rich edit control rather than just append a ‘\r’ in ITextSelection::GetText. So I just let a FIXME comment there. I am not sure if I am right. Do you have any suggestion?
Also, in the tests, you leak the BSTRs.
Thanks again.
Huw.
-- Regards, Jactry Zeng
On Fri, Jul 04, 2014 at 07:00:16PM +0800, Jactry Zeng wrote:
A few weeks ago, I have made some tests about this problem. And it seem is an unimplement feature of Rich edit control and should not be fixed in ME_GetTextW or ITextSeletction::GetText simply:
1. end-of-story is different from end-of-paragraph From document of ITextDocument::GetStoryCount: Rich edit controls have only one story.[1] So ‘story’ may is an unit like 'paragraph' in Rich edit control. We may need add a MERF_ENDSTORY to implement it.[2]
2. What does the end-of-story looks like? When we run the wordpad with native riched20, we can use ‘ctrl + shitf + →’ to select the ‘space’ at the end of the Rich edit control. But we cannot set the cursor behind it.(a picture here[3]) And wine’s riched20 didn’t implement this ‘space’. This 'space' may is the end- of-story.
A crash will happen when we copy the ‘space’ from a native Rich edit control to a builtin one.(backtrace.txt[4])
BTW, there is a know bug of EM_SETSEL: cannot select the last character of Rich edit control.[5]
3. From the last paragraph of this blog: http://blogs.msdn.com/b/murrays/archive/ 2008/11/22/paragraphs-and-paragraph-formatting.aspx It is sure there is a final EOP in a Rich edit control.
There seems to be every indication that we should implement the final EOP of Rich edit control rather than just append a ‘\r’ in ITextSelection::GetText. So I just let a FIXME comment there. I am not sure if I am right. Do you have any suggestion?
Well first of all, if there are bugs with the underlying richedit, these need fixing first. Note, that every richedit consists of at least one paragraph (and hence one final EOP). I was suggesting adding to ME_GetTextW the ability to append a '\r' if it were passed an appropriate flag, of course this wouldn't be the behaviour used for its current callers. There's already a diTextEnd item in the display list to mark the end of the document, so there should be no need to add a new end-of-story marker. Most of the iterator functions stop when this is reached, so you tend not to actually see it, but that shouldn't be a problem. Huw.
Hi Huw, diTextEnd is useful for append a '\r' to the last paragraph. But because of bug of EM_SETSEL or EM_GETSEL, this feature still cannot be implemented completely. For example: When the text in Rich Edit control is: "TestSomeText" 01234567890123 We cannot distinguish between SendMessageA(w, EM_SETSEL, 0, 12) and SendMessageA(w, EM_SETSEL, 0, 13). Attachment is the patch, I just added wine_todo for failed tests cause by this bug. But I still have no idea about fixing this bug. Could you give me some suggestion? Or just mark them as todo now is OK? Thank you! 2014-07-07 16:38 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
On Fri, Jul 04, 2014 at 07:00:16PM +0800, Jactry Zeng wrote:
A few weeks ago, I have made some tests about this problem. And it seem is an unimplement feature of Rich edit control and should not be fixed in ME_GetTextW or ITextSeletction::GetText simply:
1. end-of-story is different from end-of-paragraph From document of ITextDocument::GetStoryCount: Rich edit controls have only one story.[1] So ‘story’ may is an unit like 'paragraph' in Rich edit control. We may need add a MERF_ENDSTORY to implement it.[2]
2. What does the end-of-story looks like? When we run the wordpad with native riched20, we can use ‘ctrl + shitf + →’ to select the ‘space’ at the end of the Rich edit control. But we cannot set the cursor behind it.(a picture here[3]) And wine’s riched20 didn’t implement this ‘space’. This 'space' may is the end- of-story.
A crash will happen when we copy the ‘space’ from a native Rich edit control to a builtin one.(backtrace.txt[4])
BTW, there is a know bug of EM_SETSEL: cannot select the last character of Rich edit control.[5]
3. From the last paragraph of this blog: http://blogs.msdn.com/b/murrays/archive/ 2008/11/22/paragraphs-and-paragraph-formatting.aspx It is sure there is a final EOP in a Rich edit control.
There seems to be every indication that we should implement the final EOP of Rich edit control rather than just append a ‘\r’ in ITextSelection::GetText. So I just let a FIXME comment there. I am not sure if I am right. Do you have any suggestion?
Well first of all, if there are bugs with the underlying richedit, these need fixing first. Note, that every richedit consists of at least one paragraph (and hence one final EOP).
I was suggesting adding to ME_GetTextW the ability to append a '\r' if it were passed an appropriate flag, of course this wouldn't be the behaviour used for its current callers. There's already a diTextEnd item in the display list to mark the end of the document, so there should be no need to add a new end-of-story marker. Most of the iterator functions stop when this is reached, so you tend not to actually see it, but that shouldn't be a problem.
Huw.
-- Regards, Jactry Zeng
On 15 Jul 2014, at 02:18, Jactry Zeng wrote:
But because of bug of EM_SETSEL or EM_GETSEL, this feature still cannot be implemented completely. For example: When the text in Rich Edit control is: "TestSomeText" 01234567890123 We cannot distinguish between SendMessageA(w, EM_SETSEL, 0, 12) and SendMessageA(w, EM_SETSEL, 0, 13). Attachment is the patch, I just added wine_todo for failed tests cause by this bug. But I still have no idea about fixing this bug. Could you give me some suggestion? Or just mark them as todo now is OK?
Do you fully understand this bug yet? In other words under what conditions does the current implementation fail? It should be trivial to fix up ME_SetSelection(). Huw.
Hi, Thanks for your reply. I am not sure if my understanding is right or not... + ME_GetSelection(This->reOle->editor, &start, &end); + nChars = ME_GetCursorOfs(end) - ME_GetCursorOfs(start); + bEOP = (end->pRun->type == diTextEnd); Here I would like to use ME_GetSelection() to get the end of the selection range. And than judge if it is the end of the text buffer and pass bEOP into ME_GetTextW. If bEOP is true, ME_GetTextW will append a '\r' in the end of the return string. But because of bug of ME_GetSelection/ME_SetSelection, I can't got a ME_Cursor of the end of the text buffer. 2014-07-15 17:23 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
On 15 Jul 2014, at 02:18, Jactry Zeng wrote:
But because of bug of EM_SETSEL or EM_GETSEL, this feature still cannot be implemented completely. For example: When the text in Rich Edit control is: "TestSomeText" 01234567890123 We cannot distinguish between SendMessageA(w, EM_SETSEL, 0, 12) and SendMessageA(w, EM_SETSEL, 0, 13). Attachment is the patch, I just added wine_todo for failed tests cause by this bug. But I still have no idea about fixing this bug. Could you give me some suggestion? Or just mark them as todo now is OK?
Do you fully understand this bug yet? In other words under what conditions does the current implementation fail? It should be trivial to fix up ME_SetSelection().
Huw.
-- Regards, Jactry Zeng
On 15 Jul 2014, at 13:51, Jactry Zeng wrote:
Thanks for your reply. I am not sure if my understanding is right or not...
+ ME_GetSelection(This->reOle->editor, &start, &end); + nChars = ME_GetCursorOfs(end) - ME_GetCursorOfs(start); + bEOP = (end->pRun->type == diTextEnd);
Here I would like to use ME_GetSelection() to get the end of the selection range. And than judge if it is the end of the text buffer and pass bEOP into ME_GetTextW. If bEOP is true, ME_GetTextW will append a '\r' in the end of the return string. But because of bug of ME_GetSelection/ME_SetSelection, I can't got a ME_Cursor of the end of the text buffer.
My point is, we should fix the bug in S/GetSelection first. There's no point in working around this bug. Huw.
2014-07-15 21:00 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
My point is, we should fix the bug in S/GetSelection first. There's no
point in working around this bug. I still have no idea about this bug. :( Can I just let a FIXME there now? So I can continue work of ITextSelection and ITextRange. -- Regards, Jactry Zeng
On 15 Jul 2014, at 14:13, Jactry Zeng wrote:
2014-07-15 21:00 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
My point is, we should fix the bug in S/GetSelection first. There's no point in working around this bug.
I still have no idea about this bug. :( Can I just let a FIXME there now? So I can continue work of ITextSelection and ITextRange.
No, as whatever you write will need fixing once the underlying bug is fixed. Can you at least point to a test that shows this bug? Huw.
2014-07-15 21:29 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
Can you at least point to a test that shows this bug?
Do you mean tests about bug of S/GetSelection? It seem is a known bug: http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/riched20/tests/editor.... dlls/riched20/tests/editor.c:4352 struct exsetsel_s { LONG min; LONG max; LRESULT expected_retval; int expected_getsel_start; int expected_getsel_end; int _getsel_todo_wine; }; const struct exsetsel_s exsetsel_tests[] = { /* sanity tests */ {5, 10, 10, 5, 10, 0}, {15, 17, 17, 15, 17, 0}, /* test cpMax > strlen() */ {0, 100, 18, 0, 18, 1}, /* test cpMin == cpMax */ {5, 5, 5, 5, 5, 0}, /* test cpMin < 0 && cpMax >= 0 (bug 4462) */ {-1, 0, 5, 5, 5, 0}, {-1, 17, 5, 5, 5, 0}, {-1, 18, 5, 5, 5, 0}, /* test cpMin < 0 && cpMax < 0 */ {-1, -1, 17, 17, 17, 0}, {-4, -5, 17, 17, 17, 0}, /* test cMin >=0 && cpMax < 0 (bug 6814) */ {0, -1, 18, 0, 18, 1}, {17, -5, 18, 17, 18, 1}, {18, -3, 17, 17, 17, 0}, /* test if cpMin > cpMax */ {15, 19, 18, 15, 18, 1}, {19, 15, 18, 15, 18, 1} }; -- Regards, Jactry Zeng
On 15 Jul 2014, at 14:43, Jactry Zeng wrote: 2014-07-15 21:29 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
Can you at least point to a test that shows this bug?
Do you mean tests about bug of S/GetSelection? It seem is a known bug: http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/riched20/tests/editor....
Yes. So adding a special case to SetSelection should fix this. 'to' is already setup correctly to len+1, so the problem is adjusting cursors[0]. MoveCursorChars is clamping the cursor to len. So you have two options, either alter MoveCursorChars to allow it to move to len + 1. You'd need to retain the current behaviour in some circumstances though (such as moving to the right on right-arrow keys press), so you'd need to pass a flag or something. Option 2 is just the modify cursor[0] in SetSelection. If this is the only place where one can select the final EOP, then this is probably the preferred solution. Either way, you probably need to change the 'middle of end paragraph run' fix up for cursor[0] at the end of SetSelection (probably by setting nOffset = run.len). Having done this, check the tests still work (and the todos are fixed). Also play with wordpad for a while to see it behaves correctly. Does that make sense? Huw.
2014-07-16 19:44 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
Yes. So adding a special case to SetSelection should fix this.
'to' is already setup correctly to len+1, so the problem is adjusting cursors[0]. MoveCursorChars is clamping the cursor to len.
So you have two options, either alter MoveCursorChars to allow it to move to len + 1. You'd need to retain the current behaviour in some circumstances though (such as moving to the right on right-arrow keys press), so you'd need to pass a flag or something.
Option 2 is just the modify cursor[0] in SetSelection. If this is the only place where one can select the final EOP, then this is probably the preferred solution.
Either way, you probably need to change the 'middle of end paragraph run' fix up for cursor[0] at the end of SetSelection (probably by setting nOffset = run.len).
Having done this, check the tests still work (and the todos are fixed). Also play with wordpad for a while to see it behaves correctly.
Does that make sense?
Thank you! I will have a try. -- Regards, Jactry Zeng
Hi Huw, Is this the right way? Testbot result: https://testbot.winehq.org/JobDetails.pl?Key=8030 2014-07-16 19:44 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
On 15 Jul 2014, at 14:43, Jactry Zeng wrote: 2014-07-15 21:29 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
Can you at least point to a test that shows this bug?
Do you mean tests about bug of S/GetSelection? It seem is a known bug:
http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/riched20/tests/editor....
Yes. So adding a special case to SetSelection should fix this.
'to' is already setup correctly to len+1, so the problem is adjusting cursors[0]. MoveCursorChars is clamping the cursor to len.
So you have two options, either alter MoveCursorChars to allow it to move to len + 1. You'd need to retain the current behaviour in some circumstances though (such as moving to the right on right-arrow keys press), so you'd need to pass a flag or something.
Option 2 is just the modify cursor[0] in SetSelection. If this is the only place where one can select the final EOP, then this is probably the preferred solution.
Either way, you probably need to change the 'middle of end paragraph run' fix up for cursor[0] at the end of SetSelection (probably by setting nOffset = run.len).
Having done this, check the tests still work (and the todos are fixed). Also play with wordpad for a while to see it behaves correctly.
Does that make sense?
Huw.
-- Regards, Jactry Zeng
On Sat, Jul 19, 2014 at 01:08:19AM +0800, Jactry Zeng wrote:
Hi Huw,
Is this the right way?
@@ -199,7 +205,9 @@ int ME_SetSelection(ME_TextEditor *editor, int from, int to) /* Selection is not allowed in the middle of an end paragraph run. */ if (editor->pCursors[1].pRun->member.run.nFlags & MERF_ENDPARA) editor->pCursors[1].nOffset = 0; - if (editor->pCursors[0].pRun->member.run.nFlags & MERF_ENDPARA) + if (editor->pCursors[0].pRun->member.run.nFlags & MERF_ENDPARA && to > len) + editor->pCursors[0].nOffset = editor->pCursors[0].pRun->member.run.len; + else if (editor->pCursors[0].pRun->member.run.nFlags & MERF_ENDPARA) editor->pCursors[0].nOffset = 0; return to; }
Looks ok apart from this last hunk. It would be cleaner to test the nFlags once and have a nested if/else for the to > len case. Huw.
On Sat, Jul 19, 2014 at 01:08:19AM +0800, Jactry Zeng wrote:
diff --git a/dlls/riched20/caret.c b/dlls/riched20/caret.c index 90b30a2..6aa16e9 100644 --- a/dlls/riched20/caret.c +++ b/dlls/riched20/caret.c @@ -140,6 +140,7 @@ int ME_SetSelection(ME_TextEditor *editor, int from, int to) ME_SetCursorToStart(editor, &editor->pCursors[1]); ME_SetCursorToEnd(editor, &editor->pCursors[0]); ME_InvalidateSelection(editor); + editor->pCursors[0].nOffset = editor->pCursors[0].pRun->member.run.len; return len + 1; }
Sorry, one more issue. I would expect this cursor fix up needs to be before the invalidate call. Was there a reason you put it after? Huw.
2014-07-21 17:21 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
Sorry, one more issue. I would expect this cursor fix up needs to be before the invalidate call. Was there a reason you put it after?
No, you are right, I made a mistake here. Thanks for your review! -- Regards, Jactry Zeng
2014-07-04 19:00 GMT+08:00 Jactry Zeng <jactry92(a)gmail.com>:
1. end-of-story is different from end-of-paragraph From document of ITextDocument::GetStoryCount: Rich edit controls have
only one story.[1]
So ‘story’ may is an unit like 'paragraph' in Rich edit control. We may need add a MERF_ENDSTORY to implement it.[2]
2. What does the end-of-story looks like? When we run the wordpad with native riched20, we can use ‘ctrl + shitf + →’ to select the ‘space’ at the end of the Rich edit control. But we cannot set the cursor behind it.(a picture here[3]) And wine’s riched20 didn’t implement this ‘space’. This 'space' may is the end-of-story.
A crash will happen when we copy the ‘space’ from a native Rich edit control to a builtin one.(backtrace.txt[4])
BTW, there is a know bug of EM_SETSEL: cannot select the last character of Rich edit control.[5]
3. From the last paragraph of this blog: http://blogs.msdn.com/b/murrays/archive/2008/11/22/paragraphs-and-paragraph-... It is sure there is a final EOP in a Rich edit control.
There seems to be every indication that we should implement the final EOP of Rich edit control rather than just append a ‘\r’ in ITextSelection::GetText. So I just let a FIXME comment there. I am not sure if I am right.
I missed some links here: [1] http://msdn.microsoft.com/en-us/library/windows/desktop/bb774027(v=vs.85).as... [2] http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/riched20/editstr.h#l11... [3] http://img.vim-cn.com/6f/dbc090928d97b2cdf53838278087fa95660b9b.png [4] https://gist.github.com/Jactry/cae7d739162fca4f072f [5] http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/riched20/tests/editor.... -- Regards, Jactry Zeng
participants (2)
-
Huw Davies -
Jactry Zeng