Skip to content
This repository has been archived by the owner on May 23, 2019. It is now read-only.

[Feature] Screen Saver #287

Closed

Conversation

LuckyMallari
Copy link
Contributor

@LuckyMallari LuckyMallari commented Jan 29, 2018

New Feature: ScreenSaver

Resolves #282

Same features from #282
I made the feature name simple, just call it screen saver, since users can simply navigate to a dashboard when screensaver kicks in. That dashboard page could have another album slideshow, etc. Another feature is to rotate the dashboards (configurable).

Signed-off-by: Lucky Mallari luckymallari@gmail.todotcom

Demo
ss_demo

Configurable
ss_settings

Error Checking
ss_errorchecking

Friendly Help messages
ss_help

Responsive
ss_responsive

Lucky Mallari added 22 commits January 26, 2018 15:57
Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
Added Translation keys and values

Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
Added more missing translation keys
Added error message if no dashboards setup
Disable checkbox if no dashboards configured
Added translation keys for help
Updated md files and package.json
Gulped scss
Added more translation keys for the button text
Text changes
Added and Implemented ui-sortable
Style changes
Gulped
Added more translation keys

Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
More code improvements
Admin.pot changes

Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
More changes to uib-alerts
Styling changes

Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
Admin.pot translation additions
Reset screensaver _config on dashboard settings changes

Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
Gulped

Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
@ghys
Copy link
Member

ghys commented Feb 1, 2018

Alright, thanks for your work!

Didn't review the code yet, just had a quick look, but tested on an incognito window with no config and it doesn't work (and breaks the settings screen):

(index):1 This page was loaded non-securely in an incognito mode browser. A warning has been added to the URL bar. For more information, see https://goo.gl/y8SRRv.
widgets.module.js:38 Registered widget type: button
widgets.module.js:38 Registered widget type: chart
widgets.module.js:38 Registered widget type: clock
widgets.module.js:38 Registered widget type: colorpicker
widgets.module.js:38 Registered widget type: dummy
widgets.module.js:38 Registered widget type: frame
widgets.module.js:38 Registered widget type: image
widgets.module.js:38 Registered widget type: knob
widgets.module.js:38 Registered widget type: label
widgets.module.js:38 Registered widget type: selection
widgets.module.js:38 Registered widget type: slider
widgets.module.js:38 Registered widget type: switch
widgets.module.js:38 Registered widget type: timeline
widgets.module.js:38 Registered widget type: template
vendor.js:135 TypeError: _slideshowDashboards is not iterable
    at localStorageService.set.onStart.dashboards (screensaver.service.js:72)
    at initConfig (screensaver.service.js:79)
    at $timeout (screensaver.service.js:245)
    at vendor.js:176
    at f (vendor.js:61)
    at vendor.js:64
(anonymous) @ vendor.js:135
(index):31 Service worker not registered (not using HTTPS?): SecurityError: Only secure origins are allowed (see: https://goo.gl/Y0ZkNV).
openhab.service.js:310 openHAB 2 service configuration loaded
openhab.service.js:325 Adding widget from configuration: rollershutter-example
openhab.service.js:325 Adding widget from configuration: gauge
vendor.js:135 TypeError: Cannot read property 'onStart' of null
    at reConfig (screensaver.service.js:286)
    at m.$broadcast (vendor.js:165)
    at loadConfigurationFromLocalStorage (persistence.service.js:23)
    at persistence.service.js:47
    at vendor.js:148
    at m.$eval (vendor.js:163)
    at m.$digest (vendor.js:160)
    at m.$apply (vendor.js:163)
    at l (vendor.js:115)
    at K (vendor.js:119)
(anonymous) @ vendor.js:135
openhab.service.js:310 openHAB 2 service configuration loaded
openhab.service.js:325 Adding widget from configuration: rollershutter-example
openhab.service.js:325 Adding widget from configuration: gauge
vendor.js:135 TypeError: Cannot read property 'onStart' of null
    at reConfig (screensaver.service.js:286)
    at m.$broadcast (vendor.js:165)
    at loadConfigurationFromLocalStorage (persistence.service.js:23)
    at persistence.service.js:47
    at vendor.js:148
    at m.$eval (vendor.js:163)
    at m.$digest (vendor.js:160)
    at m.$apply (vendor.js:163)
    at l (vendor.js:115)
    at K (vendor.js:119)
(anonymous) @ vendor.js:135
openhab.service.js:59 Loaded 144 openHAB items

Also it causes the config to be loaded twice.

@ghys
Copy link
Member

ghys commented Feb 1, 2018

A few initial remarks:

  • The size of vendor.js goes from 740 KB of minified code to 1.1 MB (a 50% increase!). Can't accept 3 new dependencies including jQuery UI and this much added bulk for a single control buried in a settings screen. You can probably use gridster with a 1-column grid and a dropdown/"add" button along with maybe 15 lines of code to implement the reorderable list with no additional dependency.
  • I see you also added ngAnimate as a dependency. It adds hooks everywhere and causes side effects as soon as it's loaded. I once had to troubleshoot performance issues on a project which were caused by ngAnimate, so I've been avoiding it and still intend to. You can implement transitions with vanilla CSS.
  • HABPanel targets ES5 and your new service & controller is coded with ES6 features. This will break on older browsers which I know for a fact people still use (Safari 9 is an example) because they refurbish old hardware, mostly iPads, that can't run newer browsers. Therefore I'd like you to avoid arrow functions, spread operators, replace for...of by angular.forEach, let/const by var etc. It's also customary to try to adapt to the project's coding style so that your code doesn't look out of place.

@LuckyMallari
Copy link
Contributor Author

Yeah I'll work on these suggestions in a bit (currently hammered at work)

Lucky Mallari added 4 commits February 2, 2018 06:31
Remove angular-ui-sortable (lib, jQuery, and jQueryUI)
Implemented ng-sortable
Style changes
Removed unnecessary classes

Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
@LuckyMallari
Copy link
Contributor Author

LuckyMallari commented Feb 2, 2018

with no config and it doesn't work

Fixed

The size of vendor.js goes from 740 KB of minified code to 1.1 MB (a 50% increase!).

Removed them all and used ng-sortable (which uses vanilla js). Preferred not to reinvent the wheel.

I see you also added ngAnimate as a dependency

Removed unnecessary animations. This is meant to be a screensaver, which is triggered when user is idle so animations, I believe, are totally redundant here.

HABPanel targets ES5 and your new service & controller is coded with ES6 features.

Completely downgraded to ES5 style

@ghys
Copy link
Member

ghys commented Feb 2, 2018

Thanks! This looks better :)

Removed them all and used ng-sortable (which uses vanilla js). Preferred not to reinvent the wheel.

That works.
Speaking of which, is there a reason why you reimplemented ng-idle which you said you would use in #282?

Removed unnecessary animations. This is meant to be a screensaver, which is triggered when user is idle so animations, I believe, are totally redundant here.

Agreed, I'm not a big fan of overusing animations (I try to always think about lower-end hardware which a lot of users use HABPanel on) but I have nothing against them per se, I only avoid ngAnimate :)

I'll do a proper code review next week, but some additional general remarks about the presentation:

  • maybe the settings about events to consider could be put in an "advanced" section, it's pretty technical and most users won't care
  • there aren't a lot of settings in the end so they could be a simple page separated by <h4>Headings</h4> instead of tabs, there isn't a lot of content in the tabs
  • The 'play' button in the dashboards header should only be displayed if the screensaver is enabled
  • why save the settings in local storage only and not in the panel configuration like the other stuff? The screensaver is likely to be used on all devices using the panel configuration and wouldn't have to be configured on every device, that's the point of it

@LuckyMallari
Copy link
Contributor Author

That works.
Speaking of which, is there a reason why you reimplemented ng-idle which you said you would use in #282?

Ng-Idle was too big for what it is. It had 3 other events I didn't need but lacked the one I actually do. It doesn't not raise an even when the idle ends (user is not idle any more). It's "idle end" event is trigerred when the user does not do anything after x time. The idle end event if Ng-Idle is basically the beginning of the "warning" phase.

maybe the settings about events to consider could be put in an "advanced" section, it's pretty technical and most users won't care

Yeah this one makes sense. I'll try to move it out further and not the first one they see.

there aren't a lot of settings in the end so they could be a simple page separated by 

Headings

instead of tabs, there isn't a lot of content in the tabs

Yeah I think there should basically only 2 tabs here.. General and Advanced.

The 'play' button in the dashboards header should only be displayed if the screensaver is enabled

They can disable the button just like they can for any of the header buttons. I meant this to still be there even if screensaver is off but the button setting is on. The reason is they might not like the screensaver to be automatic but still want the slideshow on demand.

why save the settings in local storage only and not in the panel configuration like the other stuff? The screensaver is likely to be used on all devices using the panel configuration and wouldn't have to be configured on every device, that's the point of it

That's the opposite for me. I have several habpanels in different devices and I want only a few of them with the screensaver turned on. On some of the devices, I want different sets if dashboards to rotate. So having the setting separate was a must for me.

Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
@LuckyMallari
Copy link
Contributor Author

Settings page changes
(should look same as everything else now)

ss_settings2
ss_settings1

Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
@ghys
Copy link
Member

ghys commented Feb 5, 2018

It looks better!
One day perhaps I'll reorganize all the settings screens with tabs or pills (panel storage, display, voice, widgets, screensaver) but not now.

They can disable the button just like they can for any of the header buttons. I meant this to still be there even if screensaver is off but the button setting is on. The reason is they might not like the screensaver to be automatic but still want the slideshow on demand.

At the very least it should be hidden if the screensaver is not configured because the button will do nothing and clutter the screen for no reason - long titles on narrow screens won't fit anymore and so on. Same thing as the speak button doesn't appear if the browser doesn't support it.
(I'm beginning to think those buttons should perhaps be opt-in instead of opt-out, I wonder if they're used that often)

That's the opposite for me. I have several habpanels in different devices and I want only a few of them with the screensaver turned on. On some of the devices, I want different sets if dashboards to rotate. So having the setting separate was a must for me.

I understand, but I still dont think this particular setting should be treated separately, it feels illogical. Besides, not having it tied to the panel configuration means you have to silently mess with the dashboard rotation config made by the user when it's changed. Need to think about this. Maybe there should be some sort of "local override" (with a warning) for certain settings - the "voice" and "switch when item changes" would also be good candidates.

@LuckyMallari
Copy link
Contributor Author

LuckyMallari commented Feb 5, 2018

It looks better!
One day perhaps I'll reorganize all the settings screens with tabs or pills (panel storage, display, voice, widgets, screensaver) but not now.

I initially wanted to use bootstrap panels but .panel itself was overridden

At the very least it should be hidden if the screensaver is not configured because the button will do nothing and clutter the screen for no reason - long titles on narrow screens won't fit anymore and so on. Same thing as the speak button doesn't appear if the browser doesn't support it.
(I'm beginning to think those buttons should perhaps be opt-in instead of opt-out, I wonder if they're used that often)

Yeah that makes sense. Should be an easy change.

I understand, but I still dont think this particular setting should be treated separately, it feels illogical. Besides, not having it tied to the panel configuration means you have to silently mess with the dashboard rotation config made by the user when it's changed. Need to think about this. Maybe there should be some sort of "local override" (with a warning) for certain settings - the "voice" and "switch when item changes" would also be good candidates.

I agree. And perhaps the better approach is to have a means to target specific HABPanel instances. Just on top of my head, perhaps from an item (string):

{
    id: someHabpanelId
    command: "switchdashboard",
    params: "the dashboard id to switch to"
}

.. and of course, we can still make it backwards-compatible, so other common users can still use the 'old' way

Lucky Mallari added 2 commits February 16, 2018 03:48
Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
Save to panel config instead of local storage.

Signed-off-by: Lucky Mallari <luckymallari@gmail.todotcom>
@LuckyMallari
Copy link
Contributor Author

Barely had time :)
All suggestions incorporated. Now uses/saves to panelConfig, and conditional button on header

@ghys
Copy link
Member

ghys commented Feb 16, 2018

Nice! No worries, I had a few things on my plate too :)

Cool that you put the settings in the panel configuration, it's more in line with the documented concepts so people won't be surprised their screensaver configs are not carried over with the other settings.

I haven't found a good solution for allowing "local overrides" for settings that doesn't feel odd or overengineered (it's an edge case I believe so introducing complexity for all users might not be the best), I'm even actually beginning to think it's not strictly necessary - if you want to deviate from the server-hosted panel configuration you can always switch to local storage and make the local changes. To pick up changes from the server, you change back to the panel configuration then back again to local storage and re-configure the local deviations.
It'll be a small hassle for some but IMO it's for the best because the vast majority of users won't have to deal with yet another obscure and complex setting, it'll help prevent breakages with incompatible local/remote settings combinations, and hey, you can't please everybody ;)

I'll try to do a thorough review on Sunday.

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

Alright, here are the promised review/comments ;)

@@ -40,7 +42,7 @@ <h2 class="dashboard-title" ng-if="!vm.dashboard.header.use_custom_widget">
tooltip-placement="top">
<i class="glyphicon" ng-class="vm.isListening ? 'glyphicon-stop' : 'glyphicon-comment'"></i>
</a>
<div class="scrollable" ng-show="vm.ready" gridster="vm.gridsterOptions">
<div class="scrollable widget-container" ng-show="vm.ready" gridster="vm.gridsterOptions">
Copy link
Member

Choose a reason for hiding this comment

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

New class? Is this needed?

// Uniqify our arrays
function onlyUnique(value, index, self) { return self.indexOf(value) === index; }
config.onStart.dashboards = config.onStart.dashboards.filter(onlyUnique);
config.onStart.dashboardsExcluded = config.onStart.dashboardsExcluded.filter(onlyUnique);
Copy link
Member

Choose a reason for hiding this comment

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

Why store the excluded dashboards in the config, wouldn't the ones in the rotation be enough? Seems weird and would need maintenance when the dashboard list is updated.
This is what I have in the panel configuration after storing the settings?

    "settings": {
        ...
        "screensaver": {
            "idleTimeoutSec": 300,
            "slideshowIntervalSec": 5,
            "isEnabled": true,
            "eventsToWatch": {
                "mousedown": true,
                "keydown": true,
                "mousewheel": true,
                "touchstart": true,
                "touchmove": true
            },
            "additionalEventsToWatch": [],
            "onStart": {
                "type": "slideshow",
                "dashboardsExcluded": [
                    {
                        "id": "test"
                    },
                    {
                        "id": "mobile"
                    },
                    {
                        "id": "map"
                    },
                    {
                        "id": "weather"
                    },
                    {
                        "id": "ec  vdsgsrg"
                    }
                ],
                "dashboards": [],
                "dashboard": null
            },
            "onStop": {
                "type": "stop",
                "dashboard": "test"
            }
        }
    }

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 initially wanted to have 2 containers for the dbs (included/excluded) so that I would not have to loop through the array while in the settings controller (for the included/excluded) draggable items. But let me see if I can improve this.

get: function () { return _config; }
})

var reConfig = function () {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this isn't needed anymore now that the dashboard rotation is stored in the panel config? The only remaining check necessary as I see it is if a dashboard has been removed while it's part of the screensaver rotation.

@@ -96,6 +97,24 @@ <h4 translate-keep-content translate="settings.panel.customwidgets.header">Custo
<span translate-keep-content translate="settings.panel.customwidgets.manage">Manage</span> &gt;
</a>

<br />
<br />
<h4 translate-keep-content translate="settings.panel.screensaver">ScreenSaver</h4>
Copy link
Member

Choose a reason for hiding this comment

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

Screensaver?

'$timeout'
];

function ScreensaverSettingsCtrl(
Copy link
Member

Choose a reason for hiding this comment

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

Can you use controlllerAs like the other controllers in this folder?

<option translate-keep-content translate="screensaver.settings.common.gotodashboard" value='gotodashboard'>Go to dashboard</option>
<option translate-keep-content translate="screensaver.settings.onstarttype.options.slideshow" value='slideshow'>Dashboard Slideshow</option>
</select>
<span class='helpinfo'>
Copy link
Member

Choose a reason for hiding this comment

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

Wonder whether this is really needed. Seems obvious, adds translation work and could be handled in the docs instead.

<input id="ss_idleTimeout" class="d-block" type="number" ng-min="10" ng-change="validate()" name="idleTimeout" ng-model="config.idleTimeoutSec"
required>
<span>
<em translate-keep-content translate="screensaver.settings.idletimeout.info">Seconds of being idle until screensaver kicks in</em>
Copy link
Member

Choose a reason for hiding this comment

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

Wonder whether this is really needed. Seems obvious, adds translation work and could be handled in the docs instead.

<label translate-keep-content translate="screensaver.settings.common.perform" for="ss_onStopType">Perform: </label>
<div>
<select id="ss_onStopType" class="d-block" name='onStopType' ng-model="config.onStop.type" ng-change="validate()">
<option translate-keep-content translate="screensaver.settings.common.stop" value='stop'>Stop</option>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe "Do nothing" is a better label?

<option translate-keep-content translate="screensaver.settings.common.stop" value='stop'>Stop</option>
<option translate-keep-content translate="screensaver.settings.common.gotodashboard" value='gotodashboard'>Go to dashboard</option>
</select>
<span class='helpinfo'>
Copy link
Member

Choose a reason for hiding this comment

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

Wonder whether this is really needed. Seems obvious, adds translation work and could be handled in the docs instead.

<a>{{::translations.showadvanced}}</a>
</label>
</div>
<ul class="settings-outer" ng-if="showAdvancedSettings">
Copy link
Member

Choose a reason for hiding this comment

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

I'm actually wondering if all that customization really adds value for the end user. Sensible hardcoded defaults could be enough, with an eventual textbox with csv values to allow events to be customized if deemed necessary for the (really) advanced users.

@kubawolanin
Copy link
Contributor

@LuckyMallari are you still on track? I'd love to use that feature myself! :)

@LKuech
Copy link

LKuech commented May 17, 2018

Same here. This feature is quite nice. This would allow me to get rid of a rule switching to an empty Dashboard (to avoid overheating my raspim, because the main dashboard has some animated weather icons... which seems to be a heavy task for a raspi 3).

@LuckyMallari & @ghys
I have an additional idea/request: In parallel to the dashboard slideshow, I would prefer to have an option showing a slideshow with a set of images or maybe a defined subfolder of the html folder of openhab containing images. Like this my raspi hosting the habpanel dashboards will act like a "picture frame" and would just show the dashboard when the touchscreen is touched or the item defined the the habpanel settings "Switch dashboard with item value" has been changed.

@jmccoy555
Copy link

@LKuech I'm achieving something similar with PicApport and a web frame.... would be nice for the in-build functionality though!

@AJErazzor
Copy link

agerly waiing to test this - has the project gone cold?

@LuckyMallari
Copy link
Contributor Author

Closing.
Continued on https://github.com/LuckyMallari/hpex

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants