primefaces icon indicating copy to clipboard operation
primefaces copied to clipboard

Datatable: make Features replacable again

Open giemes opened this issue 4 years ago • 5 comments

Description

With Primefaces 11, the DataTable and its components saw some revisions that are clashing with how we used it in previous versions.

We have extended the SortFeature, and specifically modified how compare() works within it, as we are dealing with message keys whose localized messages should be compared for sorting, not the message key itself.

With PF11, compare() is now a static method, the DataTable Features are now singletons accessed by getInstance(), and those singletons reference each other (e.g. FilterFeature directly calls SortFeature.getInstance()) making minimally invasive modifications impossible:

// update filtered value accordingly to take account sorting
if (table.isSortingCurrentlyActive()) {
  SortFeature.getInstance().sort(context, table);
}

The DataTable FEATURES list is also static and unmodifiable, requiring us to overwrite decode() and encodeEnd() just to replace a feature in the list.

From PF11 onward, we have to basically copy and replace the entire implementation and all features, which will be a continuous pain with future updates.

Describe the solution you would like

Refactor the structure to allow easy modification of features, for example by using protected methods to (1) retrieve the list of features, and (2) retrieve the instance of a feature, that subclasses can extend and modify. The reference implementation should use those methods instead of obtaining singletons directly.

Keep certain methods that can change with different use cases, like compare(), as instance methods instead of making them static.

Thank you for your consideration.

giemes avatar Nov 09 '21 12:11 giemes

couldn't you use sortFunction on p:column?

Its by design that they are not replaceable anymore as this are internal artifacts

tandraschko avatar Nov 09 '21 12:11 tandraschko

https://primefaces.github.io/primefaces/11_0_0/#/components/datatable?id=sorting

tandraschko avatar Nov 09 '21 12:11 tandraschko

This could be easily fixed using SPI

Rapster avatar Nov 09 '21 12:11 Rapster

couldn't you use sortFunction on p:column?

Its by design that they are not replaceable anymore as this are internal artifacts

We use this functionality in basically all of our substantial number of tables, and our current solution allowed to extend the functionality we needed in a central spot that is always available, instead of having to carry a sortFunction declaration everywhere.

Is it not preferrable to still allow users to overwrite single features with our own implementation, instead of requiring us to subclass the entire DataTable class and copy large amounts of code for a few specific changes in features that we require? We'd rather avoid diverting from upstream code too much, and extendable features would be an elegant solution for that (which is why we used it till PF11 even if it wasn't explicitly supported)

giemes avatar Nov 09 '21 15:11 giemes

in the past it was replaceable BUT not by design everything replaceable has a official SPI (like VirusScanner) or attribute on the tags (like sortFunction)

in general i would suggest to use sortFunction as the official way instead instead of introducing a SPI which makes such component specific internal code replaceable

otherwise im ok to make this replaceable in a right, official, SPI way and make it also reuseable by other components like TreeTable

tandraschko avatar Nov 09 '21 15:11 tandraschko

I agree with @tandraschko on this. Just exposing this internal logic again is not the right answer. It would make it difficult for core devs to make changes in the future if we have to expect people are overriding it.

Since no one else is really asking for this and there are workarounds to your problem provided I am going to close this.

Another option if you absolutely need this is to just fork the whole component in your own codebase and maintain it since you are modifying internals. Sorry!

melloware avatar Jan 03 '23 00:01 melloware