-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify sorting and make it more readable #58
Conversation
Okay, when I tried it worked very well. Maybe you will find issues, but the problem is that I wouldn't create a global function storing sorting data, and as this is a table of messages containing tables with info, it's hard to reproduce a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally good, thx !
I've converted this back to draft because it still does not implement all functionality, we discussed this in mt-mods discord and results can be found from another PR which targets this branch. So before merging this one either #59 should be merged and tested or missing functionality added directly to My personal preference, go with #59 instead of reinforcing formspec/ui dependency on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good work, for me it's doesn't present anything weird. I approve the merge.
No issues found.
Main changes:
Be aware that this is incomplete work.There's 2 operation modes forFixed.sortfield == "1"
, this is because I was not sure how exactly it is supposed to work.Maybe it is supposed to be likeFixed.a.from < b.from or a.to < b.to
?This will also modify original table but I think that shouldn't be problem, not 100% sure though... Checked this too, now makes shallow copy.