diff options
author | Pekka Hyvönen <pekka@vaadin.com> | 2017-02-16 10:14:38 +0200 |
---|---|---|
committer | Henri Sara <henri.sara@gmail.com> | 2017-02-16 10:14:38 +0200 |
commit | 1891ffa6e1c4db8bc0d0f60eca255895a6448bdd (patch) | |
tree | e93a79a4be27fdecaadab7effc002d2f5492f9f2 | |
parent | 20947e0e0893c955ec6e42579a29a913c3c74883 (diff) | |
download | vaadin-framework-1891ffa6e1c4db8bc0d0f60eca255895a6448bdd.tar.gz vaadin-framework-1891ffa6e1c4db8bc0d0f60eca255895a6448bdd.zip |
Reduce ComboBox initial requests (#8571)
* Reduce ComboBox initial requests
Use initial fetched data on client side,
do not request data from server side for each time popup is opened.
Fixed initial filter being null for ComboBox on DataProvider,
causing unnecessary size & fetch for not-changed filter.
Fixed ComboBox sending default filter unnecessarily to server.
Fixed wrong page indexing in VComboBox -> ComboBoxConnector.
Fixes #8496
Fixes vaadin/framework8-issues#488
* Fix last item missing
When pageLength was 0 and nullSelectionAllowed,
the last item was not shown. Tried to sensify the API
for total suggestions versus total suggestions + null selection item.
* Fix ComboBox selected item updates
Handles changing of ItemCaptionGenerator or ItemIconGenerator,
need to update the selected item caption and icon separately.
Previously it worked because all data was sent all the time to client.
Doesn't fix the issue, when selected item is updated with refreshItem(),
and it is not on the active range that will be sent to client.
For that, ComboBox would need a separate notification about item update.
* Updated screenshots
9 files changed, 778 insertions, 109 deletions
diff --git a/client/src/main/java/com/vaadin/client/ui/VComboBox.java b/client/src/main/java/com/vaadin/client/ui/VComboBox.java index 6ecea2d116..77e1108417 100644 --- a/client/src/main/java/com/vaadin/client/ui/VComboBox.java +++ b/client/src/main/java/com/vaadin/client/ui/VComboBox.java @@ -208,12 +208,12 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, return false; } ComboBoxSuggestion other = (ComboBoxSuggestion) obj; - if ((key == null && other.key != null) - || (key != null && !key.equals(other.key))) { + if (key == null && other.key != null + || key != null && !key.equals(other.key)) { return false; } - if ((caption == null && other.caption != null) - || (caption != null && !caption.equals(other.caption))) { + if (caption == null && other.caption != null + || caption != null && !caption.equals(other.caption)) { return false; } if (!SharedUtil.equals(untranslatedIconUri, @@ -241,12 +241,12 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, return $entry(function(e) { var deltaX = e.deltaX ? e.deltaX : -0.5*e.wheelDeltaX; var deltaY = e.deltaY ? e.deltaY : -0.5*e.wheelDeltaY; - + // IE8 has only delta y if (isNaN(deltaY)) { deltaY = -0.5*e.wheelDelta; } - + @com.vaadin.client.ui.VComboBox.JsniUtil::moveScrollFromEvent(*)(widget, deltaX, deltaY, e, e.deltaMode); }); }-*/; @@ -398,14 +398,11 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, * * @param currentPage * The current page number - * @param totalSuggestions - * The total amount of suggestions */ - public void showSuggestions(final int currentPage, - final int totalSuggestions) { + public void showSuggestions(final int currentPage) { debug("VComboBox.SP: showSuggestions(" + currentPage + ", " - + totalSuggestions + ")"); + + getTotalSuggestions() + ")"); final SuggestionPopup popup = this; // Add TT anchor point @@ -418,14 +415,13 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, setPopupPosition(x, topPosition); - int nullOffset = (nullSelectionAllowed && "".equals(lastFilter) ? 1 - : 0); - boolean firstPage = (currentPage == 0); + int nullOffset = getNullSelectionItemShouldBeVisible() ? 1 : 0; + boolean firstPage = currentPage == 0; final int first = currentPage * pageLength + 1 - (firstPage ? 0 : nullOffset); final int last = first + currentSuggestions.size() - 1 - (firstPage && "".equals(lastFilter) ? nullOffset : 0); - final int matches = totalSuggestions - nullOffset; + final int matches = getTotalSuggestions(); if (last > 0) { // nullsel not counted, as requested by user status.setInnerText((matches == 0 ? 0 : first) + "-" + last @@ -435,7 +431,8 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, } // We don't need to show arrows or statusbar if there is // only one page - if (totalSuggestions <= pageLength || pageLength == 0) { + if (getTotalSuggestionsIncludingNullSelectionItem() <= pageLength + || pageLength == 0) { setPagingEnabled(false); } else { setPagingEnabled(true); @@ -609,8 +606,8 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, public void scrollDown() { debug("VComboBox.SP.LPS: scrollDown()"); if (pageLength > 0 - && totalMatches > (currentPage + pagesToScroll + 1) - * pageLength) { + && getTotalSuggestionsIncludingNullSelectionItem() > (currentPage + + pagesToScroll + 1) * pageLength) { pagesToScroll++; cancel(); schedule(200); @@ -860,7 +857,7 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, if (suggestionPopupWidth == null) { if (naturalMenuWidth < desiredWidth) { naturalMenuWidth = desiredWidth - popupOuterPadding; - width = (desiredWidth - popupOuterPadding) + "px"; + width = desiredWidth - popupOuterPadding + "px"; } } else if (isrelativeUnits(suggestionPopupWidth)) { float mainComponentWidth = desiredWidth - popupOuterPadding; @@ -904,8 +901,8 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, */ public boolean isJustClosed() { debug("VComboBox.SP: justClosed()"); - final long now = (new Date()).getTime(); - return (lastAutoClosed > 0 && (now - lastAutoClosed) < 200); + final long now = new Date().getTime(); + return lastAutoClosed > 0 && now - lastAutoClosed < 200; } /* @@ -922,7 +919,7 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, debug("VComboBox.SP: onClose(" + event.isAutoClosed() + ")"); } if (event.isAutoClosed()) { - lastAutoClosed = (new Date()).getTime(); + lastAutoClosed = new Date().getTime(); } } @@ -999,8 +996,8 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, */ String getPreferredHeight(int pageItemsCount) { if (currentSuggestions.size() > 0) { - final int pixels = (getPreferredHeight() - / currentSuggestions.size()) * pageItemsCount; + final int pixels = getPreferredHeight() + / currentSuggestions.size() * pageItemsCount; return pixels + "px"; } else { return ""; @@ -1022,9 +1019,10 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, final Iterator<ComboBoxSuggestion> it = suggestions.iterator(); boolean isFirstIteration = true; while (it.hasNext()) { - final ComboBoxSuggestion s = it.next(); - final MenuItem mi = new MenuItem(s.getDisplayString(), true, s); - String style = s.getStyle(); + final ComboBoxSuggestion suggestion = it.next(); + final MenuItem mi = new MenuItem(suggestion.getDisplayString(), + true, suggestion); + String style = suggestion.getStyle(); if (style != null) { mi.addStyleName("v-filterselect-item-" + style); } @@ -1040,20 +1038,23 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, selectItem(mi); } - if (currentSuggestion != null && s.getOptionKey() + if (currentSuggestion != null && suggestion.getOptionKey() .equals(currentSuggestion.getOptionKey())) { - // refresh also selected caption and icon in case they have - // been updated on the server + // Refresh also selected caption and icon in case they have + // been updated on the server, e.g. just the item has been + // updated, but selection (from state) has stayed the same. + // FIXME need to update selected item caption separately, if + // the selected item is not in "active data range" that is + // being sent to the client. Then this can be removed. if (currentSuggestion.getReplacementString() .equals(tb.getText())) { - currentSuggestion = s; + currentSuggestion = suggestion; selectItem(mi); setSelectedCaption( currentSuggestion.getReplacementString()); setSelectedItemIcon(currentSuggestion.getIconUri()); } } - isFirstIteration = false; } } @@ -1080,8 +1081,8 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, // do not send a value change event if null was and // stays selected if (!"".equals(enteredItemValue) - || (selectedOptionKey != null - && !"".equals(selectedOptionKey))) { + || selectedOptionKey != null + && !"".equals(selectedOptionKey)) { doItemAction(potentialExactMatch, true); } suggestionPopup.hide(); @@ -1099,8 +1100,8 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, // TODO try to select the new value if it matches what was // sent for V7 compatibility } - } else if (item != null && !"".equals(lastFilter) && (item.getText() - .toLowerCase().contains(lastFilter.toLowerCase()))) { + } else if (item != null && !"".equals(lastFilter) && item.getText() + .toLowerCase().contains(lastFilter.toLowerCase())) { doItemAction(item, true); } else { // currentSuggestion has key="" for nullselection @@ -1367,7 +1368,7 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, showPopup = true; } if (showPopup) { - suggestionPopup.showSuggestions(currentPage, totalMatches); + suggestionPopup.showSuggestions(currentPage); } waitingForFilteringResponse = false; @@ -1523,9 +1524,12 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, * new selected item caption if sent by the server or null - * this is used when the selected item is not on the current * page + * @param selectedIconUri + * new selected item icon if sent by the server or {@ code + * null} to clear */ public void updateSelectionFromServer(String selectedKey, - String selectedCaption) { + String selectedCaption, String selectedIconUri) { boolean oldSuggestionTextMatchTheOldSelection = currentSuggestion != null && currentSuggestion.getReplacementString() .equals(tb.getText()); @@ -1538,10 +1542,8 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, cancelPendingPostFiltering(); setSelectedCaption(selectedCaption); - if (currentSuggestion != null && serverSelectedKey - .equals(currentSuggestion.getOptionKey())) { - setSelectedItemIcon(currentSuggestion.getIconUri()); - } + + setSelectedItemIcon(selectedIconUri); } } @@ -1617,8 +1619,8 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, /** For internal use only. May be removed or replaced in the future. */ public boolean allowNewItems; - /** For internal use only. May be removed or replaced in the future. */ - public int totalMatches; + /** Total number of suggestions, excluding null selection item. */ + private int totalSuggestions; /** For internal use only. May be removed or replaced in the future. */ public boolean nullSelectionAllowed; @@ -1765,8 +1767,9 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, * last page */ public boolean hasNextPage() { - return (pageLength > 0 - && totalMatches > (currentPage + 1) * pageLength); + return pageLength > 0 + && getTotalSuggestionsIncludingNullSelectionItem() > (currentPage + + 1) * pageLength; } /** @@ -1793,16 +1796,13 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, public void filterOptions(int page, String filter) { debug("VComboBox: filterOptions(" + page + ", " + filter + ")"); - if (filter.equals(lastFilter) && currentPage == page) { - if (!suggestionPopup.isAttached()) { - dataReceivedHandler.startWaitingForFilteringResponse(); - connector.requestPage(currentPage); - } else { - // already have the page - dataReceivedHandler.dataReceived(); - } + if (filter.equals(lastFilter) && currentPage == page + && suggestionPopup.isAttached()) { + // already have the page + dataReceivedHandler.dataReceived(); return; } + if (!filter.equals(lastFilter)) { // when filtering, let the server decide the page unless we've // set the filter to empty and explicitly said that we want to see @@ -1816,11 +1816,15 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, } dataReceivedHandler.startWaitingForFilteringResponse(); - connector.setFilter(filter); - connector.requestPage(currentPage); + connector.requestPage(page, filter); lastFilter = filter; - currentPage = page; + + // If the data was updated from cache, the page has been updated too, if + // not, update + if (dataReceivedHandler.isWaitingForFilteringResponse()) { + currentPage = page; + } } /** For internal use only. May be removed or replaced in the future. */ @@ -1914,7 +1918,7 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, setText(text); setSelectedItemIcon(suggestion.getIconUri()); - if (!(newKey.equals(selectedOptionKey))) { + if (!newKey.equals(selectedOptionKey)) { selectedOptionKey = newKey; connector.sendSelection(selectedOptionKey); setSelectedCaption(text); @@ -1932,23 +1936,19 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, if (enableDebug) { debug("VComboBox: onNullSelected()"); } - // TODO do we need this feature? - if (nullSelectItem) { - reset(); - return; - } dataReceivedHandler.cancelPendingPostFiltering(); currentSuggestion = null; setText(""); setSelectedItemIcon(null); - if (!"".equals(selectedOptionKey) || (selectedOptionKey != null)) { + if (!"".equals(selectedOptionKey) || selectedOptionKey != null) { selectedOptionKey = ""; setSelectedCaption(""); connector.sendSelection(null); // currentPage = 0; } + updatePlaceholder(); suggestionPopup.hide(); } @@ -1958,7 +1958,7 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, * side of the item caption text. Set the URI to null to remove the icon. * * @param iconUri - * The URI of the icon + * The URI of the icon, or null to remove icon */ public void setSelectedItemIcon(String iconUri) { @@ -2017,7 +2017,6 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, boolean updatePromptAndSelectionIfMatchFound) { if (selectedKey == null || "".equals(selectedKey)) { currentSuggestion = null; // #13217 - setSelectedItemIcon(null); selectedOptionKey = null; setText(""); } @@ -2042,7 +2041,6 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, } } currentSuggestion = suggestion; - setSelectedItemIcon(suggestion.getIconUri()); // only a single item can be selected break; } @@ -2294,19 +2292,21 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, */ private void reset() { debug("VComboBox: reset()"); - if (currentSuggestion != null) { - String text = currentSuggestion.getReplacementString(); - setText(text); - setSelectedItemIcon(currentSuggestion.getIconUri()); - - selectedOptionKey = currentSuggestion.key; - - } else { - setText(""); - setSelectedItemIcon(null); - updatePlaceholder(); + // just fetch selected information from state + String text = connector.getState().selectedItemCaption; + setText(text == null ? "" : text); + setSelectedItemIcon(connector.getState().selectedItemIcon); + selectedOptionKey = (connector.getState().selectedItemKey); + if (selectedOptionKey == null || "".equals(selectedOptionKey)) { + currentSuggestion = null; // #13217 selectedOptionKey = null; + updatePlaceholder(); + } else { + currentSuggestion = currentSuggestions.stream() + .filter(suggestion -> suggestion.getOptionKey() + .equals(selectedOptionKey)) + .findAny().orElse(null); } lastFilter = ""; @@ -2551,7 +2551,7 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, * popupopener */ - tb.setWidth((suggestionPopupMinWidth - iconWidth - buttonWidth) + tb.setWidth(suggestionPopupMinWidth - iconWidth - buttonWidth + "px"); } @@ -2563,7 +2563,7 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, if (!tb.getElement().getStyle().getWidth().endsWith("px")) { int iconWidth = selectedItemIcon == null ? 0 : selectedItemIcon.getOffsetWidth(); - tb.setWidth((tb.getOffsetWidth() - iconWidth) + "px"); + tb.setWidth(tb.getOffsetWidth() - iconWidth + "px"); } } } @@ -2763,4 +2763,52 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, this.allowNewItems = allowNewItems; } + /** + * Sets the total number of suggestions. + * <p> + * NOTE: this excluded the possible null selection item! + * <p> + * NOTE: this just updates the state, but doesn't update any UI. + * + * @since 8.0 + * @param totalSuggestions + * total number of suggestions + */ + public void setTotalSuggestions(int totalSuggestions) { + this.totalSuggestions = totalSuggestions; + } + + /** + * Gets the total number of suggestions, excluding the null selection item. + * + * @since 8.0 + * @return total number of suggestions + */ + public int getTotalSuggestions() { + return totalSuggestions; + } + + /** + * Gets the total number of suggestions, including the possible null + * selection item, if it should be visible. + * + * @return total number of suggestions with null selection items + */ + private int getTotalSuggestionsIncludingNullSelectionItem() { + return getTotalSuggestions() + + (getNullSelectionItemShouldBeVisible() ? 1 : 0); + } + + /** + * Returns null selection item should be visible or not. + * <p> + * NOTE: this checks for any entered filter value, and whether the feature + * is enabled + * + * @since 8.0 + * @return {@code true} if it should be visible, {@code} + */ + public boolean getNullSelectionItemShouldBeVisible() { + return nullSelectionAllowed && "".equals(lastFilter); + } } diff --git a/client/src/main/java/com/vaadin/client/ui/combobox/ComboBoxConnector.java b/client/src/main/java/com/vaadin/client/ui/combobox/ComboBoxConnector.java index a4a9c17f4d..7fec2ae8e0 100644 --- a/client/src/main/java/com/vaadin/client/ui/combobox/ComboBoxConnector.java +++ b/client/src/main/java/com/vaadin/client/ui/combobox/ComboBoxConnector.java @@ -16,12 +16,15 @@ package com.vaadin.client.ui.combobox; import java.util.List; +import java.util.Objects; +import java.util.logging.Logger; import com.vaadin.client.Profiler; import com.vaadin.client.annotations.OnStateChange; import com.vaadin.client.communication.StateChangeEvent; import com.vaadin.client.connectors.AbstractListingConnector; import com.vaadin.client.connectors.data.HasDataSource; +import com.vaadin.client.data.DataChangeHandler; import com.vaadin.client.data.DataSource; import com.vaadin.client.ui.HasErrorIndicator; import com.vaadin.client.ui.HasRequiredIndicator; @@ -76,6 +79,8 @@ public class ComboBoxConnector extends AbstractListingConnector getWidget().suggestionPopup.updateStyleNames(getState()); + // TODO if the pop up is opened, the actual item should be removed from + // the popup (?) getWidget().nullSelectionAllowed = getState().emptySelectionAllowed; // TODO having this true would mean that the empty selection item comes // from the data source so none needs to be added - currently @@ -87,6 +92,9 @@ public class ComboBoxConnector extends AbstractListingConnector getDataReceivedHandler().serverReplyHandled(); + // all updates except options have been done + getWidget().initDone = true; + Profiler.leave("ComboBoxConnector.onStateChanged update content"); } @@ -99,10 +107,11 @@ public class ComboBoxConnector extends AbstractListingConnector } } - @OnStateChange({ "selectedItemKey", "selectedItemCaption" }) + @OnStateChange({ "selectedItemKey", "selectedItemCaption", "selectedItemIcon" }) private void onSelectionChange() { getDataReceivedHandler().updateSelectionFromServer( - getState().selectedItemKey, getState().selectedItemCaption); + getState().selectedItemKey, getState().selectedItemCaption, + getState().selectedItemIcon); } @Override @@ -164,11 +173,12 @@ public class ComboBoxConnector extends AbstractListingConnector * @param filter * the current filter string */ - public void setFilter(String filter) { - if (filter != getWidget().lastFilter) { + protected void setFilter(String filter) { + if (!Objects.equals(filter, getWidget().lastFilter)) { getDataReceivedHandler().clearPendingNavigation(); + + rpc.setFilter(filter); } - rpc.setFilter(filter); } /** @@ -183,22 +193,27 @@ public class ComboBoxConnector extends AbstractListingConnector * the page number to get or -1 to let the server/connector * decide based on current selection (possibly loading more data * from the server) + * @param filter + * the filter to apply, never {@code null} */ - public void requestPage(int page) { + public void requestPage(int page, String filter) { + setFilter(filter); + if (page < 0) { if (getState().scrollToSelectedItem) { - getDataSource().ensureAvailability(0, 10000); + // TODO this should be optimized not to try to fetch everything + getDataSource().ensureAvailability(0, getDataSource().size()); return; } else { page = 0; } } - int adjustment = getWidget().nullSelectionAllowed - && "".equals(getWidget().lastFilter) ? 1 : 0; + int adjustment = getWidget().nullSelectionAllowed && "".equals(filter) + ? 1 : 0; int startIndex = Math.max(0, page * getWidget().pageLength - adjustment); int pageLength = getWidget().pageLength > 0 ? getWidget().pageLength - : 10000; + : getDataSource().size(); getDataSource().ensureAvailability(startIndex, pageLength); } @@ -261,7 +276,7 @@ public class ComboBoxConnector extends AbstractListingConnector public void setDataSource(DataSource<JsonObject> dataSource) { super.setDataSource(dataSource); dataChangeHandlerRegistration = dataSource - .addDataChangeHandler(range -> refreshData()); + .addDataChangeHandler(new PagedDataChangeHandler(dataSource)); } @Override @@ -278,14 +293,13 @@ public class ComboBoxConnector extends AbstractListingConnector private void refreshData() { updateCurrentPage(); - getWidget().currentSuggestions.clear(); - int start = getWidget().currentPage * getWidget().pageLength; int end = getWidget().pageLength > 0 ? start + getWidget().pageLength : getDataSource().size(); - if (getWidget().nullSelectionAllowed - && "".equals(getWidget().lastFilter)) { + getWidget().currentSuggestions.clear(); + + if (getWidget().getNullSelectionItemShouldBeVisible()) { // add special null selection item... if (isFirstPage()) { addEmptySelectionItem(); @@ -294,13 +308,14 @@ public class ComboBoxConnector extends AbstractListingConnector start = start - 1; } // in either case, the last item to show is - // shifted by one - end = end - 1; + // shifted by one, unless no paging is used + if (getState().pageLength != 0) { + end = end - 1; + } } updateSuggestions(start, end); - getWidget().totalMatches = getDataSource().size() - + (getState().emptySelectionAllowed ? 1 : 0); + getWidget().setTotalSuggestions(getDataSource().size()); getDataReceivedHandler().dataReceived(); } @@ -308,16 +323,18 @@ public class ComboBoxConnector extends AbstractListingConnector private void updateSuggestions(int start, int end) { for (int i = start; i < end; ++i) { JsonObject row = getDataSource().getRow(i); - if (row != null) { String key = getRowKey(row); String caption = row.getString(DataCommunicatorConstants.NAME); String style = row.getString(ComboBoxConstants.STYLE); String untranslatedIconUri = row .getString(ComboBoxConstants.ICON); - getWidget().currentSuggestions - .add(getWidget().new ComboBoxSuggestion(key, caption, - style, untranslatedIconUri)); + ComboBoxSuggestion suggestion = getWidget().new ComboBoxSuggestion( + key, caption, style, untranslatedIconUri); + getWidget().currentSuggestions.add(suggestion); + } else { + // there is not enough options to fill the page + return; } } } @@ -360,4 +377,54 @@ public class ComboBoxConnector extends AbstractListingConnector getWidget().currentPage = 0; } } + + private static final Logger LOGGER = Logger + .getLogger(ComboBoxConnector.class.getName()); + + private class PagedDataChangeHandler implements DataChangeHandler { + + private final DataSource<?> dataSource; + + public PagedDataChangeHandler(DataSource<?> dataSource) { + this.dataSource = dataSource; + } + + @Override + public void dataUpdated(int firstRowIndex, int numberOfRows) { + // NOOP since dataAvailable is always triggered afterwards + } + + @Override + public void dataRemoved(int firstRowIndex, int numberOfRows) { + // NOOP since dataAvailable is always triggered afterwards + } + + @Override + public void dataAdded(int firstRowIndex, int numberOfRows) { + // NOOP since dataAvailable is always triggered afterwards + } + + @Override + public void dataAvailable(int firstRowIndex, int numberOfRows) { + refreshData(); + } + + @Override + public void resetDataAndSize(int estimatedNewDataSize) { + if (getState().pageLength == 0) { + if (getWidget().suggestionPopup.isShowing()) { + dataSource.ensureAvailability(0, estimatedNewDataSize); + } + // else lets just wait till the popup is opened before + // everything is fetched to it. this could be optimized later on + // to fetch everything if in-memory data is used. + } else { + // reset data: clear any current options, set page to 0 + getWidget().currentPage = 0; + getWidget().currentSuggestions.clear(); + dataSource.ensureAvailability(0, getState().pageLength); + } + } + + } } diff --git a/server/src/main/java/com/vaadin/ui/ComboBox.java b/server/src/main/java/com/vaadin/ui/ComboBox.java index a2db94a8c6..ed1e9d8beb 100644 --- a/server/src/main/java/com/vaadin/ui/ComboBox.java +++ b/server/src/main/java/com/vaadin/ui/ComboBox.java @@ -575,6 +575,9 @@ public class ComboBox<T> extends AbstractSingleSelect<T> public void setItemCaptionGenerator( ItemCaptionGenerator<T> itemCaptionGenerator) { super.setItemCaptionGenerator(itemCaptionGenerator); + if (getSelectedItem().isPresent()) { + updateSelectedItemCaption(); + } } /** @@ -614,6 +617,10 @@ public class ComboBox<T> extends AbstractSingleSelect<T> @Override public void setItemIconGenerator(IconGenerator<T> itemIconGenerator) { super.setItemIconGenerator(itemIconGenerator); + + if (getSelectedItem().isPresent()) { + updateSelectedItemIcon(); + } } @Override @@ -631,7 +638,7 @@ public class ComboBox<T> extends AbstractSingleSelect<T> */ public void setNewItemHandler(NewItemHandler newItemHandler) { this.newItemHandler = newItemHandler; - getState().allowNewItems = (newItemHandler != null); + getState().allowNewItems = newItemHandler != null; markAsDirty(); } @@ -670,14 +677,32 @@ public class ComboBox<T> extends AbstractSingleSelect<T> protected void doSetSelectedKey(String key) { super.doSetSelectedKey(key); + updateSelectedItemCaption(); + updateSelectedItemIcon(); + } + + private void updateSelectedItemCaption() { String selectedCaption = null; - T value = getDataCommunicator().getKeyMapper().get(key); + T value = getDataCommunicator().getKeyMapper().get(getSelectedKey()); if (value != null) { selectedCaption = getItemCaptionGenerator().apply(value); } getState().selectedItemCaption = selectedCaption; } + private void updateSelectedItemIcon() { + String selectedItemIcon = null; + T value = getDataCommunicator().getKeyMapper().get(getSelectedKey()); + if (value != null) { + Resource icon = getItemIconGenerator().apply(value); + if (icon != null) { + selectedItemIcon = ResourceReference + .create(icon, ComboBox.this, null).getURL(); + } + } + getState().selectedItemIcon = selectedItemIcon; + } + @Override protected Element writeItem(Element design, T item, DesignContext context) { Element element = design.appendElement("option"); @@ -748,7 +773,7 @@ public class ComboBox<T> extends AbstractSingleSelect<T> "filterConverter cannot be null"); SerializableFunction<String, C> convertOrNull = filterText -> { - if (filterText == null) { + if (filterText == null || filterText.isEmpty()) { return null; } @@ -814,10 +839,12 @@ public class ComboBox<T> extends AbstractSingleSelect<T> extends SerializableBiPredicate<String, String> { /** - * Check item caption against entered text + * Check item caption against entered text. * * @param itemCaption + * the caption of the item to filter, not {@code null} * @param filterText + * user entered filter, not {@code null} * @return {@code true} if item passes the filter and should be listed, * {@code false} otherwise */ diff --git a/shared/src/main/java/com/vaadin/shared/ui/combobox/ComboBoxState.java b/shared/src/main/java/com/vaadin/shared/ui/combobox/ComboBoxState.java index a7be4865a7..666fff1e58 100644 --- a/shared/src/main/java/com/vaadin/shared/ui/combobox/ComboBoxState.java +++ b/shared/src/main/java/com/vaadin/shared/ui/combobox/ComboBoxState.java @@ -93,4 +93,11 @@ public class ComboBoxState extends AbstractSingleSelectState { */ public String emptySelectionCaption = ""; + /** + * Selected item icon uri. + * + * @since 8.0 + */ + public String selectedItemIcon; + } diff --git a/testbench-api/src/main/java/com/vaadin/testbench/elements/ComboBoxElement.java b/testbench-api/src/main/java/com/vaadin/testbench/elements/ComboBoxElement.java index 64023c7d69..e0e4799c15 100644 --- a/testbench-api/src/main/java/com/vaadin/testbench/elements/ComboBoxElement.java +++ b/testbench-api/src/main/java/com/vaadin/testbench/elements/ComboBoxElement.java @@ -109,6 +109,15 @@ public class ComboBoxElement extends AbstractSelectElement { return !isReadOnly(getInputField()); } + /** + * Checks whether the suggestion popup is open or not. + * + * @return {@code true} if popup is open, {@code false if not} + */ + public boolean isPopupOpen() { + return isElementPresent(bySuggestionPopup); + } + /* * Workaround selenium's bug: sendKeys() will not send left parentheses * properly. See #14048. @@ -147,7 +156,7 @@ public class ComboBoxElement extends AbstractSelectElement { * @return List of suggestion texts */ public List<String> getPopupSuggestions() { - List<String> suggestionsTexts = new ArrayList<String>(); + List<String> suggestionsTexts = new ArrayList<>(); List<WebElement> suggestions = getPopupSuggestionElements(); for (WebElement suggestion : suggestions) { String text = suggestion.getText(); @@ -280,7 +289,7 @@ public class ComboBoxElement extends AbstractSelectElement { private void clickElement(WebElement element) { if (isFirefox()) { // Workaround for Selenium/TB and Firefox 45 issue - ((TestBenchElement) (element)).clickHiddenElement(); + ((TestBenchElement) element).clickHiddenElement(); } else { element.click(); } diff --git a/tests/screenshots b/tests/screenshots -Subproject c233d26d0af136183eef3707461e2df0d026ff4 +Subproject 9a77b414d2eed1d74c559916d81df9b92ce680e diff --git a/uitest/src/main/java/com/vaadin/tests/components/combobox/ComboBoxBackEndRequests.java b/uitest/src/main/java/com/vaadin/tests/components/combobox/ComboBoxBackEndRequests.java new file mode 100644 index 0000000000..be2ce6c67e --- /dev/null +++ b/uitest/src/main/java/com/vaadin/tests/components/combobox/ComboBoxBackEndRequests.java @@ -0,0 +1,72 @@ +package com.vaadin.tests.components.combobox; + +import com.vaadin.server.VaadinRequest; +import com.vaadin.shared.ui.combobox.ComboBoxState; +import com.vaadin.tests.components.AbstractTestUI; +import com.vaadin.tests.util.LoggingItemDataProvider; +import com.vaadin.ui.Button; +import com.vaadin.ui.CheckBox; +import com.vaadin.ui.ComboBox; +import com.vaadin.ui.HorizontalLayout; +import com.vaadin.ui.Panel; +import com.vaadin.ui.VerticalLayout; + +public class ComboBoxBackEndRequests extends AbstractTestUI { + + public static final String PAGE_LENGTH_REQUEST_PARAMETER = "pageLength"; + public static final String ITEMS_REQUEST_PARAMETER = "items"; + public static final int DEFAULT_NUMBER_OF_ITEMS = 200; + public static final int DEFAULT_PAGE_LENGTH = new ComboBoxState().pageLength; + + @Override + protected void setup(VaadinRequest request) { + int pageLength = DEFAULT_PAGE_LENGTH; + int items = DEFAULT_NUMBER_OF_ITEMS; + if (request.getParameter(PAGE_LENGTH_REQUEST_PARAMETER) != null) { + pageLength = Integer.parseInt(request + .getParameter(PAGE_LENGTH_REQUEST_PARAMETER).toString()); + } + if (request.getParameter(ITEMS_REQUEST_PARAMETER) != null) { + items = Integer.parseInt( + request.getParameter(ITEMS_REQUEST_PARAMETER).toString()); + } + + ComboBox<String> cb = new ComboBox<>(); + cb.setPageLength(pageLength); + VerticalLayout logContainer = new VerticalLayout(); + logContainer.setSpacing(false); + + CheckBox textInputAllowed = new CheckBox("textInputAllowed", + cb.isTextInputAllowed()); + textInputAllowed.addValueChangeListener( + event -> cb.setTextInputAllowed(textInputAllowed.getValue())); + + CheckBox emptySelectionAllowed = new CheckBox("emptySelectionAllowed", + cb.isEmptySelectionAllowed()); + emptySelectionAllowed.addValueChangeListener(event -> cb + .setEmptySelectionAllowed(emptySelectionAllowed.getValue())); + + CheckBox scrollToSelectedItem = new CheckBox("scrollToSelectedItem", + cb.isScrollToSelectedItem()); + scrollToSelectedItem.addValueChangeListener(event -> cb + .setScrollToSelectedItem(scrollToSelectedItem.getValue())); + + VerticalLayout options = new VerticalLayout(textInputAllowed, + emptySelectionAllowed, scrollToSelectedItem, + new Button("Swap DataProvider", + event -> cb.setDataProvider(new LoggingItemDataProvider( + 500, logContainer))), + new Button("Clear logs", + event -> logContainer.removeAllComponents())); + + cb.setDataProvider(new LoggingItemDataProvider(items, logContainer)); + Panel panel = new Panel(logContainer); + addComponent(new HorizontalLayout(cb, panel, options)); + } + + @Override + protected Integer getTicketNumber() { + return 8496; + } + +} diff --git a/uitest/src/main/java/com/vaadin/tests/util/LoggingItemDataProvider.java b/uitest/src/main/java/com/vaadin/tests/util/LoggingItemDataProvider.java new file mode 100644 index 0000000000..0cc12c1ed6 --- /dev/null +++ b/uitest/src/main/java/com/vaadin/tests/util/LoggingItemDataProvider.java @@ -0,0 +1,71 @@ +package com.vaadin.tests.util; + +import java.util.Locale; +import java.util.stream.IntStream; +import java.util.stream.Stream; + +import com.vaadin.data.provider.CallbackDataProvider; +import com.vaadin.data.provider.Query; +import com.vaadin.ui.Label; +import com.vaadin.ui.VerticalLayout; + +/** + * CallbackDataProvider that logs info in UI on Queries it receives. + */ +public class LoggingItemDataProvider + extends CallbackDataProvider<String, String> { + + private int counter; + + public LoggingItemDataProvider(int size, VerticalLayout logContainer) { + super(q -> fetch(logContainer, q, size), + q -> size(logContainer, q, size)); + } + + private static Stream<String> fetch(VerticalLayout logContainer, + Query<String, String> query, int size) { + log("FETCH", query, logContainer); + return itemStream(query, size).skip(query.getOffset()) + .limit(query.getLimit()); + } + + private static int size(VerticalLayout logContainer, + Query<String, String> query, int size) { + log("SIZE", query, logContainer); + return size(query, size); + } + + private static Stream<String> itemStream(Query<String, String> q, + int size) { + Stream<String> stream = IntStream.range(0, size) + .mapToObj(i -> "Item " + i); + String filterText = q.getFilter().orElse("").toLowerCase(Locale.US); + + if (filterText.isEmpty()) { + return stream; + } + + return stream.filter( + text -> text.toLowerCase(Locale.US).contains(filterText)); + } + + private static int size(Query<String, String> q, int size) { + if (!q.getFilter().orElse("").isEmpty()) { + return (int) itemStream(q, size).count(); + } + return size; + } + + private static Label log(String queryType, Query<String, String> query, + VerticalLayout logContainer) { + int componentCount = logContainer.getComponentCount(); + Label label = new Label(componentCount + ": " + queryType + " " + + query.getOffset() + " " + query.getLimit() + " " + + query.getFilter().orElse("")); + logContainer.addComponentAsFirst(label); + label.setId(queryType + "-" + componentCount); + label.setStyleName("log"); + return label; + } + +} diff --git a/uitest/src/test/java/com/vaadin/tests/components/combobox/ComboBoxBackEndRequestsTest.java b/uitest/src/test/java/com/vaadin/tests/components/combobox/ComboBoxBackEndRequestsTest.java new file mode 100644 index 0000000000..73447a09c4 --- /dev/null +++ b/uitest/src/test/java/com/vaadin/tests/components/combobox/ComboBoxBackEndRequestsTest.java @@ -0,0 +1,368 @@ +package com.vaadin.tests.components.combobox; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.math.BigDecimal; +import java.math.RoundingMode; +import java.util.List; + +import org.junit.Before; +import org.junit.Ignore; +import org.junit.Test; +import org.openqa.selenium.By; +import org.openqa.selenium.WebElement; + +import com.vaadin.testbench.elements.ButtonElement; +import com.vaadin.testbench.elements.CheckBoxElement; +import com.vaadin.testbench.elements.ComboBoxElement; +import com.vaadin.tests.tb3.SingleBrowserTest; + +public class ComboBoxBackEndRequestsTest extends SingleBrowserTest { + + @Before + public void open() { + openTestURL("?debug"); + } + + private void open(int pageLength, int items) { + openTestURL(ComboBoxBackEndRequests.PAGE_LENGTH_REQUEST_PARAMETER + "=" + + pageLength + "&" + + ComboBoxBackEndRequests.ITEMS_REQUEST_PARAMETER + "=" + items + + "&restartApplication"); + } + + @Test + public void testInitialLoad_onlySizeAndFirstItemsRequested() { + verifyInitialLoadRequests(); + } + + @Test + public void testOpeningDropDown_noRequests() { + verifyInitialLoadRequests(); + + clearLogs(); + openPopup(); + + verifyNumberOrRequests( + "opening drop down should not have caused requests", 0); + + // select something to close popup + selectByClick("Item 2"); + + verifyNumberOrRequests("selecting should have not caused requests", 0); + + openPopup(); + verifyNumberOrRequests( + "opening drop down should not have caused requests", 0); + } + + @Test + public void testNoPaging_nullSelectionAllowed() { + final int pageLength = 0; + final int items = 20; + open(pageLength, items); + + verifyInitialLoadRequests(); + + clearLogs(); + + openPopup(); + + verifyNumberOrRequests("extra request expected", 1); + // TODO with pageLength 0, the cache strategy forces to fetch the whole + // set for opening popup + verifyFetchRequest(0, 0, items, null); + + verifyPopupItems(true, 0, items - 1); + + // pop up status not shown for only one page! + } + + @Test + public void testNoPaging_nullSelectionDisallowed() { + final int pageLength = 0; + final int items = 20; + open(pageLength, items); + + verifyInitialLoadRequests(); + + clearLogs(); + + triggerNullSelectionAllowed(false); + + openPopup(); + + verifyNumberOrRequests("extra request expected", 1); + // TODO the cache strategy forces to fetch the whole range, instead of + // just the missing 40-50 + verifyFetchRequest(0, 0, items, null); + + verifyPopupItems(false, 0, items - 1); + + // pop up status not shown for only one page! + } + + @Test + public void testPagingWorks_nullSelectionAllowed_defaultSizes() { + verifyPopupPages(ComboBoxBackEndRequests.DEFAULT_PAGE_LENGTH, + ComboBoxBackEndRequests.DEFAULT_NUMBER_OF_ITEMS, true); + } + + @Test + public void testPagingWorks_nullSelectionDisallowed_defaultSizes() { + triggerNullSelectionAllowed(false); + + verifyPopupPages(ComboBoxBackEndRequests.DEFAULT_PAGE_LENGTH, + ComboBoxBackEndRequests.DEFAULT_NUMBER_OF_ITEMS, false); + } + + @Test + public void testInitialPage_pageLengthBiggerThanInitialCache() { + // initial request is for 40 items + final int pageLength = 50; + final int items = 100; + + open(pageLength, items); + + verifyNumberOrRequests("three initial requests expected", 3); + verifySizeRequest(0, null); + verifyFetchRequest(1, 0, 40, null); + verifyFetchRequest(2, 40, 60, null); + + clearLogs(); + + verifyPopupPages(pageLength, items, true); + + // browsing through the pages should't have caused more requests + verifyNumberOrRequests("no additional requests should have happened", + 0); + } + + @Test + public void testPagingWorks_nullSelectionAllowed_customSizes() { + final int pageLength = 23; + final int items = 333; + open(pageLength, items); + + // with null selection allowed + verifyPopupPages(pageLength, items, true); + } + + @Test + public void testPagingWorks_nullSelectionDisallowed_customSizes() { + final int pageLength = 23; + final int items = 333; + open(pageLength, items); + + triggerNullSelectionAllowed(false); + + verifyPopupPages(pageLength, items, false); + } + + @Test + @Ignore("cache strategy is still broken for CB") + public void testPaging_usesCachedData() { + verifyInitialLoadRequests(); + clearLogs(); + openPopup(); + verifyNumberOrRequests( + "opening drop down should not have caused requests", 0); + + nextPage(); + + verifyNumberOrRequests("next page should have not caused more requests", + 0); + } + + @Test + public void testPaging_nullSelectionAllowed_correctNumberOfItemsShown() { + verifyInitialLoadRequests(); + openPopup(); + + verifyPopupItems(true, 0, 8); + + nextPage(); + + verifyPopupItems(false, 9, 18); + } + + @Test + public void testPaging_nullSelectionDisallowed_correctNumberOfItemsShown() { + verifyInitialLoadRequests(); + triggerNullSelectionAllowed(false); + + openPopup(); + + verifyPopupItems(false, 0, 9); + + nextPage(); + + verifyPopupItems(false, 10, 19); + } + + private void triggerNullSelectionAllowed(boolean expectedValue) { + $(CheckBoxElement.class).caption("emptySelectionAllowed").first() + .click(); + assertEquals(expectedValue, $(CheckBoxElement.class) + .caption("emptySelectionAllowed").first().isChecked()); + } + + private void clearLogs() { + $(ButtonElement.class).caption("Clear logs").first().click(); + verifyNumberOrRequests("logs should have been cleared", 0); + } + + private void selectByClick(String itemText) { + $(ComboBoxElement.class).first().getPopupSuggestionElements().stream() + .filter(element -> element.getText().equals(itemText)) + .findFirst().get().click(); + assertEquals("Wrong selected", itemText, + $(ComboBoxElement.class).first().getValue()); + } + + private void nextPage() { + assertTrue("no next page available", + $(ComboBoxElement.class).first().openNextPage()); + } + + private void previousPage() { + assertTrue("no previous page available", + $(ComboBoxElement.class).first().openPrevPage()); + } + + private void openPopup() { + ComboBoxElement element = $(ComboBoxElement.class).first(); + assertFalse("popup shouldn't be open", element.isPopupOpen()); + + element.getSuggestionPopup(); + + assertTrue("popup should be open", element.isPopupOpen()); + } + + private void verifyPopupPages(int pageSize, int totalItems, + boolean nullSelectionAllowed) { + final int numberOfPages = new BigDecimal( + totalItems + (nullSelectionAllowed ? 1 : 0)) + .divide(new BigDecimal(pageSize), RoundingMode.UP) + .intValue(); + openPopup(); + + verifyPopupStatus(pageSize, 1, numberOfPages, totalItems, + nullSelectionAllowed); + + for (int i = 2; i <= numberOfPages; i++) { + nextPage(); + verifyPopupStatus(pageSize, i, numberOfPages, totalItems, + nullSelectionAllowed); + } + + assertFalse("there should not be a next page", + $(ComboBoxElement.class).first().openNextPage()); + + if (numberOfPages < 2) { + return; + } + for (int i = numberOfPages - 1; i > 0; i--) { + previousPage(); + verifyPopupStatus(pageSize, i, numberOfPages, totalItems, + nullSelectionAllowed); + } + + assertFalse("there should not be a previous page", + $(ComboBoxElement.class).first().openPrevPage()); + } + + private void verifyPopupStatus(int pageSize, int currentPage, + int numberOfPages, int totalItems, boolean nullSelectionAllowed) { + int start; + int end; + + if (currentPage == 1) { // first page + start = 1; + end = pageSize - (nullSelectionAllowed ? 1 : 0); + } else if (currentPage == numberOfPages) { // last page + final int lastPageSize = (totalItems + + (nullSelectionAllowed ? 1 : 0)) % pageSize; + + start = pageSize * (currentPage - 1) + + (nullSelectionAllowed ? 0 : 1); + end = start + (lastPageSize == 0 ? pageSize : lastPageSize) - 1; + } else { // all other + start = pageSize * (currentPage - 1) + + (nullSelectionAllowed ? 0 : 1); + end = pageSize * currentPage - (nullSelectionAllowed ? 1 : 0); + } + + verifyPopupStatus(start, end, totalItems, currentPage, numberOfPages); + } + + private void verifyPopupStatus(int start, int end, int totalItems, + int currentPage, int numberOfPages) { + WebElement element = findElement(By.className("v-filterselect-status")); + assertEquals( + "Wrong status text on popup page " + currentPage + "/" + + numberOfPages, + start + "-" + end + "/" + totalItems, element.getText()); + } + + private void verifyPopupItems(boolean hasNullSelectionItem, int startIndex, + int lastIndex) { + List<String> popupSuggestions = $(ComboBoxElement.class).first() + .getPopupSuggestions(); + if (hasNullSelectionItem) { + String text = popupSuggestions.remove(0); + assertEquals("nullSelectionItem should be visible on page", " ", + text); + } + assertEquals("invalid number of suggestions", + lastIndex - startIndex + 1, popupSuggestions.size()); + for (int i = startIndex; i <= lastIndex; i++) { + assertEquals("unexpected item", "Item " + i, + popupSuggestions.get(i - startIndex)); + } + } + + private void verifyInitialLoadRequests() { + verifyNumberOrRequests("two initial requests expected", 2); + verifySizeRequest(0, null); + verifyFetchRequest(1, 0, 40, null); + } + + private WebElement verifyFetchRequest(int requestNumber, int startIndex, + int size, String filter) { + WebElement element = verifyRequest(requestNumber, "FETCH"); + // format is 1: FETCH 0 40 _filter_ + final String actual = element.getText(); + String expected = requestNumber + ": FETCH " + startIndex + " " + size + + (filter == null ? "" : " " + filter); + assertEquals("invalid fetch data", expected, actual); + return element; + + } + + private WebElement verifySizeRequest(int index, String filter) { + WebElement element = verifyRequest(index, "SIZE"); + if (filter != null) { + // format is "0: SIZE 0 2147483647 _filter_ + String currentFilter = element.getText().split("2147483647")[1]; + assertEquals("Wrong filter", filter, currentFilter); + } + return element; + } + + private WebElement verifyRequest(int index, String queryType) { + WebElement element = findElement(By.id(queryType + "-" + index)); + return element; + } + + private void verifyNumberOrRequests(String message, int numberOfRequests) { + List<WebElement> logRows = getLogRows(); + assertEquals(message, numberOfRequests, logRows.size()); + } + + private List<WebElement> getLogRows() { + return findElements(By.className("log")); + } +} |