search

MesserLab / SLiM

SLiM is a genetically explicit forward simulation software package for population genetics and evolutionary biology. It is highly flexible, with a built-in scripting language, and has a cross-platform graphical modeling environment called SLiMgui.
https://messerlab.org/slim/
GNU General Public License v3.0
159 stars 30 forks source link

QtSLiM *Open Recipe* list is sorted lexicographically rather than naturally #436

Closed bryce-carson closed 1 month ago

bryce-carson commented 5 months ago

Shouldn't the sorting order be natural? The commit I built from when I saw this is ad71419

Screenshot at 2024-03-21 17-03-15

bhaller commented 5 months ago

I looked into this a little, IIRC, and it was hard to fix. Qt does the sorting here, and it doesn't have a smart sorting option that is available to me. I don't remember the details. I'll leave this open, because yes, it should be fixed, but since I've already looked at it once and thrown my hands up in despair, I'm going to mark it "long-term" and not look at it for a while.

bryce-carson commented 5 months ago

I did a little searching and reading through the QtSLiM sources and this is something I could fix given some time. It's a helpful way to brush up on my C++ as well.

These are the relevant places in the code, yes?

  1. https://github.com/MesserLab/SLiM/blob/6d00ad3709df8ee7e3a82d4ce4f03699464a5561/QtSLiM/QtSLiMAppDelegate.cpp#L425
  2. https://github.com/MesserLab/SLiM/blob/6d00ad3709df8ee7e3a82d4ce4f03699464a5561/QtSLiM/QtSLiMFindRecipe.cpp#L102

The sorting you are doing is definitely correct. Your commentary in the code reveals, as you've mentioned that you looked into it before, it really likely is a Qt issue.

An alternative is to use a QMap rather than a QStringList for the entryList. I can look into this once I'm done with a small Python project. Some obvious requirements are:

bhaller commented 5 months ago

Hi @bryce-carson! Yes, those are the relevant spots.

The std::sort() calls using collator just need to be replaced with something that works on all platforms. I'm not sure how QMap rather than QStringList helps...? Seems like maybe you just need to write a comparison lambda/function for the std::sort() call that does the right thing, replacing collator. In the comparator, strip off whatever prefix is shared between the two strings, extract the number that remains at the beginning (up to a dot or space), convert to integers, and return the comparison of the integers. Should be pretty straightforward. Might get a little more complicated because some of the recipes have a form like Recipe X.Y.Z blah blah blah I versus Recipe X.Y.Z blah blah blah II, with a Roman numeral at the end; I think Roman numerals as high as V are used. Happily, the Roman numerals from I to V (and even a bit further!) happen to be alphabetically sorted already, so in that case you can just revert to an alphabetic sort.

Do please try to avoid rewriting the surrounding code, just for stability; that should not be necessary, I think. If this makes sense to you, then sure, go for it! :->

bryce-carson commented 3 months ago

The sorting order is (correctly) natural when SLiMgui 4.2.2 is used on Ubuntu 18.04. There must have been a change in Qt 5.15 which modified the way sorting operates after the release of Ubuntu 18.04; six years of maintenance changes, so it's possible.

bhaller commented 3 months ago

The sorting order is (correctly) natural when SLiMgui 4.2.2 is used on Ubuntu 18.04. There must have been a change in Qt 5.15 which modified the way sorting operates after the release of Ubuntu 18.04; six years of maintenance changes, so it's possible.

Could be a change in Ubuntu, no? Note the comments on the Qt bug saying "You must turn ICU support on to get proper numeric collation" and similar. Maybe Ubuntu 18.04 included ICU in its Qt build (whatever ICU is), and later versions did not? Might even depend on the way Qt was built/installed on a particular Ubuntu system, since there are various ways of getting Qt. Anyhow, the path forward is clear: the QCollator usage needs to be replaced as discussed above.

bhaller commented 1 month ago

@bryce-carson Do you intend to fix this bug? If not, I'm happy to fix it myself, but I'd like it to get fixed one way or the other. :-> Thanks!

bryce-carson commented 1 month ago

@bryce-carson Do you intend to fix this bug? If not, I'm happy to fix it myself, but I'd like it to get fixed one way or the other. :-> Thanks!

I an not able to recently. I would need to do some decent review of C++; it's been a bit for me.

You go ahead with it, please.

bhaller commented 1 month ago

OK, no worries! :->