SAP / openui5

OpenUI5 lets you build enterprise-ready web applications, responsive to all devices, running on almost any browser of your choice.
http://openui5.org
Apache License 2.0
2.97k stars 1.24k forks source link

MultiInput does not update the model on item-add #2965

Closed wernerdaehn closed 4 years ago

wernerdaehn commented 4 years ago

I have a MultiInput control bound to a model, two-way binding.

My initial problem was when adding an item, it was added at the end of the list although the control has a sorter defined. When trying to understand that, I found that new items are not added to the model at all!?!?

Any explanations?

https://jsbin.com/qojavih/edit?html,js,output

My goal is, by default the model has the values "step1" and "step2", when I manually add "step0" the oModel.getJSON() is printed. However "step0" does not show up and is not being put at the start of the list.

I can obviously manipulate the model manually in the updateToken() event but would like to understand why this does not happen automatically. Bug or misunderstanding on my side?

Thanks in advance!

flovogt commented 4 years ago

Hello @wernerdaehn , Thank you for sharing this finding. I've created an internal incident 2080321380. The status of the issue will be updated here in GitHub. Regards, Florian

codeworrior commented 4 years ago

Well, I wanted to point you to the corresponding documentation, but obviously this is not as clearly documented as it could be:

Two-Way binding is only supported for Property bindings.

For aggregations (like the tokens in the MultiInput or the items in a List), there's no abstraction in the UI5 model API how to create new entries in the aggregations / lists. The reason to my understanding is, that there's no single common way how new entries should be created. E.g. some models might prefer a factory approach to create new items whereas others allow to just create an item and add it to an existing list/collection. Server models might want to create new entries on the server side etc. Having a broad API that covers all these variants would be an (presumably expensive) option. UI5 instead requires entities to be created in the model (with model specific APIs / contracts). They should show up in the UI then automatically, via the normal one-way binding mechanics.

Back to the documentation: the only hints that I could find are

'yet' in this second place might be read as a sign of hope, but to my knowledge, there are no plans to implement two-way binding for aggregations. OData V4 has invested some effort to make entity creation easier to handle. But still, this is a one-way operation (create in the model first, then let the UI be updated).

I understand that for the MultiInput, this is not at all convenient as the entity creation is so implicit.

wernerdaehn commented 4 years ago

I have formulated multiple responses now, this is the final edit:

No, I don't think it is a documentation issue. The argumentation that aggregations do not support two-way bindings is running in the wrong direction. For me the argumentation chain would be: Controls do not provide out of the box functionality to add/remove elements in aggregations, e.g. the sap.m.table has no no native functionality to modify the table contents by adding rows. If the aggregation do not change, the model does not change.

The multiinput is a control that allows modifications of the items aggregation. And it is very odd that the model does not reflect that. Also, why do tables lack the native functionality to create items? Because the items can be complex. Item, ListItem, CustomItem,... and all with very different settings. So fully understandable. But again, beside the point. The table does not support items to the aggregation. It is not the case that it does add items but does not update the model.

The second reason why a table does not allow adding items is the two way binding as such. We define the complex binding from model to ListItem. Who says there is a simple inverse function? Example: text="{name} ({code})" would be rendered as "Screw (MAT-01)". When a user enters that text, you would need to update the name and code property if that is even possible.

I am fine with not updating the aggregations.

Having said that, I have no solution. If the token is mapped with key="{key}" and text="{text}" all is fine. But as soon as it gets more complex... Adding a property String[] getKeys() and String[] getTexts() and use that as binding would require a different Model structure, wouldn't it?

wernerdaehn commented 4 years ago

I have been thinking about it again. How about that statement:

The root cause is the validator function. It returns a Token but should actually return an object that is then added to the model.

If you agree with that, the solution would be simple: A validator function can return either a Token or an object. If it is the Token, then everything remains as is. Token is added, no model change. If it is an object instead and the control's tokens aggregation is bound to a model and two-way binding is turned on, then the model is updated thus refreshing automatically (not yet!!) the control. If the conditions are not met -> error.

This could actually be a role model for other controls with two way binding and aggregations as well. They could get a validator function returning either a List Item or a model row.

hristop commented 4 years ago

Hi @wernerdaehn ,

We have updated the documentation of the MultiInput and created a sample which illustrates how the update of the model can happen. The new sample will be available in version 1.84.

Why we do not want for the controls to update the model the answer is simple and complicated in the same time. The controls should not know about anything outside themselves. They are building blocks and should stay such. For us, the model bound to the control is part of another layer for which the application or this layer should be taking care. There are other implications on control level, which I will leave aside as they are technical and solutions would probably be found with the appropriate amount of effort (which will not be small). As the application developers depend on the API of the controls, we in the same way depend on the APIs of the core framework functionalities provided.

In this particular case, the MultiInput is providing enough events and has enough API via which the application developer can update the model properly and explicitly for the business logic as needed.

If in future the core provides the ability for the aggregations to work with two way bindings, we will surely make such thing work not only in the MultiInput, but in many other controls as well.

We will make sure to update the documentation for bindings in future so that it is more clear on the matters mentioned above.

Best Regards, Hristo

wernerdaehn commented 4 years ago

Thanks Hristo. I am fine with your answer and decision, however, imagine we would talk about an input control. Your response would read "The text input control should not update the model and there are enough events and methods so you can update the model when the user type in text". As you said, can be seen so or so. Your key argument is, user implementation is simple and a generic implementation would be difficult. In that case it is simply not worth it, as you rightly pointed out.

Anyway, thanks for the feedback, appreciate your time!