Skip to content
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

Open
Aleksey2093 opened this issue Nov 25, 2024 · 10 comments · May be fixed by #7133
Open

Vaadin ComboBox Filter timeout #6863

Aleksey2093 opened this issue Nov 25, 2024 · 10 comments · May be fixed by #7133
Labels
enhancement New feature or request vaadin-combo-box

Comments

@Aleksey2093
Copy link

Aleksey2093 commented Nov 25, 2024

this._filterDebouncer = Debouncer.debounce(this._filterDebouncer, timeOut.after(500), () => {
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();
}

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

@Aleksey2093 Aleksey2093 changed the title kjh Vaadin ComboBox Filter timeout Nov 25, 2024
@web-padawan
Copy link
Member

Related to #1031

@Aleksey2093
Copy link
Author

Aleksey2093 commented Nov 25, 2024

to connector.js

            let timerValue = 1000
            if (comboBox.filterDebouncerTimer) {
              timerValue = comboBox.filterDebouncerTimer
            }
            this._filterDebouncer = Debouncer.debounce(this._filterDebouncer, timeOut.after(timerValue), () => {

to java

combobox.getElement().setProperty("filterDebouncerTimer", "5000")

Can it be done like this?

@mstahv
Copy link
Member

mstahv commented Feb 18, 2025

Did you @Aleksey2093 try that solution?

@Aleksey2093
Copy link
Author

Did you @Aleksey2093 try that solution?

Yes. That #6863 (comment) work

@mstahv
Copy link
Member

mstahv commented Feb 20, 2025

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.

mstahv added a commit to mstahv/flow-components that referenced this issue Feb 20, 2025
PoC created based on suggestion by @Aleksey2093

Number of different issues related to topic, like vaadin#6863
@mstahv mstahv linked a pull request Feb 20, 2025 that will close this issue
10 tasks
@mstahv
Copy link
Member

mstahv commented Feb 20, 2025

I couldn't resist drafting this as PR. The problem was that the project don't compile for me (related to some other component) 🤷‍♂️


[INFO] Scanning for projects...
[ERROR] Some problems were encountered while processing the POMs
[ERROR] The build could not read 1 project -> [Help 1]
[ERROR]   
[ERROR]   The project [inherited]:vaadin-flow-components:pom:24.7-SNAPSHOT (/Users/mstahv/git/flow-components/pom.xml) has 2 errors
[ERROR]     'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: org.slf4j:slf4j-simple:jar -> version 2.0.16 vs (?) @ com.vaadin:vaadin-tabs-flow:24.7-SNAPSHOT, file:///Users/mstahv/git/flow-components/vaadin-tabs-flow-parent/vaadin-tabs-flow/pom.xml, line 54, column 9
[ERROR]     'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: org.slf4j:slf4j-simple:jar -> version 2.0.16 vs (?) @ com.vaadin:vaadin-tabs-flow:24.7-SNAPSHOT, file:///Users/mstahv/git/flow-components/vaadin-tabs-flow-parent/vaadin-tabs-flow/pom.xml, line 54, column 9
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the '-e' switch
[ERROR] Re-run Maven using the '-X' switch to enable verbose output
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/ProjectBuildingException

@mstahv
Copy link
Member

mstahv commented Feb 20, 2025

FYI: with this change the project compiles "enough" for ComboBox module. Still some issue with grid module.

mstahv@ed8c017

@mstahv
Copy link
Member

mstahv commented Feb 20, 2025

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 🤓

@Aleksey2093
Copy link
Author

Did you @Aleksey2093 try that solution?

Yes. That #6863 (comment) work

The corrected code:
src/main/resources/META-INF/resources/frontend/comboBoxConnector.js

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.

@mstahv
Copy link
Member

mstahv commented Feb 21, 2025

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 🧸).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request vaadin-combo-box
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants