From 6ef0f6bcacc16da58f813071a2bfc92aa4512eca Mon Sep 17 00:00:00 2001 From: Markus Block Date: Thu, 11 Jan 2018 16:01:53 +0100 Subject: [PATCH] Fix ui save button autoassignment (#620) * Save button state (enabled or disabled) depends on data change but data change wasn't recognized. Fixed by setting values to view elements before window builder is called. Because the CommonDialogWindow stores internally the original values for later comparing them against the new values. DistributionSet Table stays now visible independently from the assignment checkbox, only the enabled/disabled state of the table changes. This keeps the window from changing its size when toggling the checkbox. Signed-off-by: Markus Block * Fixed sonar issues, and incorporated code review remarks. Signed-off-by: Markus Block --- .../hawkbit/ui/common/CommonDialogWindow.java | 38 +++++++----- .../DistributionSetSelectWindow.java | 58 +++++++++++++------ 2 files changed, 62 insertions(+), 34 deletions(-) diff --git a/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/common/CommonDialogWindow.java b/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/common/CommonDialogWindow.java index 279258c4c..ce0554ad2 100644 --- a/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/common/CommonDialogWindow.java +++ b/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/common/CommonDialogWindow.java @@ -65,10 +65,10 @@ import com.vaadin.ui.Window; import com.vaadin.ui.themes.ValoTheme; /** - * + * * Table pop-up-windows including a minimize and close icon in the upper right * corner and a save and cancel button at the bottom. Is not intended to reuse. - * + * */ public class CommonDialogWindow extends Window { @@ -104,7 +104,7 @@ public class CommonDialogWindow extends Window { /** * Constructor. - * + * * @param caption * the caption * @param content @@ -326,8 +326,8 @@ public class CommonDialogWindow extends Window { private boolean isMandatoryFieldNotEmptyAndValid(final Component currentChangedComponent, final Object newValue) { boolean valid = true; - final List> requiredComponents = allComponents.stream().filter(field -> field.isRequired()) - .collect(Collectors.toList()); + final List> requiredComponents = allComponents.stream().filter(AbstractField::isRequired) + .filter(AbstractField::isEnabled).collect(Collectors.toList()); requiredComponents.addAll(allComponents.stream().filter(this::hasNullValidator).collect(Collectors.toList())); @@ -396,12 +396,18 @@ public class CommonDialogWindow extends Window { if (c instanceof TabSheet) { final TabSheet tabSheet = (TabSheet) c; - for (final Iterator i = tabSheet.iterator(); i.hasNext();) { - final Component component = i.next(); - if (component instanceof AbstractLayout) { - components.addAll(getAllComponents((AbstractLayout) component)); - } - } + components.addAll(getAllComponentsFromTabSheet(tabSheet)); + } + } + return components; + } + + private static List> getAllComponentsFromTabSheet(final TabSheet tabSheet) { + final List> components = new ArrayList<>(); + for (final Iterator i = tabSheet.iterator(); i.hasNext();) { + final Component component = i.next(); + if (component instanceof AbstractLayout) { + components.addAll(getAllComponents((AbstractLayout) component)); } } return components; @@ -519,7 +525,7 @@ public class CommonDialogWindow extends Window { * Adds the component manually to the allComponents-List and adds a * ValueChangeListener to it. Necessary in Update Distribution Type as the * CheckBox concerned is an ItemProperty... - * + * * @param component * AbstractField */ @@ -550,7 +556,7 @@ public class CommonDialogWindow extends Window { /** * Checks if the safe action can executed. - * + * * @return = save action can executed = cannot execute * safe action . */ @@ -558,7 +564,7 @@ public class CommonDialogWindow extends Window { /** * Checks if the window can closed after the safe action is executed - * + * * @return = window will close = will not closed. */ default boolean canWindowClose() { @@ -567,7 +573,7 @@ public class CommonDialogWindow extends Window { /** * Saves/Updates action. Is called if canWindowSaveOrUpdate is . - * + * */ void saveOrUpdate(); } @@ -575,7 +581,7 @@ public class CommonDialogWindow extends Window { /** * Updates the field allComponents. All components existing on the given * layout are added to the list of allComponents - * + * * @param layout * AbstractLayout */ diff --git a/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/filtermanagement/DistributionSetSelectWindow.java b/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/filtermanagement/DistributionSetSelectWindow.java index 4734d0463..2b777c683 100644 --- a/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/filtermanagement/DistributionSetSelectWindow.java +++ b/hawkbit-ui/src/main/java/org/eclipse/hawkbit/ui/filtermanagement/DistributionSetSelectWindow.java @@ -58,7 +58,6 @@ public class DistributionSetSelectWindow private final transient TargetFilterQueryManagement targetFilterQueryManagement; - private CommonDialogWindow window; private CheckBox checkBox; private Long tfqId; @@ -71,7 +70,7 @@ public class DistributionSetSelectWindow this.targetFilterQueryManagement = targetFilterQueryManagement; } - private void initLocal() { + private VerticalLayout initView() { final Label label = new Label(i18n.getMessage("label.auto.assign.description")); checkBox = new CheckBox(i18n.getMessage("label.auto.assign.enable")); @@ -79,27 +78,26 @@ public class DistributionSetSelectWindow checkBox.setImmediate(true); checkBox.addValueChangeListener(this); + setTableEnabled(false); + final VerticalLayout verticalLayout = new VerticalLayout(); verticalLayout.addComponent(label); verticalLayout.addComponent(checkBox); verticalLayout.addComponent(dsTable); - window = new WindowBuilder(SPUIDefinitions.CREATE_UPDATE_WINDOW) - .caption(i18n.getMessage("caption.select.auto.assign.dist")).content(verticalLayout).layout(verticalLayout) - .i18n(i18n).saveDialogCloseListener(this).buildCommonDialogWindow(); - window.setId(UIComponentIdProvider.DIST_SET_SELECT_WINDOW_ID); + return verticalLayout; } - public void setValue(final Long distSet) { - dsTable.setVisible(distSet != null); + private void setValue(final Long distSet) { checkBox.setValue(distSet != null); dsTable.setValue(distSet); dsTable.setCurrentPageFirstItemId(distSet); + dsTable.setNullSelectionAllowed(false); } /** * Shows a distribution set select window for the given target filter query - * + * * @param tfqId * target filter query id */ @@ -108,7 +106,7 @@ public class DistributionSetSelectWindow final TargetFilterQuery tfq = targetFilterQueryManagement.get(tfqId) .orElseThrow(() -> new EntityNotFoundException(TargetFilterQuery.class, tfqId)); - initLocal(); + final VerticalLayout verticalLayout = initView(); final DistributionSet distributionSet = tfq.getAutoAssignDistributionSet(); if (distributionSet != null) { @@ -117,6 +115,12 @@ public class DistributionSetSelectWindow setValue(null); } + // build window after values are set to view elements + final CommonDialogWindow window = new WindowBuilder(SPUIDefinitions.CREATE_UPDATE_WINDOW) + .caption(i18n.getMessage("caption.select.auto.assign.dist")).content(verticalLayout) + .layout(verticalLayout).i18n(i18n).saveDialogCloseListener(this).buildCommonDialogWindow(); + window.setId(UIComponentIdProvider.DIST_SET_SELECT_WINDOW_ID); + window.setWidth(40.0F, Sizeable.Unit.PERCENTAGE); UI.getCurrent().addWindow(window); window.setVisible(true); @@ -124,27 +128,45 @@ public class DistributionSetSelectWindow /** * Is triggered when the checkbox value changes - * + * * @param event * change event */ @Override public void valueChange(final Property.ValueChangeEvent event) { - dsTable.setVisible(checkBox.getValue()); - if (window != null) { - window.center(); - + if (checkBox.getValue()) { + setTableEnabled(true); + } else { + dsTable.select(null); + setTableEnabled(false); } } + private void setTableEnabled(final boolean enabled) { + dsTable.setEnabled(enabled); + dsTable.setRequired(enabled); + } + /** * Is triggered when the save button is clicked - * + * * @return whether the click should be allowed */ @Override public boolean canWindowSaveOrUpdate() { - return !checkBox.getValue() || dsTable.getValue() != null; + return isFormularValid(); + } + + private boolean isFormularValid() { + return isAutoAssignmentDisabled() || isAutoAssignmentEnabledAndDistributionSetSelected(); + } + + private boolean isAutoAssignmentEnabledAndDistributionSetSelected() { + return checkBox.getValue() && dsTable.getValue() != null; + } + + private boolean isAutoAssignmentDisabled() { + return !checkBox.getValue(); } /** @@ -239,7 +261,7 @@ public class DistributionSetSelectWindow mainTextLabel = new Label(i18n.getMessage("message.confirm.assign.consequences.none")); } else { mainTextLabel = new Label( - i18n.getMessage("message.confirm.assign.consequences.text", new Object[] { targetsCount })); + i18n.getMessage("message.confirm.assign.consequences.text", targetsCount)); } layout.addComponent(mainTextLabel);