Skip to content

Commit

Permalink
Fix exact item count resolving if scrolled far (#8692)
Browse files Browse the repository at this point in the history
* Fix exact item count resolving if scrolled far

Adds isCountEstimated() to ItemCountChangeEvent.

If item count estimate allows the user to scroll way past the exact count,
DataCommunicator will now resolve the exact count immediately instead of
returning 0 items and new size to the client, which would have to keep
requesting items until the end was reached.

Fixes #8643

* Fix sonar issues

Co-authored-by: Mikhail Shabarov <mikhail@vaadin.com>
  • Loading branch information
pleku and mshabarov committed Jul 6, 2020
1 parent ff19799 commit 7337310
Show file tree
Hide file tree
Showing 8 changed files with 237 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public AbstractDataView(
@Override
public Registration addItemCountChangeListener(
ComponentEventListener<ItemCountChangeEvent<?>> listener) {
Objects.requireNonNull(listener, "SizeChangeListener cannot be null");
Objects.requireNonNull(listener, "ItemCountChangeListener cannot be null");
return ComponentUtil.addListener(component, ItemCountChangeEvent.class,
(ComponentEventListener) listener);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public class DataCommunicator<T> implements Serializable {
private int itemCountEstimate = -1;
private int itemCountEstimateIncrease = -1;
private boolean definedSize = true;
private boolean skipSizeCheckUntilReset;
private boolean skipCountIncreaseUntilReset;
private boolean sizeReset;
private int pageSize;

Expand Down Expand Up @@ -183,7 +183,7 @@ public void setRequestedRange(int start, int length) {
* It effectively resends all available data.
*/
public void reset() {
skipSizeCheckUntilReset = false;
skipCountIncreaseUntilReset = false;
sizeReset = true;
resendEntireRange = true;
dataGenerator.destroyAllData();
Expand Down Expand Up @@ -429,7 +429,7 @@ public void setCountCallback(
}
this.countCallback = countCallback;
definedSize = true;
skipSizeCheckUntilReset = false;
skipCountIncreaseUntilReset = false;
// there is no reset but we need to get the defined size
sizeReset = true;
requestFlush();
Expand Down Expand Up @@ -457,7 +457,7 @@ public void setItemCountEstimate(int itemCountEstimate) {
this.itemCountEstimate = itemCountEstimate;
this.countCallback = null;
definedSize = false;
if (!skipSizeCheckUntilReset
if (!skipCountIncreaseUntilReset
&& requestedRange.getEnd() < itemCountEstimate) {
sizeReset = true;
requestFlush();
Expand All @@ -477,8 +477,8 @@ public int getItemCountEstimate() {
}

/**
* Sets the item count estimate increase to use and switches the component to
* undefined size if not yet used. Any previously set count callback is
* Sets the item count estimate increase to use and switches the component
* to undefined size if not yet used. Any previously set count callback is
* cleared. The step is used the next time that the count is adjusted.
* <em>NOTE:</em> the increase should be greater than the
* {@link #setPageSize(int)} or it may cause bad performance.
Expand Down Expand Up @@ -517,8 +517,8 @@ public int getItemCountEstimateIncrease() {
* count callback. Calling with value {@code true} will use the
* {@link DataProvider#size(Query)} for getting the size. Calling with
* {@code false} will use whatever has been set with
* {@link #setItemCountEstimate(int)} and increase the count when needed with
* {@link #setItemCountEstimateIncrease(int)}.
* {@link #setItemCountEstimate(int)} and increase the count when needed
* with {@link #setItemCountEstimateIncrease(int)}.
*
* @param definedSize
* {@code true} for defined size, {@code false} for undefined
Expand All @@ -528,7 +528,7 @@ public void setDefinedSize(boolean definedSize) {
if (this.definedSize != definedSize) {
this.definedSize = definedSize;
countCallback = null;
skipSizeCheckUntilReset = false;
skipCountIncreaseUntilReset = false;
if (definedSize) {
// Always fetch explicit count from data provider
requestFlush();
Expand Down Expand Up @@ -734,7 +734,11 @@ private void handleDetach() {
}

private void requestFlush() {
if (flushRequest == null) {
requestFlush(false);
}

private void requestFlush(boolean forced) {
if (flushRequest == null || forced) {
flushRequest = context -> {
if (!context.isClientSideInitialized()) {
reset();
Expand Down Expand Up @@ -771,7 +775,8 @@ private void flush() {
// With defined size the backend is only queried when necessary
if (definedSize && (resendEntireRange || sizeReset)) {
assumedSize = getDataProviderSize();
} else if (!definedSize && (!skipSizeCheckUntilReset || sizeReset)) {
} else if (!definedSize
&& (!skipCountIncreaseUntilReset || sizeReset)) {
// with undefined size, size estimate is checked when scrolling down
updateUndefinedSize();
}
Expand All @@ -792,7 +797,20 @@ private void flush() {
// the end has been reached
assumedSize = requestedRange.getStart()
+ activation.getActiveKeys().size();
skipSizeCheckUntilReset = true;
skipCountIncreaseUntilReset = true;
/*
* If the fetch query returned 0 items, it means that the user
* has scrolled past the end of the exact item count. Instead of
* returning 0 items to the client and letting it incrementally
* request for the previous pages, we'll cancel this flush and
* tweak the requested range and flush again.
*/
if (assumedSize != 0 && activation.getActiveKeys().isEmpty()) {
int delta = requestedRange.length();
requestedRange = requestedRange.offsetBy(-delta);
requestFlush(true); // to avoid recursiveness
return;
}
}
effectiveRequested = requestedRange
.restrictTo(Range.withLength(0, assumedSize));
Expand Down Expand Up @@ -832,7 +850,9 @@ private void fireItemCountEvent(int itemCount) {
.getComponent();
if (component.isPresent()) {
ComponentUtil.fireEvent(component.get(),
new ItemCountChangeEvent<>(component.get(), itemCount));
new ItemCountChangeEvent<>(component.get(), itemCount,
!(isDefinedSize()
|| skipCountIncreaseUntilReset)));
}
lastSent = itemCount;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,19 @@ public interface DataView<T> extends Serializable {
void refreshItem(T item);

/**
* Add an item count change listener that is fired when the item
* count changes. This can happen for instance when filtering the data set.
* Add an item count change listener that is fired when the item count
* changes. This can happen for instance when filtering the items.
* <p>
* Item count change listener is bound to the component and will be
* retained even if the data changes by setting of a new items or
* Item count change listener is bound to the component and will be retained
* even if the data changes by setting of a new items or
* {@link DataProvider} to component.
* <p>
* <em>NOTE:</em> when the component supports lazy loading (implements
* {@link HasLazyDataView}) and a count callback has not been provided, an
* estimate of the item count is used and increased until the actual count
* has been reached. When the estimate is used, the event is fired with the
* {@link ItemCountChangeEvent#isItemCountEstimated()} returning
* {@code true}.
*
* @param listener
* item count change listener to register
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@
import com.vaadin.flow.component.ComponentEvent;

/**
* Event describing the item count change for a component.
* The {@link ItemCountChangeEvent} will fired during the "before client
* Event describing the item count change for a component. The
* {@link ItemCountChangeEvent} will fired during the "before client
* response"-phase, so changes done during the server round trip will only
* receive one event.
* For example, this code will trigger only one event, although there are
* two methods called which cause the item count change:
* receive one event. For example, this code will trigger only one event,
* although there are two methods called which cause the item count change:
*
* <pre>
* {@code
* dataView.addItemCountChangeListener(listener);
Expand All @@ -33,30 +33,54 @@
* }
* </pre>
*
* @param <T> the event source type
* @param <T>
* the event source type
* @since
*/
public class ItemCountChangeEvent<T extends Component> extends ComponentEvent<T> {
private int itemCount;
public class ItemCountChangeEvent<T extends Component>
extends ComponentEvent<T> {
private final int itemCount;
private final boolean itemCountEstimated;

/**
* Creates a new event using the given source and indicator whether the
* event originated from the client side or the server side.
*
* @param source the source component
* @param itemCount new items count
* @param source
* the source component
* @param itemCount
* new items count
* @param itemCountEstimated
* whether item count is an estimate
*/
public ItemCountChangeEvent(T source, int itemCount) {
public ItemCountChangeEvent(T source, int itemCount, boolean itemCountEstimated) {
super(source, false);
this.itemCount = itemCount;
this.itemCountEstimated = itemCountEstimated;
}

/**
* Get the new items count for the component data.
* Get the new item count for the component.
*
* @return items count
*/
public int getItemCount() {
return itemCount;
}

/**
* Returns whether the item count {@link #getItemCount()} is an estimate or
* the exact count. An estimate is used when items are fetched lazily from
* the backend and the count callback has not been provided. See further
* details from {@link LazyDataView#setItemCountEstimate(int)}.
* <p>
* <em>NOTE:</em> this only applies for components that do lazy loading from
* the backend and implement {@link HasLazyDataView}.
*
* @return {@code true} when the count is an estimate, {@code false} when
* the count is exact
*/
public boolean isItemCountEstimated() {
return itemCountEstimated;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public void addItemCountChangeListener_fireEvent_listenerNotified() {
event -> fired.compareAndSet(0, event.getItemCount()));

ComponentUtil.fireEvent((Component) component,
new ItemCountChangeEvent<>((Component) component, 10));
new ItemCountChangeEvent<>((Component) component, 10, false));

Assert.assertEquals(10, fired.get());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public void addItemCountChangeListener_fireEvent_listenerNotified() {
event -> fired.compareAndSet(0, event.getItemCount()));

ComponentUtil
.fireEvent(component, new ItemCountChangeEvent<>(component, 10));
.fireEvent(component, new ItemCountChangeEvent<>(component, 10, false));

Assert.assertEquals(10, fired.get());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ public void addItemCountChangeListener_itemsCountChanged_newItemCountSuppliedInE
dataView.addItemCountChangeListener(event -> {
Assert.assertEquals("Unexpected item count", 1,
event.getItemCount());
Assert.assertFalse(event.isItemCountEstimated());
invocationChecker.set(true);
});

Expand Down
Loading

0 comments on commit 7337310

Please sign in to comment.