Skip to content
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

MudDataGrid: Allow user to use a custom IComparer<object> for sorting #6368

Merged

Conversation

SinisterMaya
Copy link
Contributor

Description

Proposition to add a IComparer to the SortDefinition record class in DataGrid. User can either set their custom Comparer as a Column Parameter or they can set it once with SetSortAsync Task or the SortDefinitions Dictionary.

This feature is optional to be used. It's a tool to allow advanced sorting. Those wishing to use a custom comparer have to implement their own Comparer class like the one I made for my example and my test unit.
image

The reason why I did this was to allow custom Comparer like the StrCmpLogicalW from the Shlwapi.dll on Windows so we can properly sort filenames or string with a hierarchical pattern (e.g.: {"1", "1.1", "1.1.2", "1.2", "1.10"})

Since OrderBy and OrderByDescending both takes a nullable IComparer parameter, we can default it as null without causing a breaking change. That way, if comparer is not supplied, OrderBy and OrderByDescending will use the default comparer for the type.

Here's the proposed documentation addition with it:
(default string sorting)
image

(natural sorting enabled by the Comparer Parameter)
image

We can see that 10 comes now after 2 and 1_2 comes now before 1_10 and 1_11

Ordering using a IComparer is not supported for Linq to Entities operations, but I don't think that is ever the case here unless a Column can sort directly from a Linq query ?

How Has This Been Tested?

I created a unit test to validate calling SetSortAsync works with and without the comparer on different field types. I also tested assigning a comparer to a column and validated that the ordering was the one expected.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added enhancement New feature or request PR: needs review labels Feb 22, 2023
@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Patch coverage: 98.87% and project coverage change: +0.15 🎉

Comparison is base (037ea61) 90.47% compared to head (5de54cd) 90.63%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #6368      +/-   ##
==========================================
+ Coverage   90.47%   90.63%   +0.15%     
==========================================
  Files         398      399       +1     
  Lines       13487    13565      +78     
==========================================
+ Hits        12203    12295      +92     
+ Misses       1284     1270      -14     
Impacted Files Coverage Δ
...MudBlazor/Components/DataGrid/MudDataGrid.razor.cs 90.37% <87.50%> (+2.34%) ⬆️
src/MudBlazor/Components/DataGrid/Column.razor.cs 79.31% <100.00%> (+0.48%) ⬆️
.../MudBlazor/Components/DataGrid/HeaderCell.razor.cs 68.48% <100.00%> (+2.42%) ⬆️
...rc/MudBlazor/Components/DataGrid/SortDefinition.cs 100.00% <100.00%> (ø)
src/MudBlazor/Utilities/NaturalComparer.cs 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@Anu6is Anu6is left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small correction in documentation

@SinisterMaya SinisterMaya force-pushed the MudDataGrid/SortDefinition-IComparer branch 2 times, most recently from 4787ca3 to 4615ae2 Compare February 23, 2023 15:03
@SinisterMaya
Copy link
Contributor Author

EDIT:

Corrected my typo (Thank you @Anu6is) and added ctrl/alt click tests to my unit test

@henon
Copy link
Collaborator

henon commented Mar 14, 2023

DataGrid had a huge update so please merge dev and resolve conflicts.

@SinisterMaya SinisterMaya marked this pull request as draft March 14, 2023 13:14
@SinisterMaya SinisterMaya force-pushed the MudDataGrid/SortDefinition-IComparer branch 2 times, most recently from 3f43364 to 407856a Compare March 14, 2023 14:22
@SinisterMaya SinisterMaya marked this pull request as ready for review March 14, 2023 14:26
@SinisterMaya
Copy link
Contributor Author

@henon I rebased and merged. The conflict was the change from Column.Field to Column.PropertyName when handling sort change on header cells on lines where I added a comparer to the SortDefinition. This doesn't break anything for me. It was just a matter of combining changes.

I also had to change my test and doc datagrid to adopt the new PropertyColumn replacing Column.

@SinisterMaya SinisterMaya force-pushed the MudDataGrid/SortDefinition-IComparer branch from 407856a to 720b8a9 Compare March 14, 2023 15:51
Copy link
Collaborator

@henon henon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the natural comparison is great but it makes the example quite long. Also it seems duplicated in example and test case. So how about moving it into a public MudBlazor util class which is used by both. Public so it can also be used by our users.

@SinisterMaya
Copy link
Contributor Author

@henon

I can do that. It was duplicated because I thought of it as an example. We can roughly reach the same result with the Shell windows api dll on windows:
image

But since it had to work without any references, I used this algorithm I found which is about twice as slow, but still faster than a regex approach. I've been working on optimizing it, but did not yield huge results so far. Here's the Benchmark of it.
image

With this being the regex algorithm in my benchmark:
image

@SinisterMaya SinisterMaya requested a review from henon March 15, 2023 01:43
@SinisterMaya
Copy link
Contributor Author

Changes added in a new commit. Figured it would help keep track if requested changes are good for you. I did a UnitTest to test pretty much every lines of the NaturalComparer Utility. Changed my examples to use the public utility instead of their own comparer.

Comment on lines +77 to +91
,"111111111111111111.txt"
,"00222222222222222222.txt"
,"0222222222222222222.txt"
,"222222222222222222.txt"
,"00999999999999999999.txt"
,"0999999999999999999.txt"
,"999999999999999999.txt"
,"1111111111111111111.txt"
,"2222222222222222222.txt"
,"9999999999999999999.txt"
,"11111111111111111111.txt"
,"0001111111111111111111111111111111111111111111111111111111111111111111.txt"
,"1111111111111111111111111111111111111111111111111111111111111111111.txt"
,"9999999999999999999999999999999999999999999999999999999999999999999.txt"
,"11111111111111111111111111111111111111111111111111111111111111111111.txt"
Copy link
Collaborator

@henon henon Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a critique, just a fun fact. I tried the windows api dll func on my win10 to sort these file names and it sorts them differently. Very subtle differences though.

"1-a.txt",
"0001111111111111111111111111111111111111111111111111111111111111111111.txt",
"00222222222222222222.txt",
"00999999999999999999.txt",
"111111111111111111.txt",
"0222222222222222222.txt",
"222222222222222222.txt",
"0999999999999999999.txt",
"999999999999999999.txt",
"1111111111111111111.txt",
"11111111111111111111.txt",
"1111111111111111111111111111111111111111111111111111111111111111111.txt",
"11111111111111111111111111111111111111111111111111111111111111111111.txt",
"2222222222222222222.txt",
"9999999999999999999.txt",
"9999999999999999999999999999999999999999999999999999999999999999999.txt",
"a001test01.txt",

Hard to say which is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Apparently, Windows API sorting differs from versions to versions and it's hard to tell how they evaluate non alphanumerical char weight. We can see that windows API doesn't give"-" the same weight as "(", "[", "_", ... etc. "-" seems to be ignored by windows, which seems to be inconsistent with what I would expect. what do you think?

image

Copy link
Collaborator

@henon henon Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, these are all special cases, so don't worry about it. for most people what really matters is the order of 1, 2, 3, 10, 11, 12 instead of 1, 10, 11, 12, 2, 3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. in my case (and the reason why I created that PR) is to order hierarchical item name

image

This project represents a Bill Of Material. Each Item is either a part to be manufactured, ordered, or an assembly/sub-assembly. You can Imagine a keyboard as being a main assembly, each keyswitches are subassemblies containing their own components and so on. Some component are found in different subassemblies at different levels. So my items are a way to hold that information while the actual component (The Model) can be referenced more than once.

@henon
Copy link
Collaborator

henon commented Mar 15, 2023

@tjscience I think adding support for a comparer is a good idea. shall I merge this now or do you want me to merge it later to avoid possible conflicts with your changes?

@neozhu
Copy link
Contributor

neozhu commented Apr 8, 2023

Can the muddatagrid component support remote querying, filtering, and sorting on the backend api?

@henon
Copy link
Collaborator

henon commented Apr 8, 2023

@neozhu you should post this in its own issue and tag @tjscience.

@SinisterMaya could you please resolve the conflicts? we recently merged a lot of datagrid PRs. I think I'll merge yours now.

@SinisterMaya SinisterMaya force-pushed the MudDataGrid/SortDefinition-IComparer branch from c13b523 to 5de54cd Compare April 8, 2023 16:18
@SinisterMaya
Copy link
Contributor Author

@henon I rebased and merged. It was only the scoped nullable context that was added to the SortDefinition.cs file

@henon henon changed the title MudDataGrid - SortDefinition: Added a IComparer<object> to allow user to add custom Comparer MudDataGrid: Allow user to use a custom IComparer<object> for sorting Apr 8, 2023
@henon henon merged commit a499a1a into MudBlazor:dev Apr 8, 2023
3 checks passed
@henon
Copy link
Collaborator

henon commented Apr 8, 2023

Thanks a lot!

@tjscience FYI

ilovepilav pushed a commit to ilovepilav/MudBlazor that referenced this pull request Nov 25, 2023
…MudBlazor#6368)

* MudDataGrid - SortDefinition: Added a IComparer<object> to allow user to add custom Comparer

Co-authored-by: Yan Gauthier <yan.gauthier@genikinc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants