Opened 14 months ago

Closed 14 months ago

Last modified 11 months ago

#15249 closed optimization (fixed)

Improve wxGridCellAutoWrapStringRenderer to account for long words, line breaks and tabs

Reported by: nmset Owned by: vadz
Priority: normal Milestone: 3.0.0
Component: wxGrid Version: stable-latest
Keywords: wrapping Cc:
Blocked By: Blocking:
Patch: yes

Description

As it is, the wrapping functionality ignores very long words, line breaks and tabulations.

The attached patch proposes another wrapping approach to preserve the above mentioned.

I don't know if the behaviour as it is, is what has always been intended.

Thank you for considering.

Attachments (4)

gridctrl.cpp.patch download (4.0 KB) - added by nmset 14 months ago.
Wrapping function of wxGridCellAutoWrapStringRenderer
gridctrl.patch download (8.4 KB) - added by nmset 14 months ago.
Reviewed, with helper functions, and demo data
gridctrl-03.incremental.patch download (1.9 KB) - added by nmset 14 months ago.
Bug fixing - avoid infinite loop on tabs
wx-grid-wrapping.diff download (8.7 KB) - added by vadz 14 months ago.
Cleaned up and simplified version of the original patch

Download all attachments as: .zip

Change History (12)

Changed 14 months ago by nmset

Wrapping function of wxGridCellAutoWrapStringRenderer

comment:1 Changed 14 months ago by vadz

  • Milestone 2.9.5 deleted
  • Status changed from new to confirmed
  • Summary changed from wxGridCellAutoWrapStringRenderer : patch for full wrapping to Improve wxGridCellAutoWrapStringRenderer to account for long words, line breaks and tabs

Thanks, this code can definitely be improved but I really wish it were done without having 6 levels of nested conditionals (and 8 or 9 if you count loops too). Perhaps using helper functions would help?

The code also would need to be reformatted to fit wxWidgets conventions (4 spaces indent; braces on their own lines) before being applied.

Finally, what is the best way to test the changes in the grid sample? Can the improvements be already seen there or could you perhaps please add a small change to the sample to your patch to show them?

Thanks again!

comment:2 Changed 14 months ago by nmset

OK, I'll do that over the weekend, namely, splitting the loops as much as possible.

I used the grid sample application for testing, in cell A2 as far as I remember(I'm not on the right PC just now). I'll try also to adapt the grid sample application for the purpose.

Regards.

comment:3 Changed 14 months ago by nmset

Sorry, it's in cell B1 (0, 1).

Changed 14 months ago by nmset

Reviewed, with helper functions, and demo data

comment:4 Changed 14 months ago by nmset

Hello,

I've attached another patch to address the issues discussed before. Can't do better. Please ignore and close the ticket if the code still needs to be split.

Testing data appears in cell K1 in the sample grid application.

Regards.

Changed 14 months ago by nmset

Bug fixing - avoid infinite loop on tabs

comment:5 Changed 14 months ago by vadz

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

Thanks for the update, I've simplified and cleaned up the code a little:

  1. Use wxSplit() instead of wxStringTokenizer for line splitting, this is definitely simpler and probably more efficient too.
  2. Use wxDC::GetPartialTextExtents() instead of O(N^2) algorithm calling GetTextExtent() for each word prefix in the original patch. This should be very

noticeable for grids with a lot of wrapped cells.

  1. Simplified the word wrapping function significantly.
  2. Got rid of the strange special case for TABs.
  3. Don't use unnecessary variables like tooWide which are initialized and then tested exactly once.
  4. Also get rid of y variables as we're only interested in text width, not height.
  5. Also don't initialize wxStrings unnecessarily, this = wxEmptyString is just unneeded. And use clear() instead of wxEmptyString assignment for clarity. And also empty() instead of comparing with wxEmptyString.
  6. Use wxWidgets naming conventions, i.e. camelCase, not unix_like.

Please let me know if my version (attached) works for you and I'll commit it if it does.

Changed 14 months ago by vadz

Cleaned up and simplified version of the original patch

comment:6 Changed 14 months ago by nmset

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

It works perfectly well, as we might expect.

Thanks.

Closing.

comment:7 Changed 14 months ago by VZ

(In [74245]) Improve wxGrid cell wrapping in wxGridCellAutoWrapStringRenderer.

Wrap the words too long to be shown on one line on several lines.

Also take the line breaks and TABs into account.

Closes #15249.

comment:8 Changed 11 months ago by iwbnwif

I believe that this has introduced a new bug. Please review the following:

http://trac.wxwidgets.org/ticket/15464

Note: See TracTickets for help on using tickets.