Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug report: Configuration options overriding each other #38

Open
xBelladonna opened this issue Nov 24, 2020 · 1 comment · May be fixed by #39
Open

Bug report: Configuration options overriding each other #38

xBelladonna opened this issue Nov 24, 2020 · 1 comment · May be fixed by #39

Comments

@xBelladonna
Copy link

xBelladonna commented Nov 24, 2020

First of all I'd like to thank you for releasing this stellar frontend integration! It's made a huge aesthetic difference to my dashboard and I very much appreciate your work on this project.

Describe the bug
It appears there is a race condition governing whether or not the included/excluded devices will prevail over the included/excluded users and vice versa. If using both included users and excluded devices configuration options as I do in my config, sometimes the excluded devices will not display the background but most times they do. Testing this with an excluded device and included user produces the same results as testing with an excluded user and included device. Removing one or the other configuration option resolves the problem.

EDIT: upon further testing it appears that explicitly setting the enabled bool to true will override both of these configuration options, i.e. no matter if the user and device both match a condition for exclusion, the animated background will appear regardless. This does not occur vice versa, i.e. if set to false, any included users/devices will not cause such an override.

Your Animated Background configuration

animated_background:
  enabled: true
  debug: true
  default_url: 'https://cdn.flixel.com/flixel/v26zyfd6yf0r33s46vpe.hd.mp4'
  included_users:
    - xBelladonna
  excluded_devices:
    - windows
    - android
  entity: sensor.sun_weather
  state_url:
    <huge map of sensor states and video url lists>

Version Numbers

  • Home Assistant: 0.118.2
  • Animated Background: v0.6.3

Browser console log
Console logs report device exclusion however animated background is still observed:

Animated Background DEBUG: Current device is excluded
Animated Background DEBUG: Removing view background for configuration: 
Object { enabled: true, debug: true, default_url: "https://cdn.flixel.com/flixel/v26zyfd6yf0r33s46vpe.hd.mp4", included_users: (1) […], excluded_devices: (2) […], entity: "sensor.sun_weather", state_url: {…} }
animated-background.js:44:15
Animated Background DEBUG: Starting, Debug mode enabled 
Animated Background DEBUG: Configured entity sensor.sun_weather is now above_horizon-cloudy 
true animated-background.js:44:15
Animated Background DEBUG: Current device is excluded
Animated Background DEBUG: Starting, Debug mode enabled 
Animated Background DEBUG: Configured entity sensor.sun_weather is now above_horizon-cloudy 
true animated-background.js:44:15
Animated Background DEBUG: Current device is excluded
Animated Background DEBUG: Starting, Debug mode enabled 
Animated Background DEBUG: Configured entity sensor.sun_weather is now above_horizon-cloudy 
true animated-background.js:44:15
Animated Background DEBUG: Current device is excluded
Animated Background DEBUG: Starting, Debug mode enabled 
Animated Background DEBUG: Configured entity sensor.sun_weather is now above_horizon-cloudy 
true animated-background.js:44:15
Animated Background DEBUG: Current device is excluded
Animated Background DEBUG: Removing view background for configuration: 
Object { enabled: true, debug: true, default_url: "https://cdn.flixel.com/flixel/v26zyfd6yf0r33s46vpe.hd.mp4", included_users: (1) […], excluded_devices: (2) […], entity: "sensor.sun_weather", state_url: {…} }

To Reproduce
Steps to reproduce the behavior:

  1. Configure both an included/excluded users and included/excluded devices section
  2. Set the configuration such that the current device/user will be included by one of these options (either by device or by user, either one works) and excluded by another, either by explicit or implicit inclusion/exclusion
  3. Clear browser cache and refresh page to ensure new settings are loaded
  4. Observe that background is still loaded (most of the time) despite browser console reporting exclusion of device/user

Expected behavior
It is expected that if a user/device matches a condition to be excluded, it will not be included by another configuration option, and vice versa, i.e. if a user has been explicitly excluded (or not explicitly included) then no matter if they are using an included (or not explicitly excluded) device, they will not observe an animated background, and if the user is included (or not explicitly excluded) but is using a device which has been explicitly excluded (or not explicitly included), they will also not observe an animated background.

It is also expected that the enabled option, if explicitly set to true, does not override the animated background display if a device/user has matched a condition for exclusion.

Device (please complete the following information):

  • OS: Windows, Android, Linux (tested on all three)
  • Browser: Firefox
  • Version 83.0 (64-bit)
@xBelladonna xBelladonna changed the title Bug report Bug report: Configuration options overriding each other Nov 24, 2020
@Villhellm
Copy link
Owner

Includes override excludes. That's the way the logic has been designed. Something has to take priority.
https://github.com/Villhellm/lovelace-animated-background/blob/master/dist/animated-background.js#L230 this is where the definitions start for the inclusion/exclusion rules. If you can come up with a more logical pattern I would be more than happy to look at a PR

@xBelladonna xBelladonna linked a pull request Nov 26, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants