Skip to content
This repository has been archived by the owner on Nov 4, 2023. It is now read-only.

fix: improve handling of merging defaults for popup tiles #519

Merged
merged 5 commits into from
Nov 21, 2020
Merged

fix: improve handling of merging defaults for popup tiles #519

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
311c9d4
move popup defaults to constants.js
akloeckner Oct 24, 2020
f47a0b8
use -item-transparent explicitly for camera, fix cursor/actions
akloeckner Oct 26, 2020
1f0c000
Merge branch 'master' into feature/defaults
akloeckner Nov 12, 2020
5bece8b
use new feature `action: false`
akloeckner Nov 12, 2020
52dfdab
fix classes option to always only ADD classes when merging configurat…
akloeckner Nov 21, 2020
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
66 changes: 13 additions & 53 deletions scripts/controllers/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import angular from 'angular';
import Hammer from 'hammerjs';
import { mergeConfigDefaults } from './main-utilities';
import { App } from '../app';
import { TYPES, FEATURES, HEADER_ITEMS, MENU_POSITIONS, GROUP_ALIGNS, TRANSITIONS, MAPBOX_MAP, YANDEX_MAP, DEFAULT_SLIDER_OPTIONS, DEFAULT_LIGHT_SLIDER_OPTIONS, DEFAULT_VOLUME_SLIDER_OPTIONS } from '../globals/constants';
import { TYPES, FEATURES, HEADER_ITEMS, MENU_POSITIONS, GROUP_ALIGNS, TRANSITIONS, MAPBOX_MAP, YANDEX_MAP, DEFAULT_SLIDER_OPTIONS, DEFAULT_LIGHT_SLIDER_OPTIONS, DEFAULT_VOLUME_SLIDER_OPTIONS, DEFAULT_POPUP_HISTORY, DEFAULT_POPUP_IFRAME, DEFAULT_POPUP_DOOR_ENTRY } from '../globals/constants';
import { debounce, leadZero, supportsFeature, toAbsoluteServerURL } from '../globals/utils';
import Noty from '../models/noty';

Expand Down Expand Up @@ -1635,70 +1635,30 @@ App.controller('Main', function ($scope, $timeout, $location, Api) {
};

$scope.openPopupHistory = function (item, entity) {
const layout = cacheInItem(item, '_popupHistory', () => ({
classes: ['-popup-landscape', ...(getItemFieldValue('history.classes', item, entity) || [])],
styles: {},
items: [angular.merge({
type: TYPES.HISTORY,
id: item.id,
title: false,
position: [0, 0],
classes: ['-item-fullsize'],
customStyles: {
width: null,
height: null,
top: null,
left: null,
},
}, getItemFieldValue('history', item, entity))],
const layout = cacheInItem(item, '_popupHistory', () => angular.merge(DEFAULT_POPUP_HISTORY(item, entity), {
classes: getItemFieldValue('history.classes', item, entity) || [],
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

grafik

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...)

Copy link
Contributor Author

@akloeckner akloeckner Nov 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

items: [getItemFieldValue('history', item, entity) || {}],
}));
$scope.openPopup(item, entity, layout);
};

$scope.openPopupIframe = function (item, entity) {
const layout = cacheInItem(item, '_popupIframe', () => ({
classes: ['-popup-fullsize', ...(getItemFieldValue('iframeClasses', item, entity) || [])],
styles: {},
const layout = cacheInItem(item, '_popupIframe', () => angular.merge(DEFAULT_POPUP_IFRAME(item, entity), {
classes: getItemFieldValue('iframeClasses', item, entity) || [],
items: [{
type: TYPES.IFRAME,
url: item.url,
id: {},
state: false,
title: false,
position: [0, 0],
classes: ['-item-fullsize'],
customStyles: angular.merge({
width: null,
height: null,
top: null,
left: null,
}, getItemFieldValue('iframeStyles', item, entity)),
customStyles: getItemFieldValue('iframeStyles', item, entity) || {},
}],
}));
$scope.openPopup(item, entity, layout);
};

$scope.openDoorEntry = function (item, entity) {
const layout = cacheInItem(item, '_popupDoorEntry', () => (
angular.merge({
classes: ['-popup-fullsize'],
styles: {},
items: [angular.merge({
state: false,
title: false,
position: [0, 0],
action: function (item, entity) {},
classes: ['-item-fullsize', '-item-non-clickable'],
customStyles: {
width: null,
height: null,
top: null,
left: null,
},
}, getItemFieldValue('layout.camera', item, entity)),
...(getItemFieldValue('layout.tiles', item, entity) || [])],
}, getItemFieldValue('layout.page', item, entity))
));
const layout = cacheInItem(item, '_popupDoorEntry', () => angular.merge(DEFAULT_POPUP_DOOR_ENTRY(item, entity), {
items: [
getItemFieldValue('layout.camera', item, entity) || {},
...(getItemFieldValue('layout.tiles', item, entity) || []),
],
}, getItemFieldValue('layout.page', item, entity) || {}));

$scope.openPopup(item, entity, layout);

Expand Down
59 changes: 59 additions & 0 deletions scripts/globals/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,65 @@ export const DEFAULT_VOLUME_SLIDER_OPTIONS = {
},
};

export const DEFAULT_POPUP_HISTORY = (item, entitiy) => ({
classes: ['-popup-landscape'],
styles: {},
items: [{
type: TYPES.HISTORY,
id: item.id,
title: false,
position: [0, 0],
action: function (item, entity) {},
secondaryAction: function (item, entity) {},
classes: ['-item-fullsize'],
customStyles: {
width: null,
height: null,
top: null,
left: null,
},
}],
});

export const DEFAULT_POPUP_IFRAME = (item, entity) => ({
classes: ['-popup-fullsize'],
styles: {},
items: [{
type: TYPES.IFRAME,
url: item.url,
id: {},
state: false,
title: false,
position: [0, 0],
classes: ['-item-fullsize'],
customStyles: {
width: null,
height: null,
top: null,
left: null,
},
}],
});

export const DEFAULT_POPUP_DOOR_ENTRY = (item, entity) => ({
classes: ['-popup-fullsize'],
styles: {},
items: [{
state: false,
title: false,
position: [0, 0],
action: function (item, entity) {},
secondaryAction: function (item, entity) {},
akloeckner marked this conversation as resolved.
Show resolved Hide resolved
classes: ['-item-fullsize', '-item-non-clickable', '-item-transparent'],
customStyles: {
width: null,
height: null,
top: null,
left: null,
},
}],
});

export const TILE_DEFAULTS = {
[TYPES.GAUGE]: {
settings: {
Expand Down
9 changes: 5 additions & 4 deletions styles/main.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1358,13 +1358,10 @@ camera_stream {
bottom: -@popupPadding;
right: -@popupPadding;
z-index: 0;

&.-th-camera, &.-th-camera_thumbnail, &.-th-camera_stream {
background-color: transparent;
}
}
.-item-non-clickable {
/* use classes: ['-item-non-clickable'] to display as non-interactive tile */
cursor: auto;
&:active {
margin: 0;
border: none;
Expand All @@ -1373,6 +1370,10 @@ camera_stream {
display: none;
}
}
.-item-transparent {
/* use classes: ['-item-transparent'] to remove background color */
background-color: transparent;
}
}
}

Expand Down