Add natural sort functions for wxString #1923

PBfordev · 2020-07-02T16:39:12Z

This PR builds on #780, attempting to fix some possible deficiencies in that PR.

I have not updated the documentation yet, as this PR may be rejected for the reasons stated in my last comment in #780.

The code also needs to account for wxUSE_REGEX == 0 but I do not know how to best do it.

…nly WX version.

…idgets into natural-sort

Add a new string fragment type for whitespace and punctuation which needs to be assessed separately from letters and symbols. Use wxUint64 instead of long for storing the value for numeric fragment. Use collate instead of compare for non-numeric fragments. Change names for the public comparison functions: wxWidgets provided function is now named wxCmpGenericNatural() and for common public use is wxCmpNatural() which calls a native function in wxMSW and wxCmpGenericNatural() elsewhere. Make some other changes to simplify and possibly speed up the code.

In src/arrstr.cpp add wx/utils.h and wx/wxcrt.h includes.

vadz

Thanks, this looks broadly good to me and the remaining small changes should be very simple to make.

Concerning wxUSE_REGEX, I think it would be fine to just fall back on wxStrcmp() in this case, we just would need to mention this in the documentation (and also disable unit test, i.e. add a warning about skipping it, in this case).

Thanks again for reviving this, it would be nice to finally see it merged!

include/wx/arrstr.h

interface/wx/arrstr.h

src/common/arrstr.cpp

vadz · 2020-07-04T11:08:10Z

I may be missing something here, but why are you so pessimistic about the chance of this being merged? I don't see any really bad problems with these changes. Of course, they're not perfect, but then nothing is and things could be improved later, while for now this already adds a wrapper for a useful (in the context of UI development) MSW function with a reasonable fallback under the other platforms, so I think it should be fine to merge it... Unless, once again, I simply oversaw some critical problem?

PBfordev · 2020-07-04T16:25:42Z

why are you so pessimistic about the chance of this being merged?

My innate pessimism aside, the issues I see (none is critical):

I cannot guarantee there is not a bug there, the algorithm seems simple enough, but even the original PR had it wrong.
Sorting like this is very inefficient since we do not provide a sort function but only a comparison function. Hence string transformations must be done during each comparison, i.e., possibly many times for each string. OTOH, the function should probably not be used for large amounts of strings.
The "fragmentization" algorithm does not properly handle control characters. OTOH, this function should be used for strings that should not have such characters; moverover, ASCII tabs and new lines which are probably most common control characters for strings presented to the user are included in :space:. I thought of adding a string fragment type using regex :cntrl: but that would probably involve also Unicode control characters which are a can of worms which I did not dare open and left it to the collation function.
The original PR was in a limbo for two years.

Anyway, I will address the issues pointed in the review, hopefully by Tuesday at the latest.

"$" is not considered a letter/symbol, but a punctuation.

In wxCmpNaturalGeneric() do not use wxStrcmp() which requires including wxcrt.h; use wxString::compare() instead. In the wxCmpNaturalGeneric() documentation, fix the typo in wxRegEx (was wxRegex) and do not refer to undocumented wxStrcmp().

PBfordev · 2020-07-05T13:30:27Z

I have addressed the review comments, please let me know if there something more to be done here.

vadz · 2020-07-05T21:34:20Z

Looks good to me, I'm ready to merge this once the CI builds finish. Thanks again!

PBfordev · 2020-07-06T06:12:22Z

You probably have a plan for this but I wonder how to merge it? I think, if possible, it would be best to somehow squash it into just two commits: the first containing all the commits by Rocketmagnet, the second with all mine.

EDIT
I forgot to mention that I am not sure about using the simplest string comparison when wxRegEx is unavailable. If, for example, in the future the natural sorting is added to wxFileCtrl and wxDirCtrl, perhaps doing a case-insensitive collation instead would be much better.

src/common/arrstr.cpp

If wxRegEx is not available, do not just make a simple string comparison but perform a case-insensitive collation.

src/common/arrstr.cpp

PBfordev · 2020-07-07T17:51:19Z

I have addressed the comments from the last review.

Add natural sort functions. See #1923

vadz · 2020-07-07T21:18:37Z

Thanks again, I've squash-rebased as suggested and pushed it now.

Rocketmagnet added 30 commits April 9, 2018 22:35

Added Natural Sort functions to wxString.

93d19fa

Fixing build errors. Adding test for wxUSE_REGEX.

6da524f

Moved natural sort functions from wxString to wxArrayString.

2da10b4

Requested changes.

7ff7c5e

Requested changes.

83df195

Requested changes.

d4f04e9

Adding tests for wxNaturalStringSortAscending().

2c76334

Using StrCmpLogicalW() under MSW.

77527f7

Trying to solve build errors.

56d7837

Adding WXDLLIMPEXP_BASE.

9a2a279

Adding WXDLLIMPEXP_BASE to arrstr.h too.

7272129

Renaming functions.

222f7d5

using wc_str() instead of c_str().

4c1a654

Fixinf documentation.

853b024

Removing case /noCase options.

187e212

Putting WXDLLIMPEXP_BASE back in.

4d265e9

Reverting whitespace. Removing deciamal point functionality.

71747ce

Reverting whitespace.

7b00e5f

Handling value as int. Layout fixes. Typo.

d08bf9b

Handling value as int, forgot about the conversion.

eb7524a

Correcting comment.

d8aeb9a

Modifying comment.

39bdba7

Modifying comment, again.

f9bf680

__WXMSW__ -> __WINDOWS__

635ef2f

Opening up both Native and WX versions. Adding GNU version. Testing o…

c488f59

…nly WX version.

Opening up both Native and WX versions. Adding GNU version. Testing o…

2b8b1da

…nly WX version.

Removing native GNU natural sort strverscmp().

72ff85b

Testing #define _WIN32_IE 0x0501 to enable StrCmpLogicalW().

2425a55

Testing using __MINGW32_TOOLCHAIN__ to disable StrCmpLogicalW().

2495fae

Declaring StrCmpLogicalW() in arrstr.cpp #ifdef __MINGW32_TOOLCHAIN__.

86fc74e

Rocketmagnet and others added 6 commits April 27, 2018 10:50

Try removing LWSTDAPI_().

986df98

Using extern "C".

003027d

Using extern "C" only once!

701646a

Merge commit 'refs/pull/780/head' of https://github.com/wxWidgets/wxW…

02aefac

…idgets into natural-sort

Update strings in tests

192374d

PBfordev mentioned this pull request Jul 2, 2020

Added Natural Sort functions to wxString. #780

Closed

Add headers needed for wxMin() and wxStrcoll_String()

6e874f2

In src/arrstr.cpp add wx/utils.h and wx/wxcrt.h includes.

vadz reviewed Jul 3, 2020

View reviewed changes

PBfordev added 4 commits July 5, 2020 10:27

Address review comments

3bb0462

Update documentation

03bdf3d

Remove incorrect comparisons from tests

c489245

"$" is not considered a letter/symbol, but a punctuation.

Fix issues from previous commits

e099613

In wxCmpNaturalGeneric() do not use wxStrcmp() which requires including wxcrt.h; use wxString::compare() instead. In the wxCmpNaturalGeneric() documentation, fix the typo in wxRegEx (was wxRegex) and do not refer to undocumented wxStrcmp().

Add missing break in switch case

435e467

vadz added the ok to merge Was reviewed and ok'd by at least one person label Jul 5, 2020

catalinr reviewed Jul 6, 2020

View reviewed changes

PBfordev added 2 commits July 7, 2020 09:20

Address comments from the second review

b1ffd72

Try harder in wxCmpNaturalGeneric() if wxRegEx is unavailable

843bf53

If wxRegEx is not available, do not just make a simple string comparison but perform a case-insensitive collation.

catalinr reviewed Jul 7, 2020

View reviewed changes

src/common/arrstr.cpp Outdated Show resolved Hide resolved

Do not guard regex.h include

e039468

vadz added a commit that referenced this pull request Jul 7, 2020

Merge branch 'natural-sort'

2289f8b

Add natural sort functions. See #1923

vadz closed this Jul 7, 2020

PBfordev deleted the natural-sort branch October 13, 2020 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add natural sort functions for wxString #1923

Add natural sort functions for wxString #1923

PBfordev commented Jul 2, 2020 •

edited

vadz left a comment

vadz commented Jul 4, 2020

PBfordev commented Jul 4, 2020

PBfordev commented Jul 5, 2020

vadz commented Jul 5, 2020

PBfordev commented Jul 6, 2020 •

edited

PBfordev commented Jul 7, 2020

vadz commented Jul 7, 2020

Add natural sort functions for wxString #1923

Add natural sort functions for wxString #1923

Conversation

PBfordev commented Jul 2, 2020 • edited

vadz left a comment

Choose a reason for hiding this comment

vadz commented Jul 4, 2020

PBfordev commented Jul 4, 2020

PBfordev commented Jul 5, 2020

vadz commented Jul 5, 2020

PBfordev commented Jul 6, 2020 • edited

PBfordev commented Jul 7, 2020

vadz commented Jul 7, 2020

PBfordev commented Jul 2, 2020 •

edited

PBfordev commented Jul 6, 2020 •

edited