Add ability to sort compare list

[Ebag333]

See: https://puu.sh/swp4e/058450d696.gif

You can pass in either the column number or the attribute name to sort on. We attempt to handle it cleanly, and default back to no sorting if something goes wrong.

The sort parameter is not required, so anything else using it won't break.

We don't handle reverse sorts (clicking on the column header twice) and for reasons sometimes it sorts up and sometimes it sorts down. Most likely has to do with different types, but not terribly worried about it.

Also, since Name and Price aren't part of the attrs array we can't sort on those. If you click on those columns, it doesn't sort at all, to prevent various issues.

@blitzmann this is ready to be merged.

[blitzmann]

@blitzmann this is ready to be merged.

While this indeed does look good, I'm not quite there yet, due to not being able to sort by name and price - this is a very odd situation for the user. you should be able to conditionally sort by the name if they click on the first column, and then by the attribute if they click on any other column. And then there this:

We don't handle reverse sorts (clicking on the column header twice) and for reasons sometimes it sorts up and sometimes it sorts down. Most likely has to do with different types, but not terribly worried about it.

Do you know what those reasons are? Reverse sort should be easy to implement as it is right now, but I'm more interested on the comment stating that sometimes it sorts asc, and sometimes desc. Do you have an example of that that I can try? Is it reproducible?

Having said that, there is a mixin for the ListCtrl widget we use. See https://wxpython.org/docs/api/wx.lib.mixins.listctrl.ColumnSorterMixin-class.html (this is an old page, there might be newer documentation on it. Google <3)

I believe this might alleviate the issues above. Instead of passing the ListCtrl an already sorted list, you would pass it a list and then once the columns are clicked it will sort by the value inside that column. I don't think I've ever used it before, but it's worth a peak. :)

[Ebag333]

I'm not quite there yet, due to not being able to sort by name and price - this is a very odd situation for the user. you should be able to conditionally sort by the name if they click on the first column, and then by the attribute if they click on any other column.

The way we build it, we're hard coding column 1 (and the last column for price). The rest of the columns are just pulled from the array, so it's easy to handle.

Name is probably relatively easy to handle, but price isn't as we're dynamically injecting it as we build it, so there's no real good way to sort on that.

We could just sort on name if we end up not being able to sort on anything else (which would cause both name and price to sort by name). That wouldn't be terribly difficult to do.

Do you know what those reasons are? Reverse sort should be easy to implement as it is right now, but I'm more interested on the comment stating that sometimes it sorts asc, and sometimes desc. Do you have an example of that that I can try? Is it reproducible?

No idea. I'm guessing it's a combination of having mixed fields (strings, numbers) and both positive and negative numbers. It also just might be the fact that we have negative numbers in there that makes it weird, especially since in CCPs world negative is sometimes better. You can look at the .gif in the first post to see how it behaves yourself.

Having said that, there is a mixin for the ListCtrl widget we use.

I did try using the built in widget controls first, I'm not sure if we're just doing too much custom stuff for them to work but they did not handle it well at all. It entirely could be that I'm just shite at wxPython. A lot of folks write custom sort functions, because they have issues with the built in one (if StackOverflow is anything to go by).

I'm honestly not terribly concerned about being able to flip between forward and reverse sort, and I can easily add sorting by name for otherwise invalid column clicks. I think that it'd be a significant improvement from the current implementation, and worth merging.

this is a very odd situation for the user

It's less odd than columns you can't sort at all. :)

[blitzmann]

Did you get the mixin working at all and it just didn't work well, or did you not get it working? If you got it working, put the code somewhere so I can take a look.

[Ebag333]

I never got it working, kept getting errors. Code is purged, both from pyCharm and my brain. :D

[Ebag333]

Well, sorting by the name turned out to be hard. Name isn't an attribute, so it's not very straight forward. :(

I did implement something that I think is a reasonable compromise, clicking on the name column reverts back to the default sorting (meta level). So it's not perfect, but at least clicking it does something, and you don't have to go hunting for the metaLevel column to go back to the default.

Can we merge this now? inomares needs it. :D

[Inomares]

+1 to this compromise - seems reasonable to me. It's quite rare to want to sort by name anyway.

[blitzmann]

FYI, it was not difficult to add name sorting to it. You just have to create a different lambda for it since it's not an attribute, hence:

you should be able to conditionally sort by the name if they click on the first column, and then by the attribute if they click on any other column.

Additionally, I've added reverse sorting as well.

for reasons sometimes it sorts up and sometimes it sorts down.

This is probably due to not seeing the negative. I also ran into this issue until I realized what i was sorting was a negative number, so -50% would come before -25% when your brain thinks 25 should come before 50. if this is the case, then it's working properly. :)

[Ebag333]

@blitzmann Yeah yeah yeah, Mr fancy pants. :)

FYI, you should have the ability to commit directly to my branch, so you don't have to do commits directly to master. Would make the merge slightly cleaner.

[blitzmann]

@Ebag333 I did make commits on your branch and then merged it into master. I just didn't push back up to your branch (can I even do that???). You're probably talking about the merge commit "clutter", which would happen regardless. :)

[Ebag333]

I did make commits on your branch and then merged it into master.

@blitzmann The commit chain shows the merge then your separate commit. Not a big deal. It was more about..... (see below)

I just didn't push back up to your branch (can I even do that???). You're probably talking about the merge commit "clutter", which would happen regardless.

I set you with auth a long time ago on my repo, so yeah, you should be able to commit directly to the branch.

There is also a checkbox on the PR itself Allow edits from maintainers., but I think I'd only need to check that if you weren't a collaborator on my forked repo.

This was more about pointing this out for future reference as an option for you, not trying to tell you how to do your job. :)

[blitzmann]

Ah, you're correct. It's because I use this setting (fastforward):

I'll try to remember to change that next time, as I agree it helps