#159 Better working GetCurrentPosition for TEdit and TRichEdit

[Chris Kohlert]

right now TEdit::GetCurrentPosition() looks like this:

:::C++
//
/// Return the current caret position.
//
inline int TEdit::GetCurrentPosition() const
{
  PRECONDITION(GetHandle());
  int CurPos = 0;
  GetSelection(CurPos, CurPos);
  return CurPos;
}

which is problematic because first: i guess its undefined what
happens if we put the same variable in two out-parameters at the same time,
and second: if edit has a selection the result is very undesirable and
often plain wrong...

in case of a selection GetSelection wont give us a hint where the cursor
is, and we have to ask the edit-window for the cursor in a different way:

:::C++
//
/// Returns the current caret position.
///
/// \note the windows edit control can only handle position-ranges between
///       0..2^16 characters
/// 
/// \exception TXGdi is thrown on failure.
//
auto TEdit::GetCurrentPosition() const -> int
{
  PRECONDITION(GetHandle());

  TRange selection = GetSelection();
  if (selection.IsValidRange()) { 
    // we dont know the caret position yet...

    // CharFromPos would not report positions above that range
    // because the relevant information we get back is only of WORD size...
    if (selection.cpMin > USHRT_MAX || selection.cpMin > USHRT_MAX)
      throw TXGdi{IDS_GDIFAILURE, GetHandle()}; // sorry, you reached the limits of this windows-api

    /* we dont know which end of the range is the caret-position,
       so we have to ask explicitly for it... */
    TPoint caretpos = GetCaretPos();
    // CharFromPos wouldnt be able to handle positions above that range
    if (    caretpos.x > SHRT_MAX || caretpos.x < SHRT_MIN
         || caretpos.y > SHRT_MAX || caretpos.y < SHRT_MIN)
      throw TXGdi{IDS_GDIFAILURE, GetHandle()}; // windows-api limit

    auto cfp = CharFromPos(static_cast<int16>(caretpos.x), static_cast<int16>(caretpos.y));
    CHECK(-1 != cfp); // (65535, 65535) error - position out of bounds - shouldnt happen

    return LOWORD(cfp);
  }

  return selection.cpMin;
}

the result of GetSelection is always sorted, as can be seen in this implementation:
https://github.com/wine-mirror/wine/blob/master/dlls/user32/edit.c#L2524

analog TRichEdit::GetCurrentPosition() but with less limits on CharFromPos.

[Vidar Hasfjord]

That's enlightening. Good work!

I actually think this problem — i.e. the incorrect index returned — should be filed as bug, not phrased as a feature request ("please make it work properly").

I suggest you improve the brief description in the documentation as well. The current description is self-evident and useless. Perhaps clarify that it is an index that is returned, not a (x, y) position.

Nitpick: Use proper capitalisation and punctuation in the documentation. (Internal code comments can have personal style, I guess. But I recommend following the established style.)

[Vidar Hasfjord]

Regarding [r5025]:

• virtual should not be added to the Search overload taking a tstring. This function just forwards to a virtual function, i.e. Search taking LPCTSTR, which is already virtual.
• Same goes for SetSelection (const TRange&), which forwards to virtual fuction SetSelection (int, int).
• !this->operator==(other) can be written simpler as !operator==(other).

[Vidar Hasfjord]

Regarding [r5026] (the patch):

• BUG: You throw TXGdi in TEdit, passing a window handle (GDI handle expected). Don't use TXGdi here (should only be used in the GDI classes). Use TXOwl.

[Chris Kohlert]

good eye, thanks a lot, i will fix that soon :)

[Vidar Hasfjord]

The fixes in [r5025], [r5026], [r5055] and [r5064] on the trunk have now been merged into Owlet [r5114].

Thanks for your contribution, Chris!

PS. Don't commit multiple changes relating to more than one ticket in one and the same revision. Split it into separate revisions (see our Coding Standards). That simplifies merging a great deal.