vaadin / flow-components

Java counterpart of Vaadin Web Components
100 stars 66 forks source link

Use regular key mapper as a default unique key provider for the TreeGrid [2 days] #3156

Closed mshabarov closed 2 years ago

mshabarov commented 2 years ago

Describe your motivation

TreeGrid defines the default unique key provider for it's items, as follows:

    private final AtomicLong uniqueKeyCounter = new AtomicLong(0);
    private final Map<Object, Long> objectUniqueKeyMap = new HashMap<>();

    ValueProvider<T, String> defaultUniqueKeyProvider = item -> String.valueOf(
            objectUniqueKeyMap.computeIfAbsent(getDataProvider().getId(item),
                    key -> uniqueKeyCounter.getAndIncrement()));

This default provider basically uses the same approach as a regular key mapper: integer counter as a source for a keys. But it adds an extra collection and moreover, this collection is never cleaned up.

Describe the solution you'd like

Use KeyMapper class as a default fallback for unique key provider:

  1. Default unique key provider delegates to DataCommunicator's KeyMapper (not to the wrapper uniqueKeyMapper from 2., but to the one original one).
  2. Unique key mapper of the HierarchicalDataCommunicator delegates to key provider it it's not empty:
    private final KeyMapper<T> uniqueKeyMapper = new KeyMapper<T>() {
        @Override
        public String key(T object) {
            return Optional.ofNullable(uniqueKeyProviderSupplier.get())
                    .map(provider -> provider.apply(object))
                    .orElse(super.createKey());
        }
    };

Describe alternatives you've considered

No response

Additional context

No response

mshabarov commented 2 years ago

The solution described above triggers a circular calls between keyMapper of HierarchicalDataCommunicator and uniqueKeyProvider of TreeGrid causing StackOverflow.

Additionally, HDC and TreeGrid depends on each other in a following way:

Possible solution for this is to pass a holder object to the HDC instead of plain uniqueKeyProviderSupplier, which would contain the key provider supplier (the same as previously) and a consumer for keyMapper. Then HDC could call this consumer and transfer the original keyMapper (without key provider fallback) to the TreeGrid.

This solution would require a deprecation of existing HDC CTOR and adding a new one with this holder object described above in place of key provider supplier. TreeGrid would then use this new CTOR and get the existing keyMapper.

yuriy-fix commented 2 years ago

We would probably need to collaborate with the Flow team to check if we can enhance the HDC to preserve the keys that are still in use.

mshabarov commented 2 years ago

Flow and flow-components have a patch already, so let's close this ticket.