tlaukkan / vaadin-lazyquerycontainer

Lazy Query Container is a lazy loading container implementation for accessing data from business services.
http://vaadin.com/directory#addon/117
39 stars 73 forks source link

ItemSetChange event not fired after adding/removing filters to container #47

Closed rste79 closed 9 years ago

rste79 commented 11 years ago

Hi, I've attached an ItemSetChangeListener to the container (LazyQueryContainer version 2.0.8) but this event is not fired when a filter is added/removed, even if adding/removing the filter actually changes the item set. Thanks in advance, Stefano

guentherwieser commented 11 years ago

Can confirm the behavior as described in https://vaadin.com/forum#!/thread/186858/3207980.

tlaukkan commented 11 years ago

I see adding this feature as counter productive. If you still want to push it please add pull request for a fix where entity query has boolean which you can set with getters and setters to true if you want to item set change listener to be invoked automatically after filter change. Please do not add it to constructors. Name suggestion: refreshOnFilterChange Please add good unit tests as well.

guentherwieser commented 11 years ago

I've implemented a workaround for this (depending on the table that one uses either via Listeners or by implementing FilterGenerator (for users of FilterTable)), so it works fine now. But it would be great if this behavior is at least documented. It breaks the contract of the interface Container$Filterable that LazyQueryContainer implements.

tlaukkan commented 11 years ago

Which ever way I turn this issue the result is that to enable correct behavior from end user perspective the automatic item set change event should not be enabled. For example when you first filter to see cities of Sweden and then user switches to filter cities of Norway automatic item set change event on filter change would cause unnecessary query for all cities when the old filter is removed. Whether current functionality breaches interface contract or not, the fact is that unnecessary database query for full set of items on each filter change is a bug in application and serious if the database table is large and queries take time. We also need to take into account that this addon is especially targeted for large tables as it is lazy container. Nevertheless I am willing to apply a pull request with functionality mentioned in my previous comment (if someone is willing to shoot ones application in the leg) ;).

guentherwieser commented 11 years ago

We're using a very fast DB layer that usually returns filtered results in less than 200ms, even though the table holds millions of records. Nevertheless, the best and minimum would be to document this unexpected behavior of an implementor of Container$Filterable. Then it's up to the programmer how to implement this if needed, which is very easy to do.

rste79 commented 11 years ago

Hi, I really appreciate your efforts in pondering the pros and cons of this change. There are two aspects that form this issue and that should not be mixed together: the first is the correctness of the implementation and the second is the impact on application performance. I think that the first should be your primary concern and the second should be a concern of the developer that is using your container: there are many use case that involves huge data sources that are really fast to query... it's just a matter of optimizing the data source. To prevent a possible bad usage of the data source by breaking the contract of the ItemSetChangeNotifier can be considered a premature optimization and, as Donald Knuth said, it is the root of all evil :-)

tuscland commented 8 years ago

Hello @tlaukkan,

How do I enable the behavior your described on Jun 14, 2013? I have spurious requests when filters are removed. I just want one request to happen when the user is done selecting options.

The item set change notification behavior is sealed with final or private methods, so there is no way I can extend or modify it.

Best, Cam

tlaukkan commented 8 years ago

Hi @tuscland

A patch introduces behavior that refreshes container after each filter change which can run multiple unnecessary queries for example when multiple filters are removed. This behavior was introduced in a pull request and I missed the fact when accepting it. It is probably good time to think about adding functionality for mass removing filters in LazyQueryContainer API so that it disabled the filters at that point.

For now you can set query size to 0 before removing filters. If you are using LazyEntityQuery this disables queries. If you have custom query you can implement the same behavior. After removing filters you can reset query size to correct value and refresh your view.

Br, Tomi

tuscland commented 8 years ago

Great, thank you!