Fix awkward solution for merging popup defaults

[akloeckner]

Hey, I just stubled accros the new line in openPopup (introduced in #555 for reason of #553). Shouldn't the original use case actually be solved in the following way?

{
    // Appearance of actual tile not changed
    type: TYPES.SWITCH, // ... id etc...
    // Here we call openPopup with an explicit layout
    secondaryAction: function (item, entity) {
       this.$scope.openPopup(item, entity, 
          {
             tileSize: 100,
             items: [
                {
                   type: TYPES.SWITCH,
                   subtitle: 'Kitchen1',
                   position: [1, 3],
                   id: 'light.f1_r1_light_base_fan',
                   title: 'Test',
                   icons: {
                      on: 'mdi-spin mdi-fan',
                      off: 'mdi-fan',
                   },
                   states: {
                      on: "On",
                      off: "Off"
                   },
                },
             ]
          }
       );
    },
 }

The even more explicit solution would be to defined the popup key in the actual item as follows:

{
    // Appearance of actual tile not changed
    type: TYPES.SWITCH, // ... id etc...
    // Define the popup key explicitly
    popup: {
             tileSize: 100,
             items: [
                {
                   type: TYPES.SWITCH,
                   subtitle: 'Kitchen1',
                   position: [1, 3],
                   id: 'light.f1_r1_light_base_fan',
                   title: 'Test',
                   icons: {
                      on: 'mdi-spin mdi-fan',
                      off: 'mdi-fan',
                   },
                   states: {
                      on: "On",
                      off: "Off"
                   },
                },
             ]
          },
    // Then call openPopup as usual
    secondaryAction: function (item, entity) {
       this.$scope.openPopup(item, entity);
    },
 }

I find it quite strange to allow creating items on the fly, because we rely in various places on those items being pointers into the CONFIG variable. The one issue fixed here is probably just a very small edge case. E.g., the layout is still not default-merged with #555.

The only thing that my two solutions above won't do as the original proposal, is that the popup title cannot be set this way. This should be introduced as a new feature, in order to not require users to go through the lengths of defining complete "ghost" tiles that will never be displayed themselves...

What do you think?

[rchl]

I find it quite strange to allow creating items on the fly, because we rely in various places on those items being pointers into the CONFIG variable.

I didn't know that it's not "allowed". openPopup is not really documented so I guess that it wasn't really disallowed either.

Not sure what you mean that there is a reliance on stuff being pointers. Can you give an example?

But all in all, I agree that it's a bit of a mess right now and I'm not entirely against forcing users to do it another way. Only that we probably should try to document better how this should be used. For example, having a popup property on a switch tile is not really allowed per-documentation so it wouldn't be clear right now.

[akloeckner]

You're right. There's nothing "disallowed", currently, :-)

As example: Take the caching with cacheInItem. It expects a persistent item to use for caching. (And it's also used by openPopup.) This won't work with on-the-fly items, would it? There's a lot of that stuff, even in the very original core code, e.g itemStyles, which set styles on the item.

I must admit, though, that creating the layout on the fly might cause similar headaches later on with the layout.items...

Maybe you're right again and it's not worth bothering too much. (Unless we start a general "clean-up" action.) Probably, an occasionally failing cache will not break anything. I just have the impression it's a bit over the top to have code in the codebase, specifically for a usecase which could be achieved in a cleaner (at least from CONFIG perspective) way.