fix: improve handling of merging defaults for popup tiles

[akloeckner]

This will let the user overwrite default popup layouts. And it makes the code more readable.

[rchl]

It doesn't look like angular.merge will merge those array values:

> angular.merge({ a: [1] }, { a: [2] }).a
> [2]

[rchl]

I think https://www.npmjs.com/package/lodash.merge should work. angular.merge is deprecated anyway and they recommend lodash.merge.

[akloeckner]

The question for this line might be: What is actually expected? angular.merge will merge by key, which is 1,2,3,... in the case of arrays. So, values in the same place will be overwritten, values in other places will be added. Since both use cases should be possible, I'd say this behavior is reasonable to expect.

Here's a more complete test case:

If we wanted only add user-defined classes, we could do so like this:

classes: [ , ...(getItemFieldValue('history.classes', item, entity) || [])],

[rchl]

In the case of classes, I think the default behavior should be to append user classes to the ones that are there already because then the user doesn't need the knowledge of internal classes to add something. The [ , ...] also requires knowledge of how many internal classes are already applied and is error-prone in case something changes internally in the future.

[akloeckner]

I meant, we could use the [, ...] in main.js. The users should definitely not need to to this themselves.

If we were to use another library, I'll need help to include it in the bundle. Just a hint: The merge of arrays will need to use the current (angular) behavior in a few prominent places, e.g., the main tile in popups (i.e. camera, iframe, history)...

In summary, I suggest to change the classes line(s) in main.js in the [, ...] way.

[akloeckner]

An additional thought on that is: we have two kinds of places where we would want to merge tile configurations
a) before "evaluation" of functions and templates, prominent example is during config loading.
b) after "evaluation" of functions and templates, prominent example might be the popups.

The a) case could be solved by nesting functions:

classes = newFunction(item) { objA.concat( oldFunction(item) ) }

Question is: Which item is meant in each of the functions? During config loading it will easy, because it can only be the same item. In the case of popups, it's not that easy. The function in the default will probably want to use the item in the popup. The function in the calling item will probably want to use that calling item.

So, I'm under the impression, it might boild down to the point: when do we actually want to evaluate functions and templates?

(Sorry for turning into a philosopher. :-) Maybe I should wait for you to be less ocupied...)

[akloeckner]

I updated akloeckner/TileBoard@feature/defaults...akloeckner:test/merge-tiles in a way that I believe should handle all cases well. (It's untested, so treat it as a sketch.)

• classes will always be concatenated.
• to that end, everything that is not an array is embedded in an array
• in order to allow for functions and templates, I wrap everything in another anonymous function and call this.parseFieleValue on both properties to be merged
• functions and templates are thus evaluated against the same item always
• unless we explicitly pass differently evaluated properties to the merger, such as in the line of this discussion. So, we are in total control of what is evaluated when and against which item.

Possible downsides:

• everything is parseFieldValueed. But that should be ok, because non-parseable properties are simply piped through.
• properties might become parsed multiple times. But that should be ok for the same reason.

[rchl]

Didn't mean to leave you here talking to yourself but I don't have much capacity for it currently.

I guess go ahead with your plan and I'll likely approve after a brief look :)

[akloeckner]

No problem. I prefer to put down my notes, so you could pick up the line of thought. I'll give it another round of thinking and then go ahead.

[akloeckner]

I added this approach to the PR. Would you mind a quick look? ;-)