Flow: Grid.Column.setSortProperty does nothing for in-memory data providers

[mvysny]

When I call Grid.Column.setSortProperty("foo") I expect the ListDataProvider to sort by the property as well.

Currently, the setSortProperty makes the column sortable but leaves the default comparator as-is. The old comparator always returns 0 and so it does not perform any actual sorting. Therefore setting sort property will only take effect for the “external backend” DataProvider and it will do nothing with in-memory DataProvider such as ListDataProvider. The setSortProperty() should therefore also call setComparator() with appropriate comparator as well.

[Legioth]

Sorting based on a property name, e.g. "foo", would require using reflection to find a corresponding getter and then hope the return type of that getter is Comparable. The problem here is that we won't necessarily even have a Class instance for the item type and we can thus not even do any reflection stuff until acting on actual item instances, which might each have a different getClass() return value that just happens to be sharing a common super class.

[mvysny]

Yes, you're right. However in-memory data source is not expected to contain massive amount of items; also Introspected beans are cached (or you could cache the Class/property name/Method triplet) so this is doable. Casting stuff to Comparable sucks, but it will work properly in most cases; in the worst case for non-comparable values you'll at least get an exception which may contain a proper message indicating the origin of the failure. IMHO that is way better than the current behaviour of silently doing nothing.

[Legioth]

Might be better, though there are some additional problems:
1) When there's an explosion because of any of the potential problems with relying on reflection and optimistic casting, the call to setSortProperty("foo") won't be on the stacktrace, which in turn means that it might be really hard for the application developer to trace the origin of the problem.
2) We would also have to define behaviour in cases when both setSortProperty and setComparator is used.

[mvysny]

1. It's not too late and we still don't have to care for backward compatibility that much. We may still design Grid to not to have zero-arg constructor and to require the Class parameter. In that case I would strongly advise to not to auto-generate columns: https://github.com/mvysny/karibu-dsl/issues/4
2. If not 1., then the exception message could indicate to fix the parameter in setSortProperty()
3. If setComparator() is called, it should override anything setSortProperty() introduced; also any later call to setSortProperty() should not override anything configured by a manual call to setComparator().

[denis-anisimov]

Won't this PR: https://github.com/vaadin/vaadin-grid-flow/pull/164 fix the issue ?

[pleku]

would strongly advise to not to auto-generate columns

@mvysny Not an option, this a very handy productivity feature and quite much used.

@denis-anisimov it would seem so, but should be easy to add/modify a test to verify that #164 fixes also default sorting when using setSortProperty ?

[denis-anisimov]

Sure. The question : what's the resulting expectation of this ticket ?
I see here a long discussion which includes some suggestion about reflection....
That's why I'm asking a question: it's not clear what's the final requirements of this ticket.
Don't really want to go through all the comments and just asking whether the current state already enough for this ticket.

[mvysny]

The expectation was that Grid.Column.setSortProperty("foo") would also set the Comparator which reflectively reads the property "foo" instead of polling the data provider. Yet I guess it makes little sense to set some other property than what's being read in the ValueProvider; and since https://github.com/vaadin/vaadin-grid-flow/pull/164 pre-sets the comparator to comparing values from the ValueProvider, I'd say this bug is fixed.

[denis-anisimov]

Actually sort property doesn't relate to the comparator at all (with the current implementation).

setSortProperty() effectively makes the column sortable.
The comparator is available only when it's explicitly set or if addColumn(ValueProvider) method has been used.
It's possible to use addColumn(Renderer) which doesn't set any comparator.
And in this case reflection or any approach which somehow uses the property still makes sense.

So :

• if the ticket is considered as fixed then I don't see how it may be tested since there is nothing to test. setSortProperty() has no any relation to the way how Column has been created and whether the good comparator is available.
• if the ticket is not fixed then it should be fixed somehow (and the exact expectation should be written here in details).

[pleku]

For an easy example case, as a user I would expect that if I have a column where I use a ValueProvider to combine two properties, eg. first + last name, I can use the setSortProperty to define that
a) this column can be sortable
b) shorting should be happening using the property I've given for the column, eg. firstName

If this is not doable for inmemory / list data provider, I would rather throw for those cases than have things just not work.

[denis-anisimov]

Issue moved to vaadin/vaadin-grid-flow #524 via ZenHub