Opened 3 years ago

Closed 20 months ago

Last modified 20 months ago

#13646 closed enhancement (fixed)

wxStyledTextCtrl::GetLineText() bugfix

Reported by: troelsk Owned by: vadz
Priority: normal Milestone: 2.9.5
Component: wxStyledText Version: stable-latest
Keywords: wxStyledTextCtrl wxTextAreaBase Cc:
Blocked By: Blocking:
Patch: yes

Description

The implementation is wrong, trailing newline(s) are not stripped

virtual wxString GetLineText(long n) const { return GetLine(n); }

Attachments (4)

GetLineText.patch download (2.4 KB) - added by troelsk 20 months ago.
Trunk
GetLineText2.patch download (1.9 KB) - added by troelsk 20 months ago.
Trunk
GetLineText3.patch download (2.3 KB) - added by troelsk 20 months ago.
Trunk
GetLineText4.patch download (1.2 KB) - added by troelsk 20 months ago.
GetLineLength

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by troelsk

  • Patch unset

Changed 20 months ago by troelsk

Trunk

comment:2 Changed 20 months ago by troelsk

  • Patch set
  • Summary changed from wxStyledTextCtrl::GetLineText() bug to wxStyledTextCtrl::GetLineText() bugfix
  • Type changed from defect to enhancement

comment:3 Changed 20 months ago by vadz

  • Milestone set to 2.9.5
  • Status changed from new to confirmed

Thanks, this does look like the right thing to do (pity that we don't have any unit tests for this control, such basic stuff wouldn't get broken if we had them...). I just wonder if using find_last_not_of("\r\n") wouldn't be slightly better than this manual iterator stuff?

Changed 20 months ago by troelsk

Trunk

comment:4 Changed 20 months ago by troelsk

I did not know of that method, find_last_not_of(). It works ok in my testbed. New patch.

comment:5 Changed 20 months ago by vadz

Sorry, I don't think this is correct. If npos is returned the string should be cleared as it means that it consists entirely from EOL characters. Or am I missing something?

Changed 20 months ago by troelsk

Trunk

comment:6 Changed 20 months ago by troelsk

  • Keywords wxTextAreaBase added

Right. New patch again.
(moved the GetLineLength() implementation to wxTextAreaBase as it seems unrelated to wxSTC)

comment:7 Changed 20 months ago by vadz

  • Owner set to vadz
  • Status changed from confirmed to accepted

Sorry, I don't like making GetLineLength() non pure virtual, you should at least think about implementing it more efficiently than by getting the line length and returning it, normally it should be possible.

I'll apply the rest of the patch soon, thanks for fixing it.

comment:8 Changed 20 months ago by VZ

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [73140]) Strip EOL characters from wxStyledTextCtrl::GetLineText() return value.

For consistency with all the other wxTextCtrl-like classes, the value returned
by this method must not include line terminator characters (like '\n'). Notice
that Scintilla-specific GetLine() does still include them, for consistency
with the Scintilla API itself.

Closes #13646.

comment:9 Changed 20 months ago by troelsk

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks!

Sorry, I don't like making GetLineLength() non pure virtual, you should at least think about implementing it more efficiently

Ok, makes sense.

The GetLineLength() modification slipped out from the commit. Added a new tiny patch.

Changed 20 months ago by troelsk

GetLineLength

comment:10 Changed 20 months ago by VZ

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [73150]) Also account for EOL chars correctly in wxStyledTextCtrl::GetLineLength().

Make GetLineLength() consistent with GetLineText() after the changes of
r73140.

Closes #13646.

comment:11 Changed 20 months ago by vadz

Oops, sorry for not noticing this part of the patch and thanks for checking!

Note: See TracTickets for help on using tickets.