Skip to content

Property support: Use static unmodifiable Maps for common defaults #2041

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.

Sign up for GitHub

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

Open
matthiaso wants to merge 1 commit into
base: releases/26.2
Choose a base branch
from
Open

Property support: Use static unmodifiable Maps for common defaults #2041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
251 changes: 251 additions & 0 deletions ...nt.test/src/test/java/org/eclipse/scout/rt/client/ui/BasicPropertySupportFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,251 @@
/*
* Copyright (c) 2010, 2026 BSI Business Systems Integration AG
*
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*/
package org.eclipse.scout.rt.client.ui;

import static org.junit.Assert.*;

import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Predicate;
import java.util.stream.Collectors;

import org.eclipse.scout.rt.client.ui.action.AbstractAction;
import org.eclipse.scout.rt.client.ui.action.menu.AbstractMenu;
import org.eclipse.scout.rt.client.ui.action.menu.IMenu;
import org.eclipse.scout.rt.client.ui.basic.table.AbstractTable;
import org.eclipse.scout.rt.client.ui.basic.table.IHeaderCell;
import org.eclipse.scout.rt.client.ui.basic.table.ITable;
import org.eclipse.scout.rt.client.ui.basic.table.columns.AbstractColumn;
import org.eclipse.scout.rt.client.ui.form.fields.AbstractBasicField;
import org.eclipse.scout.rt.client.ui.form.fields.AbstractFormField;
import org.eclipse.scout.rt.client.ui.form.fields.AbstractValueField;
import org.eclipse.scout.rt.client.ui.form.fields.IBasicField;
import org.eclipse.scout.rt.client.ui.form.fields.IBasicFieldUIFacade;
import org.eclipse.scout.rt.client.ui.form.fields.IFormField;
import org.eclipse.scout.rt.client.ui.form.fields.IValueField;
import org.eclipse.scout.rt.client.ui.form.fields.button.AbstractButton;
import org.eclipse.scout.rt.client.ui.form.fields.button.IButton;
import org.eclipse.scout.rt.client.ui.form.fields.groupbox.AbstractGroupBox;
import org.eclipse.scout.rt.client.ui.form.fields.groupbox.IGroupBox;
import org.eclipse.scout.rt.platform.IgnoreBean;
import org.eclipse.scout.rt.platform.reflect.DefaultValueMap;
import org.eclipse.scout.rt.platform.util.ObjectUtility;
import org.eclipse.scout.rt.testing.platform.mock.RegisterBeanTestRule;
import org.eclipse.scout.rt.testing.platform.runner.PlatformTestRunner;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;

/**
* Test that the {@link BasicPropertySupportFactory} more or less equal the actual defaults (e.g. changed defaults in the actual class should be reflected in this factory as well to avoid additional values pollution).
*/
@RunWith(PlatformTestRunner.class)
public class BasicPropertySupportFactoryTest {

protected P_TestBasicPropertySupportFactory m_propertySupportFactory = new P_TestBasicPropertySupportFactory();

@paolobazzi paolobazzi Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion add:

  @Before
  public void before() {
    m_propertySupportFactory.clear();
  }

@Rule
public RegisterBeanTestRule<BasicPropertySupportFactory> m_propertySupportFactoryRule = new RegisterBeanTestRule<BasicPropertySupportFactory>(BasicPropertySupportFactory.class, () -> m_propertySupportFactory);

@paolobazzi paolobazzi Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add test for org.eclipse.scout.rt.client.ui.BasicPropertySupportFactory#createFor using null as holder (default case in switch)

@Test
public void testAbstractMenu() {
new AbstractMenu() {
}.init();
P_DefaultValueMap lastDefaultValueMap = m_propertySupportFactory.getLastDefaultValueMaps().getFirst();
Map<String, Object> additionalValues = lastDefaultValueMap.getAdditionalValues();
assertEquals(Set.of(

@paolobazzi paolobazzi Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a message like 'found additional initally set property, check factory method, see org.eclipse.scout.rt.client.ui.BasicPropertySupportFactory'

IMenu.PROP_MENU_TYPES // Set is always copied in setMenuTypes, improvement would be to just store the original Set (it is also copied on get operation)
), additionalValues.keySet());
}

@Test
public void testAbstractColumn() {
new AbstractColumn() {
};
P_DefaultValueMap lastDefaultValueMap = m_propertySupportFactory.getLastDefaultValueMaps().getFirst();
Map<String, Object> additionalValues = lastDefaultValueMap.getAdditionalValues();
assertEquals(Set.of(), additionalValues.keySet());
}

@Test
public void testAbstractButton() {
new AbstractButton() {
}.init();
P_DefaultValueMap lastDefaultValueMap = m_propertySupportFactory.getLastDefaultValueMaps().getFirst();
Map<String, Object> additionalValues = lastDefaultValueMap.getAdditionalValues();
assertEquals(Set.of(
IButton.PROP_GRID_DATA,
IButton.PROP_GRID_DATA_HINTS,
IButton.PROP_STATUS_MENU_MAPPINGS, // maybe use static empty list to create a new default (change in behavior)?
IButton.PROP_KEY_STROKES, // maybe use static empty list to create a new default (change in behavior)?
IButton.PROP_CONTEXT_MENU
), additionalValues.keySet());
}

@Test
public void testAbstractGroupBox() {
new AbstractGroupBox() {
}.init();
P_DefaultValueMap lastDefaultValueMap = m_propertySupportFactory.getLastDefaultValueMaps().getFirst();
Map<String, Object> additionalValues = lastDefaultValueMap.getAdditionalValues();
assertEquals(Set.of(
IGroupBox.PROP_GRID_DATA,
IGroupBox.PROP_GRID_DATA_HINTS,
IGroupBox.PROP_STATUS_MENU_MAPPINGS, // maybe use static empty list to create a new default (change in behavior)?
IGroupBox.PROP_KEY_STROKES, // maybe use static empty list to create a new default (change in behavior)?
IGroupBox.PROP_CONTEXT_MENU,
IGroupBox.PROP_EMPTY, // usually group boxes are not empty, however ours here is
IGroupBox.PROP_HAS_VISIBLE_FIELDS, // usually group boxes should have fields, however ours does not
IGroupBox.PROP_GRID_COLUMN_COUNT,
IGroupBox.PROP_FIELDS, // maybe use static empty list to create a new default (change in behavior)? re-create on change of fields.
IGroupBox.PROP_BODY_LAYOUT_CONFIG,
IGroupBox.PROP_VISIBLE // usually group boxes are visible, ours is not
), additionalValues.keySet());
}

@Test
public void testAbstractBasicField() {
new AbstractBasicField(true) {
@Override
public IBasicFieldUIFacade getUIFacade() {
return null;
}
}.init();
P_DefaultValueMap lastDefaultValueMap = m_propertySupportFactory.getLastDefaultValueMaps().getFirst();
Map<String, Object> additionalValues = lastDefaultValueMap.getAdditionalValues();
assertEquals(Set.of(
IBasicField.PROP_GRID_DATA,
IBasicField.PROP_GRID_DATA_HINTS,
IBasicField.PROP_STATUS_MENU_MAPPINGS, // maybe use static empty list to create a new default (change in behavior)?
IBasicField.PROP_KEY_STROKES, // maybe use static empty list to create a new default (change in behavior)?
IBasicField.PROP_CONTEXT_MENU
), additionalValues.keySet());
}

@Test
public void testAbstractValueField() {
new AbstractValueField(true) {
}.init();
P_DefaultValueMap lastDefaultValueMap = m_propertySupportFactory.getLastDefaultValueMaps().getFirst();
Map<String, Object> additionalValues = lastDefaultValueMap.getAdditionalValues();
assertEquals(Set.of(
IValueField.PROP_GRID_DATA,
IValueField.PROP_GRID_DATA_HINTS,
IValueField.PROP_STATUS_MENU_MAPPINGS, // maybe use static empty list to create a new default (change in behavior)?
IValueField.PROP_KEY_STROKES, // maybe use static empty list to create a new default (change in behavior)?
IValueField.PROP_CONTEXT_MENU
), additionalValues.keySet());
}

@Test
public void testAbstractFormField() {
new AbstractFormField(true) {
}.init();
P_DefaultValueMap lastDefaultValueMap = m_propertySupportFactory.getLastDefaultValueMaps().getFirst();
Map<String, Object> additionalValues = lastDefaultValueMap.getAdditionalValues();
assertEquals(Set.of(
IFormField.PROP_GRID_DATA,
IFormField.PROP_GRID_DATA_HINTS,
IFormField.PROP_STATUS_MENU_MAPPINGS, // maybe use static empty list to create a new default (change in behavior)?
IFormField.PROP_KEY_STROKES, // maybe use static empty list to create a new default (change in behavior)?
IFormField.PROP_CONTEXT_MENU
), additionalValues.keySet());
}

@Test
public void testAbstractAction() {
new AbstractAction() {
}.init();
P_DefaultValueMap lastDefaultValueMap = m_propertySupportFactory.getLastDefaultValueMaps().getFirst();
Map<String, Object> additionalValues = lastDefaultValueMap.getAdditionalValues();
assertEquals(Set.of(), additionalValues.keySet());
}

@Test
public void testAbstractTable() {
new AbstractTable() {
}.init();
P_DefaultValueMap lastDefaultValueMap = m_propertySupportFactory.getLastDefaultValueMaps().getFirst();
Map<String, Object> additionalValues = lastDefaultValueMap.getAdditionalValues();
assertEquals(Set.of(
ITable.PROP_KEY_STROKES, // maybe use static empty list to create a new default (change in behavior)?
ITable.PROP_CONTEXT_MENU,
ITable.PROP_TABLE_ORGANIZER,
ITable.PROP_USER_FILTER_MANAGER
), additionalValues.keySet());
}

@Test
public void testAbstractWidget() {
new AbstractWidget() {
}.init();
P_DefaultValueMap lastDefaultValueMap = m_propertySupportFactory.getLastDefaultValueMaps().getFirst();
Map<String, Object> additionalValues = lastDefaultValueMap.getAdditionalValues();
assertEquals(Set.of(), additionalValues.keySet());
}

@Test
public void testHeaderCell() {
new AbstractColumn() {
};
P_DefaultValueMap lastDefaultValueMap = m_propertySupportFactory.getLastDefaultValueMaps().getLast(); // HeaderCell (created by column)
Map<String, Object> additionalValues = lastDefaultValueMap.getAdditionalValues();
assertEquals(Set.of(
IHeaderCell.PROP_COLUMN_INDEX
), additionalValues.keySet());
}

@Test
public void testCompleteness() {
Set<String> testMethods = Arrays.stream(BasicPropertySupportFactoryTest.class.getDeclaredMethods()).map(Method::getName).collect(Collectors.toSet());
Arrays.stream(BasicPropertySupportFactory.class.getDeclaredMethods()).filter(Predicate.not(Method::isSynthetic)).forEach(m -> {
String name = m.getName();
if (name.startsWith("createDefaultValueMapFor") && testMethods.contains("test" + name.substring(24))) {
return; // method is tested
}
else if (ObjectUtility.isOneOf(name, "createDefaultValueMap", "createFor")) {
return; // tested implicitly by all test methods
}
fail("Shouldn't there be a test for " + m);
});
}

@IgnoreBean
protected class P_TestBasicPropertySupportFactory extends BasicPropertySupportFactory {

private List<P_DefaultValueMap> m_lastDefaultValueMaps = new ArrayList<>();

@Override
protected Map<String, Object> createDefaultValueMap(Map<String, Object> defaultValues) {
P_DefaultValueMap defaultValueMap = new P_DefaultValueMap(defaultValues, true);
m_lastDefaultValueMaps.add(defaultValueMap);
return defaultValueMap;
}

public List<P_DefaultValueMap> getLastDefaultValueMaps() {

@paolobazzi paolobazzi Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to:

 public List<P_DefaultValueMap> getLastDefaultValueMaps() {
      if (m_lastDefaultValueMaps.isEmpty()) {
        fail("basic property support was not created by BasicPropertySupportFactory");
      }
      return m_lastDefaultValueMaps;
    }

    public void clear() {
      m_lastDefaultValueMaps.clear();
    }

--> this let widgets fail which forgot to implement to create the property support and use the default impl:

        return super.createPropertySupport();

instead of

  @Override
  protected BasicPropertySupport createPropertySupport() {
    return BEANS.get(BasicPropertySupportFactory.class).createFor(this);
  }

return m_lastDefaultValueMaps;
}
}

protected class P_DefaultValueMap extends DefaultValueMap {

public P_DefaultValueMap(Map<String, Object> defaultValues, boolean startEmpty) {
super(defaultValues, startEmpty);
}

protected Map<String, Object> getAdditionalValues() {
return m_additionalValues;
}
}
}
11 changes: 9 additions & 2 deletions org.eclipse.scout.rt.client/src/main/java/org/eclipse/scout/rt/client/ui/AbstractWidget.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2023 BSI Business Systems Integration AG
* Copyright (c) 2010, 2026 BSI Business Systems Integration AG
*
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
Expand All @@ -21,11 +21,13 @@

import org.eclipse.scout.rt.client.ui.action.IAction;
import org.eclipse.scout.rt.client.ui.desktop.IDesktop;
import org.eclipse.scout.rt.platform.BEANS;
import org.eclipse.scout.rt.platform.Order;
import org.eclipse.scout.rt.platform.annotations.ConfigProperty;
import org.eclipse.scout.rt.platform.classid.ClassId;
import org.eclipse.scout.rt.platform.holders.Holder;
import org.eclipse.scout.rt.platform.reflect.AbstractPropertyObserver;
import org.eclipse.scout.rt.platform.reflect.BasicPropertySupport;
import org.eclipse.scout.rt.platform.reflect.ConfigurationUtility;
import org.eclipse.scout.rt.platform.util.CollectionUtility;
import org.eclipse.scout.rt.platform.util.visitor.IBreadthFirstTreeVisitor;
Expand All @@ -46,7 +48,7 @@ public abstract class AbstractWidget extends AbstractPropertyObserver implements

private static final Logger LOG = LoggerFactory.getLogger(AbstractWidget.class);
private static final NamedBitMaskHelper ENABLED_BIT_HELPER = new NamedBitMaskHelper(IDimensions.ENABLED, IDimensions.ENABLED_GRANTED, IDimensions.ENABLED_SLAVE);
private static final String PROP_ENABLED_BYTE = "enabledByte";
public static final String PROP_ENABLED_BYTE = "enabledByte";

private final WidgetListeners m_listenerList;

Expand All @@ -70,6 +72,11 @@ protected final void callInitializer() {
setInitConfigDone(true);
}

@Override
protected BasicPropertySupport createPropertySupport() {
return BEANS.get(BasicPropertySupportFactory.class).createFor(this);
}

/**
* Will be called by {@link #callInitializer()} but only if {@link #isInitConfigDone()} returns false.
*/
Expand Down
Loading
Loading