-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Vaadin ComboBox Filter timeout #6863
Comments
Related to #1031 |
to connector.js let timerValue = 1000
if (comboBox.filterDebouncerTimer) {
timerValue = comboBox.filterDebouncerTimer
}
this._filterDebouncer = Debouncer.debounce(this._filterDebouncer, timeOut.after(timerValue), () => { to java
Can it be done like this? |
Did you @Aleksey2093 try that solution? |
Yes. That #6863 (comment) work |
To me that would sound like a good enough fix. The "components team" has a tendency to keep "client side API" in sync, but not sure if there is this kind of lazy loading for the web component 🤷♂️ The web component docs mentions lazy loading, but not sure if that API has similar throttling as there is in the Flow version (looks like that logic is in the Flow specific "connector.js") 🤷♂️ If you have the patch in your local checkout, I'd just shoot a pull request to get wheels rolling. If you don't, I can try to help as well. |
PoC created based on suggestion by @Aleksey2093 Number of different issues related to topic, like vaadin#6863
I couldn't resist drafting this as PR. The problem was that the project don't compile for me (related to some other component) 🤷♂️
|
FYI: with this change the project compiles "enough" for ComboBox module. Still some issue with grid module. |
I was about to write test for the PR, but got a quite bit WTF moment until I realized how the filtering is implemented, also currently. In addition to actually triggering the filtering on the server side, there is a filter string that is synced with a DOM event, on each key press. So if one types "1234567890", there will 10 XHRs made first and then one more after 500ms (or whatever the timeout is if this change is applied. In addition to being big waste of resources, this may well be a reason why ComboBox feels very slow on certain sites with slowish server connections (like our own demos 🧸). Although as such this PR helps the pressure towards the (typically most) exponsive backendn calls, I think this behaviour ought to be fixed as well. But that then falls into task that I think should be handled by somebody who knows at least something about the current internals. I can hop in to help if they are copied from the old GWT widget 🤓 |
The corrected code: import { Debouncer } from '@polymer/polymer/lib/utils/debounce.js';
import { timeOut } from '@polymer/polymer/lib/utils/async.js';
import { ComboBoxPlaceholder } from '@vaadin/combo-box/src/vaadin-combo-box-placeholder.js';
(function () {
const tryCatchWrapper = function (callback) {
return window.Vaadin.Flow.tryCatchWrapper(callback, 'Vaadin Combo Box');
};
window.Vaadin.Flow.comboBoxConnector = {
initLazy: (comboBox) =>
tryCatchWrapper(function (comboBox) {
// Check whether the connector was already initialized for the ComboBox
if (comboBox.$connector) {
return;
}
comboBox.$connector = {};
// holds pageIndex -> callback pairs of subsequent indexes (current active range)
const pageCallbacks = {};
let cache = {};
let lastFilter = '';
const placeHolder = new window.Vaadin.ComboBoxPlaceholder();
const serverFacade = (() => {
// Private variables
let lastFilterSentToServer = '';
let dataCommunicatorResetNeeded = false;
// Public methods
const needsDataCommunicatorReset = () => (dataCommunicatorResetNeeded = true);
const getLastFilterSentToServer = () => lastFilterSentToServer;
const requestData = (startIndex, endIndex, params) => {
const count = endIndex - startIndex;
const filter = params.filter;
comboBox.$server.setRequestedRange(startIndex, count, filter);
lastFilterSentToServer = filter;
if (dataCommunicatorResetNeeded) {
comboBox.$server.resetDataCommunicator();
dataCommunicatorResetNeeded = false;
}
};
return {
needsDataCommunicatorReset,
getLastFilterSentToServer,
requestData
};
})();
const clearPageCallbacks = (pages = Object.keys(pageCallbacks)) => {
// Flush and empty the existing requests
pages.forEach((page) => {
pageCallbacks[page]([], comboBox.size);
delete pageCallbacks[page];
// Empty the comboBox's internal cache without invoking observers by filling
// the filteredItems array with placeholders (comboBox will request for data when it
// encounters a placeholder)
const pageStart = parseInt(page) * comboBox.pageSize;
const pageEnd = pageStart + comboBox.pageSize;
const end = Math.min(pageEnd, comboBox.filteredItems.length);
for (let i = pageStart; i < end; i++) {
comboBox.filteredItems[i] = placeHolder;
}
});
};
comboBox.dataProvider = function (params, callback) {
if (params.pageSize != comboBox.pageSize) {
throw 'Invalid pageSize';
}
if (comboBox._clientSideFilter) {
// For clientside filter we first make sure we have all data which we also
// filter based on comboBox.filter. While later we only filter clientside data.
if (cache[0]) {
performClientSideFilter(cache[0], params.filter, callback);
return;
} else {
// If client side filter is enabled then we need to first ask all data
// and filter it on client side, otherwise next time when user will
// input another filter, eg. continue to type, the local cache will be only
// what was received for the first filter, which may not be the whole
// data from server (keep in mind that client side filter is enabled only
// when the items count does not exceed one page).
params.filter = '';
}
}
const filterChanged = params.filter !== lastFilter;
if (filterChanged) {
comboBox.setAttribute("inputFinished", false);
cache = {};
lastFilter = params.filter;
let timerValue = 1000
if (comboBox.filterDebouncerTimer) {
timerValue = comboBox.filterDebouncerTimer
}
this._filterDebouncer = Debouncer.debounce(this._filterDebouncer, timeOut.after(timerValue), () => {
if (serverFacade.getLastFilterSentToServer() === params.filter) {
// Fixes the case when the filter changes
// to something else and back to the original value
// within debounce timeout, and the
// DataCommunicator thinks it doesn't need to send data
serverFacade.needsDataCommunicatorReset();
}
if (params.filter !== lastFilter) {
throw new Error("Expected params.filter to be '" + lastFilter + "' but was '" + params.filter + "'");
}
comboBox.setAttribute("inputFinished", true);
// Remove the debouncer before clearing page callbacks.
// This makes sure that they are executed.
this._filterDebouncer = undefined;
// Call the method again after debounce.
clearPageCallbacks();
comboBox.dataProvider(params, callback);
});
return;
}
// Postpone the execution of new callbacks if there is an active debouncer.
// They will be executed when the page callbacks are cleared within the debouncer.
if (this._filterDebouncer) {
pageCallbacks[params.page] = callback;
return;
}
if (cache[params.page]) {
// This may happen after skipping pages by scrolling fast
commitPage(params.page, callback);
} else {
pageCallbacks[params.page] = callback;
const maxRangeCount = Math.max(params.pageSize * 2, 500); // Max item count in active range
const activePages = Object.keys(pageCallbacks).map((page) => parseInt(page));
const rangeMin = Math.min(...activePages);
const rangeMax = Math.max(...activePages);
if (activePages.length * params.pageSize > maxRangeCount) {
if (params.page === rangeMin) {
clearPageCallbacks([String(rangeMax)]);
} else {
clearPageCallbacks([String(rangeMin)]);
}
comboBox.dataProvider(params, callback);
} else if (rangeMax - rangeMin + 1 !== activePages.length) {
// Wasn't a sequential page index, clear the cache so combo-box will request for new pages
clearPageCallbacks();
} else {
// The requested page was sequential, extend the requested range
const startIndex = params.pageSize * rangeMin;
const endIndex = params.pageSize * (rangeMax + 1);
serverFacade.requestData(startIndex, endIndex, params);
}
}
};
comboBox.$connector.clear = tryCatchWrapper((start, length) => {
const firstPageToClear = Math.floor(start / comboBox.pageSize);
const numberOfPagesToClear = Math.ceil(length / comboBox.pageSize);
for (let i = firstPageToClear; i < firstPageToClear + numberOfPagesToClear; i++) {
delete cache[i];
}
});
comboBox.$connector.filter = tryCatchWrapper(function (item, filter) {
filter = filter ? filter.toString().toLowerCase() : '';
return comboBox._getItemLabel(item, comboBox.itemLabelPath).toString().toLowerCase().indexOf(filter) > -1;
});
comboBox.$connector.set = tryCatchWrapper(function (index, items, filter) {
if (filter != serverFacade.getLastFilterSentToServer()) {
return;
}
if (index % comboBox.pageSize != 0) {
throw 'Got new data to index ' + index + ' which is not aligned with the page size of ' + comboBox.pageSize;
}
if (index === 0 && items.length === 0 && pageCallbacks[0]) {
// Makes sure that the dataProvider callback is called even when server
// returns empty data set (no items match the filter).
cache[0] = [];
return;
}
const firstPageToSet = index / comboBox.pageSize;
const updatedPageCount = Math.ceil(items.length / comboBox.pageSize);
for (let i = 0; i < updatedPageCount; i++) {
let page = firstPageToSet + i;
let slice = items.slice(i * comboBox.pageSize, (i + 1) * comboBox.pageSize);
cache[page] = slice;
}
});
comboBox.$connector.updateData = tryCatchWrapper(function (items) {
const itemsMap = new Map(items.map((item) => [item.key, item]));
comboBox.filteredItems = comboBox.filteredItems.map((item) => {
return itemsMap.get(item.key) || item;
});
});
comboBox.$connector.updateSize = tryCatchWrapper(function (newSize) {
if (!comboBox._clientSideFilter) {
// FIXME: It may be that this size set is unnecessary, since when
// providing data to combobox via callback we may use data's size.
// However, if this size reflect the whole data size, including
// data not fetched yet into client side, and combobox expect it
// to be set as such, the at least, we don't need it in case the
// filter is clientSide only, since it'll increase the height of
// the popup at only at first user filter to this size, while the
// filtered items count are less.
comboBox.size = newSize;
}
});
comboBox.$connector.reset = tryCatchWrapper(function () {
clearPageCallbacks();
cache = {};
comboBox.clearCache();
});
comboBox.$connector.confirm = tryCatchWrapper(function (id, filter) {
if (filter != serverFacade.getLastFilterSentToServer()) {
return;
}
// We're done applying changes from this batch, resolve pending
// callbacks
let activePages = Object.getOwnPropertyNames(pageCallbacks);
for (let i = 0; i < activePages.length; i++) {
let page = activePages[i];
if (cache[page]) {
commitPage(page, pageCallbacks[page]);
}
}
// Let server know we're done
comboBox.$server.confirmUpdate(id);
});
const commitPage = tryCatchWrapper(function (page, callback) {
let data = cache[page];
if (comboBox._clientSideFilter) {
performClientSideFilter(data, comboBox.filter, callback);
} else {
// Remove the data if server-side filtering, but keep it for client-side
// filtering
delete cache[page];
// FIXME: It may be that we ought to provide data.length instead of
// comboBox.size and remove updateSize function.
callback(data, comboBox.size);
}
});
// Perform filter on client side (here) using the items from specified page
// and submitting the filtered items to specified callback.
// The filter used is the one from combobox, not the lastFilter stored since
// that may not reflect user's input.
const performClientSideFilter = tryCatchWrapper(function (page, filter, callback) {
let filteredItems = page;
if (filter) {
filteredItems = page.filter((item) => comboBox.$connector.filter(item, filter));
}
callback(filteredItems, filteredItems.length);
});
// Prevent setting the custom value as the 'value'-prop automatically
comboBox.addEventListener(
'custom-value-set',
tryCatchWrapper((e) => e.preventDefault())
);
})(comboBox)
};
})();
window.Vaadin.ComboBoxPlaceholder = ComboBoxPlaceholder; After that, you can do this in java: ComboBox<String> comboBox = new ComboBox<>();
comboBox.getElement().setProperty("filterDebouncerTimer", 3000);
comboBox.setItems(query -> {
List<String> list = new ArrayList<>();
if (query.getOffset() == 0) {
int limit = query.getLimit();
for (int i = 1; i <= limit; i++) {
list.add("item # " + i);
}
}
try {
Thread.sleep(5000);
} catch (InterruptedException e) {
e.printStackTrace();
}
return list.stream();
}); In this example, we will get to the query only 3000 ms after the last character entered in the text field. |
I made a separate PR for the seemingly obsolete filters string syncing. Quickly looking it might be some leftovers from some refactoring 🤔 Anyways, I think it should be considered a separate performance fix/optimisation. Although it slows down ComboBox usage, the backend pressure configurability is more essential, especially if one needs to limit them, and the PR derived from the solution by @Aleksey2093 seems to do that perfectly. Tested that in a separate project as well, but building integration tests should be about 10X more efficient for somebody that works with the project dayly (and for who the project actually builds without additional haxies 🧸). |
flow-components/vaadin-combo-box-flow-parent/vaadin-combo-box-flow/src/main/resources/META-INF/resources/frontend/comboBoxConnector.js
Lines 92 to 99 in b2fe10c
I have a very long query for DataProvider. It is necessary that the filter does not react immediately when entering and allows the user to enter more characters. 500 milliseconds is very short. You need to be able to change this parameter using "getElement().setProperty(...."
vaadin 23.3.33
The text was updated successfully, but these errors were encountered: