DataTable: cleanup sorting

[tandraschko]

Filters previous had 3 similar attributes: Filters FilterBy FilterMeta

we should check the same for sorting.

All properties related to sort:

• sortMeta
• sortField
• sortBy
• sortOrder
• sortFunction
• sortMode (single | multiple)
• allowUnsorting

Some facts: 1) it seems like sortBy and sortField is similar. One points to a fieldname of the entity, one is a VE? @Rapster Maybe you have a better overview here? 2) if sortMode = multi, sortMeta is the source of all sort informations 3) if sortMode = single, sortBy+sortOrder+sortFunction is used 4) in general, sortMeta is just a collection of (sortBy+sortOrder+sortFunction) for mulitple columns. So multiSort is actually the same thing as singleSort, just that sortMeta could only contain 1 value instead of multiple.

Goals (Reduce attributes to):

• sortBy (a map of SortMeta, multisort will contain mulitple values, singlesort only 1 value)
• initial sorting can by defined by sortOrder and sortPriority on p:column
• allowMultiSort (sortMode before, default = true?)
• allowUnsorting
• remove old attributes from PropertyKeys+taglib and throw a exception in getter/setter

[tandraschko]

All properties related to sort:

• sortMeta
• sortField
• sortBy
• sortOrder
• sortFunction
• sortMode (single | multiple)
• allowUnsorting

Some facts: 1) it seems like sortBy and sortField is similar. One points to a fieldname of the entity, one is a VE? @Rapster Maybe you have a better overview here? 2) if sortMode = multi, sortMeta is the source of all sort informations 3) if sortMode = single, sortBy+sortOrder+sortFunction is used 4) in general, sortMeta is just a collection of (sortBy+sortOrder+sortFunction) for mulitple columns. So multiSort is actually the same thing as singleSort, just that sortMeta could only contain 1 value instead of multiple.

Goals (Reduce attributes to):

• sortBy (a map of SortMeta, multisort will contain mulitple values, singlesort only 1 value)
• sortMode (single | multiple)
• allowUnsorting

i think we should even change sortMode to a bool attribute like allowMultiSort, so that allowUnsorting and sortMode are similar. Should it be true per default?

[tandraschko]

any thoughts @melloware @christophs78 ?

[melloware]

I like the idea.

I think the only problem now is if someone has sortBy='#{el1}' sortOrder='ascending' sortFunction='#{el2}' declared in EL in the UI now they have to switch to sortBy='#{bean.sortMeta} containing a single META entry and change their code completely to have the new Java method that builds this SortMeta?

That might be a pretty drastic change for a lot of people.

[melloware]

Couldn't we leave everything as is now and just throw Better FacesException if someone is using multiplemode and has any of those 3 props defined and vice versa for single mode if it has sortMeta defined?

[tandraschko]

I only talk about DataTable attributes - not column!

[melloware]

I thought on a DataTable you could delcare those on the Datatable in "single" mode to define the default sorted column no?

[melloware]

See I documented all the sort attributes in 9.0 here: https://primefaces.github.io/primefaces/9_0/#/components/datatable

[tandraschko]

ok, maybe we could do something like: initialSortBy which takes the SortMeta object or a List of SortMeta same for filtering

[melloware]

so yeah in Multiple Mode if a SortMeta is passed in does become the default sorting. So your idea of using a single SortMeta field to do it all will work, I was just pointing out it might be a drastic change for people using sortBy='#{el1}' sortOrder='ascending' sortFunction='#{el2}' but I am ok with it as long as we have it in migration notes.

[tandraschko]

we just need to collect cases and do a clean redefine. sortBy,order,function on DataTable is just used for initial sort? Later by,order,function on columns is used?

[melloware]

That is correct Those 3 fields I believe Are just used when the table is loaded the first time as defaults. Then column values take over once users start manually clicking columns.

[tandraschko]

yeah, so i would definitily we should wrap this in a initialSortBy - currently its just weird and confusing

[melloware]

I 100% agree its confusing.

[tandraschko]

So sortBy='#{entity.field}' on p:dataTable says that the initial-sorted-field?

[christophs78]

Just following your discussion. This is a change where dev´s do not get compile-time-errors. Maybe we should do a bit more than only write migration notes. (eg deprecate all old/renamed attributes and write log / throw Exceptions OR write a small tool which scan´s all XHTML´s within a project and generate an HTML-report OR ...) Cleanup this up would make sense long-term. We just should be sure it´s worth the price.

[melloware]

To answer your question @tandraschko yes sortBy='#{entity.field} means that field is the default one sorted when the table is rendered the first time.

@christophs78 I agree with you we need to throw exceptions to let people know exactly what they need to fix.

[tandraschko]

yeah, we can easily remove taglib + propertyKeys and throw exceptions in getter/setter

[tandraschko]

updated the decription

[Rapster]

General talk

I'm glad you bring that up, that's a topic I wanted to discuss for a while now. But for now, I'm gonna stick to your last comment. First, there are 2 sets of sort attributes - the ones defined on DataTable - the others defined on p:column/p:columns I never had to use sort properties on DataTable, always been using p:column. I actually don't see exactly how it would work... Maybe you can help me out before I say to get rid of them 😅 But my guess is that it's used for default sorting. As I spent a lot of time on DataTable component impl, I can see part of the complexity of this component is due to these attributes, it's a lot of fiddling to decide which attributes to consider, add to that the trick on sortBy expression string and it actually becomes hell. My idea on this ticket, is to simplify the use and meaning of this attributes for everyone.

Description of attributes

Second, here is my point of view of what those attributes you just mentionned: - Column#field: the most important and handy attribute in p:column IMO, however the less used - DataTable#sortMeta: that structure should be a map (UIColumn#columnKey being the key) used internally to the component but not exposed to the end-user. Basically, this map should gather every infos from every `p:column` when the component is created. At this point, this map should be the only thing to work with afterwards. If you need to have default sorts, then initialize your p:column(s) with a sortOrder attribute - Colum#sortBy: attribute to indicate on which field to sort on. Basically, it's a (slightly) redundant attribute, unless you sort your column with a different value (i18n). Also used by row grouping feature, see #6372 - Colum#sortFunction: custom way of sorting, for complex data? - DataTable#sortOrder: again I would put this attribute on p:column not datatable - DataTable#sortMode: whether you can sort multi columns. After this coment https://github.com/primefaces/primefaces/issues/5438#issuecomment-695771805, it should be possible to have default sorters on a single sort mode datatable - sortField: another duplicate of `Column#field`

To sum up:

Remove:

• DataTable#sortMeta (replaced by DataTable#sortBy)
• DataTable#sortField (replaced by UIColumn#field)
• DataTable#sortOrder (replaced by UIColumn#sortOrder)
• DataTable#sortFunction (replaced by UIColumn#sortFunction)
• DataTable#expandableRowGroups (replaced by HeaderRow#expandable)

Update

• DataTable#sortMode: default value "multiple"
• DataTable#allowUnsorting" default value "false"
• DataTable#sortBy: expects a single or a collection of SortMeta (default sorting can either be configured through markup or programmatically)

Add

• Column#sortPriority: defines in which order columns sorter should apply initially
• Column#sortOrder

• 6372

Breaking changes

• DataTable#sortBy: no longer expect a Map, but a single or a collection of SortMeta (use SortMeta#builder) UIColumn#field must be set to enable default sorting.
• As it is possible from now on to configure default sorted columns on a single sort mode datatable. Users will be expected to implement only one method LazyDataModel#load(int first, int pageSize, Map<String, SortMeta> sortBy, Map<String, FilterMeta> filterBy)

[tandraschko]

So you can think we can manage initial sortBy only via the order attr on p:column?

sortBy/sortFunction: currently i think sortBy is to define the default sorted field and could be replaced by order-attr on p:column.

[Rapster]

So you can think we can manage initial sortBy only via the order attr on p:column?

Yes, if you define something like <p:column headerText="Year" field="year" order="asc" />, then you want that field to be sorted asc by default

sortBy/sortFunction: currently i think sortBy is to define the default sorted field and could be replaced by order-attr on p:column.

Indeed, whichever column having a non null order attribute represent default sorters

[christophs78]

Started writing integration-tests for our current initial-sort-features and run into some questions:

1. Moving sortOrderto p:column is not ideal combined with our default sortMode="single" as it may suggest you may be able to sort mutliple columns. Does it make sense to default sortMode to multiple? (Your users still need to know the meta-key to enable it.)
2. How do we prioritice sortOrder on p:column for sortMode="multiple"? Primary order may be by column 3, secondary order may be by column 1 or inversed.

[christophs78]

Additional question: Do we still plan this for PF 9. Or do we target PF 10?

[Rapster]

Not sure I follow, how column ordering impacts selection?

[christophs78]

Not sure I follow, how column ordering impacts selection?

Sorry, i meant sortMode. Just fixed this in my comment.

[Rapster]

1. I see what you mean, something like that for example:

<p:dataTable var="car" value="#{dtBasicView.cars}" sortMode="single">
            <p:column headerText="Id" field="id" sortable="false" />

            <p:column headerText="Year" field="year" order="asc" />

            <p:column headerText="Brand" field="brand" order="desc" />

            <p:column headerText="Color" field="color" order="asc"/>
</p:dataTable>

Let me quote what datatables.net says from here:

Note that disabling this ability does not impede your ability as a developer to do multiple column ordering using columns.orderData, order or order(), it just disallows the user from performing their own multi-column order.

I think it's a very good option, basically you will initialize a default multi sorting column, however the user won't be able to make his own. Plus, by default sortMode should be multiple and not single. I don't think it is a common use case to forbid multi sort column

2. Same as today, first column selected has the highest priority n, and the rest has n - 1 priority

[christophs78]

Plus, by default sortMode should be multiple and not single. I don't think it is a common case to forbid multi sort column

Agreed sortMode=multiple should be default.

[christophs78]

It´s to some degree a question of trade-offs. When i use your example with sortMode="mulitple" i can´t do a initial primary by brand desc and secondary by year asc. When you look at https://github.com/primefaces-extensions/primefaces-integration-tests/pull/42/files#diff-23941c255d6cc359471c4439bae1090dR52-R59 you can currently to this. But with a not ideal API. (especially the columnKey is not ideal) Personally doing this - like you suggest - via markup is more straight-forward.

[Rapster]

Yeah I see, the trick in your code is to use a LinkedHashMap to set a priority or am I wrong? But if I'm right, maybe we should introduce a new attribute sortPriority:

<p:dataTable var="car" value="#{dtBasicView.cars}" sortMode="single" sortPriority="brand, year">
            <p:column headerText="Id" field="id" sortable="false" />

            <p:column headerText="Year" field="year" order="asc" />

            <p:column headerText="Brand" field="brand" order="desc" />

            <p:column headerText="Color" field="color" order="asc"/>
</p:dataTable> 

[melloware]

So we have had our own custom Datatable based on Lazy Datatable for years and I can say we definitely need a sortPriorty for this function. Our hack has always looked like this where I looked for custom f:attributes.

<p:column headerText="Id" styleClass="col-right" sortBy="#{row.id}" filterBy="#{row.id}" width="100">
    <h:outputText value="#{row.id}" />
    <f:attribute name="sortOrder" value="1" />
    <f:attribute name="sortDirection" value="ascending" />
</p:column>

So having new column attributes for this would be great. And my personal preferences would be sortOrder and sortDirection.

Also we should target this for 10.0 as 9.0 I think PrimeTek is shooting for October but I could be wrong?

[Rapster]

IMO, having Column#sortOrder and DataTable#sortPriority should be enough for what we mentionned previously. Regarding the target, If I understood well from Cagatay OneVision video, 9.0 should be skipped, and 1.0 will be the next PF version

[melloware]

@Rapster sort priority on the Datatable not the column?

Then how would you do this?

<p:column headerText="Id"  sortBy="#{row.id}">
<p:column headerText="Name"  sortBy="#{row.name}" sortPriority="2" sortOrder="desc">
<p:column headerText="Status"  sortBy="#{row.status}" sortPriority="1" sortOrder="asc">

In the above I have 3 columns and I wanted sorted by STATUS ASC, NAME DESC. So only sort by Name if they have equal Status? So I need priority on the Column no?

[tandraschko]

SortPriority must be on column and SortaMeta of course Added it to the Initial text

[Rapster]

@melloware I think the sample I provided should work for your use case:

<p:dataTable var="car" value="#{dtBasicView.cars}" sortMode="single" sortPriority="status, name">
    <p:column headerText="Id" field="id">
    <p:column headerText="Name" field="name" sortOrder="desc">
    <p:column headerText="Status" field="status" sortOrder="asc">
</p:dataTable> 

Either it's on DataTable or Column is more or less the same, except:

• if attribute on column, then you have to work with numbers. More you have columns, more you have to play around with these priorities (which of course is not difficult) but not ideal. Another reason is: responsability, Column shouldn't set his own priority over other columns.

• if attribute on DataTable, you don't bother with numbers, the order of appearance of DataTable#sortPriority defines it (it's more readable). In terms of responsability, DataTable is a higher-level of Column which is responsible of his own columns .

IMO, sortPriority should be on DataTable and can also be on SortMeta (getter only).

[Rapster]

@tandraschko I just realized you assigned this issue to yourself... Let me know if you're still planning to work on it while I started to work on this already

[melloware]

@Rapster I also meant to answer you about sortPriority being on the datatable. It doesn't make sense to put sort columns and priority in two different places it should either be...

<p:dataTable var="car" value="#{dtBasicView.cars}" sortMode="single" sortPriority="status, name" sortOrder="asc, desc">

or

 <p:column headerText="Name" field="name" sortOrder="desc" sortPriority="2">
 <p:column headerText="Status" field="status" sortOrder="asc" sortPriority="1">

But not a mixture of both I think that is confusing to users and doesn't put the metadata on the right tag it belongs on no?

[tandraschko]

i dont think i can work on it the next 2-3 weeks. So if we agree on all points (in the initial description here), feel free to work on it.

[Rapster]

Alright then, not sure if the description is updated. My notes are here: https://github.com/primefaces/primefaces/issues/5438#issuecomment-685700032

@melloware I don't see this as a priority right now, let's discuss again once the other points are covered

[tandraschko]

Not yet, i dont see a reason to still have sortaMeta attr. All can be in sortBy

[Rapster]

I started a PR. It's a good draft IMO, everything from showcase is working except table state. In addition to that, it's possible from now on to multi sort with headerRow and summaryRow. I need to merge changes that you @melloware recently made on headerRow

[Rapster]

This PR is ready for review. I'm quite happy about the result 😄

[Rapster]

I'm thinking also to fix issue #3347, e.g moving DataTable#nullSortOrder to UIColumn , and change order of execution of checks. In case, UIColumn#sortFunction is defined, it deals with nullsortOrder otherwise usual checks. WDYT?

[melloware]

I like it. The more we move to UI Column the more sense i think it makes.

[Rapster]

There is also DataTable#caseSensitiveSort...

[melloware]

@Rapster I am now getting an unexpected error

javax.faces.FacesException: var and field must be non null
    at org.primefaces.component.datatable.DataTable.createValueExprFromVarField(DataTable.java:1429)
    at org.primefaces.model.SortMeta.of(SortMeta.java:105)
    at org.primefaces.component.datatable.DataTable.initSortBy(DataTable.java:1442)
    at org.primefaces.component.datatable.DataTable.lambda$getSortByAsMap$5(DataTable.java:1546)
    at org.primefaces.util.ComponentUtils.computeIfAbsent(ComponentUtils.java:562)
    at org.primefaces.component.datatable.DataTable.getSortByAsMap(DataTable.java:1546)
    at org.primefaces.component.datatable.DataTable.isDefaultSort(DataTable.java:1136)
    at org.primefaces.component.datatable.DataTableRenderer.preRender(DataTableRenderer.java:108)
    at org.primefaces.component.datatable.DataTableRenderer.encodeEnd(DataTableRenderer.java:94)

with this table:

<p:dataTable id="reorderTable" var="car" value="#{dtReorderView.cars2}" reflow="true" draggableRows="true">
    <p:column headerText="Id">
        <h:outputText value="#{car.id}" />
    </p:column>
    <p:column headerText="Year" >
        <h:outputText value="#{car.year}" />
    </p:column>
    <p:column headerText="Brand" >
        <h:outputText value="#{car.brand}" />
    </p:column>
    <p:column headerText="Color">
        <h:outputText value="#{car.color}" />
    </p:column>
    <p:headerRow>
        <p:column colspan="2" style="text-align:left">
            <h:outputText value="Test Left" />
        </p:column>
        <p:column colspan="2" style="text-align:right">
            <h:outputText value="Test Right" />
        </p:column>
    </p:headerRow>
</p:dataTable>

I don't see why this table is not valid when I have no sorting or filtering defined on any column? Is it now saying we MUST have "field" on every column but what if my column is a virtual column that does not map to any value in the bean?

Attached is the reproducer: pf-5296.zip

[tandraschko]

+1 melloware please also add such example to the showcase if not already there

[melloware]

OK the error is misleading it is because now headerRow requires a groupBy or field attribute.

<p:headerRow groupBy="#{car.brand}">

So I think its that the error message is misleading. If it had said "HeaderRow requires either groupBy or field attribute set" I would have gotten it right away.