Re: [PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Huw Davies]

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.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Jactry Zeng]

Hello Huw,
2014-07-03 17:34 GMT+08:00 Huw Davies :
> > - 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.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Huw Davies]

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:
> - 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]
> - 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]
> -
>
> 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.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Jactry Zeng]

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 :
> 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:
> > - 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]
> > - 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]
> > -
> >
> > 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.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Huw Davies]

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.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Jactry Zeng]

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 :
> 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.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Huw Davies]

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.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Jactry Zeng]

2014-07-15 21:00 GMT+08:00 Huw Davies :
> 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.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Huw Davies]

On 15 Jul 2014, at 14:13, Jactry Zeng wrote:
> 2014-07-15 21:00 GMT+08:00 Huw Davies :
> > 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.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Jactry Zeng]

2014-07-15 21:29 GMT+08:00 Huw Davies :
> 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}
};

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Huw Davies]

On 15 Jul 2014, at 14:43, Jactry Zeng wrote:
2014-07-15 21:29 GMT+08:00 Huw Davies :
> > 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.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Jactry Zeng]

2014-07-16 19:44 GMT+08:00 Huw Davies :
> 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.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[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 :
> On 15 Jul 2014, at 14:43, Jactry Zeng wrote:
> 2014-07-15 21:29 GMT+08:00 Huw Davies :
> > > 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.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Huw Davies]

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.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Huw Davies]

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.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Jactry Zeng]

2014-07-21 17:21 GMT+08:00 Huw Davies :
> 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!

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Huw Davies]

On 21 Jul 2014, at 13:27, Jactry Zeng wrote:
> No, you are right, I made a mistake here.
> Thanks for your review!

Looks good.

Huw.

[PATCH 1/2] riched20: Implement ITextSelection::GetText. (try 3)

[Jactry Zeng]

2014-07-04 19:00 GMT+08:00 Jactry Zeng :
> - 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]
> - 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]
> -
>
> 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....