poljvd / hyperion-webapp

Colorpicker webapp for Hyperion
19 stars 5 forks source link

Refactored the index page: #3

Closed bradcornford closed 10 years ago

bradcornford commented 10 years ago

Added the ability to turn on / off Hyperion. Added priority slider. Added a duration slider (Which is enabled with a button press). Added a clear priority channel button (When a command is set in the remote). Added a clear all priority channels button. Added status messages upon command execution.

Refactored the style document: Re-designed all the page to look like the 'Android remote'

Refactored the javascript into a document: Moved all javascript from the main page to a seperate document. Added all new javascript to this new document.

Created a remote command class: Enabled calls to the class methods to be chained to enabled differing attributes.

poljvd commented 10 years ago

Hi,

I think your changes look good. It also looks like you know what you are doing (in contrast to me), your code looks much cleaner and better organized. So thank you for that.

I merged your changes into a branch and I have changed the order of the items on the page to have the important items at the top and the less important options at the bottom. Please check my changes here: https://github.com/poljvd/hyperion-webapp/tree/bradcornford-master.

Before pushing everything to the master I have a few questions... 1) Can you make the on/off switch conditionally available? This switch will only work if the webserver is running on the same system as Hyperion so it should be removed when Hyperion is on an address other than 127.0.0.1 2) The duration does not seem to be remembered after a call to 'Change color'. Is this on purpose? 3) Is it possible to remove the 'enable duration' button and instead always have the slider where the maximum value means infinity/no-duration? I think that it will look better...

Thank you for your effort!

bradcornford commented 10 years ago

Hi,

Thanks for the above comments, really nice to get some feedback on this.

I do have one question though, do you want me to just fork that created branch, or clone it into a develop branch on my own repository then make another pull request from that?

Let me also answer your questions:

1) I had the exact same idea, but only after I created the pull request! Although I can develop a way to allow commands to be passed to a remote server when not using the 127.0.0.1 or local IP, with username / password as needed. This shouldn't be too difficult to integrate. 2) Yes, it was on purpose, as the duration seemed optional to me. So it reset every time to ensure it was optional, and as such stayed at infinite. I will store this in the session along with everything else. 3) Yes that is possible, I will do this along with storing the duration in a session variable as above.

I'm planning on getting the above changed done fairly quickly so should have something for you to look at in the next day or two.

Thanks again for looking at this pull request!

poljvd commented 10 years ago

Using a remote command over ssh is indeed another option. I leave this up to you.

I don't care much about how you provide the changes. I also could make you a collaborator on this repo so you can work directly on it if you want. Just let me know.

bradcornford commented 10 years ago

Out of interest, do all commands need the ability to be sent over ssh to a remote machine? If so, i could re-factor a little to create such functionality in the command class. So the command class would assume the commands are running locally, unless the authentication and server ip was different to the current serve.

Making me a collaborator could work, if you could do that it'd be great.

poljvd commented 10 years ago

I added you as collaborator.

Making the hyperion-remote commands remote available would be useful when hyperion-remote is not available locally. Also, there will be very few people who would use it in this way so I would say that it may not cost too much effort if you want to do this. I don't think it is necessary.

bradcornford commented 10 years ago

Thanks for that.

I have now pushed the changes you requested to the bradcornford-master branch.

Let me know if you require any other changes or upgrades.

poljvd commented 10 years ago

Thanks, I will have a look when I have some time next weekend!

poljvd commented 10 years ago

Hi,

I have merged all changes into the master and posted the update on the Raspbmc forum: http://forum.stmlabs.com/showthread.php?tid=11053&pid=97858#pid97858

One issue I foresee is that not everybody uses Raspbmc to run Hyperion. Those who use a stock Raspbian image don't have initctl to start/stop Hyperion. However, if they installed using the install script they should be able to start/stop using service. It would be nice to take a look at this when you have some time. Otherwise we just wait till the issue pops up...

bradcornford commented 10 years ago

Hi,

Thanks for this. Will be nice to get some proper feedback from users!

Oh right i see, whats the exact commands for Raspbian? I could do a check on the OS to see which option to use, which shouldn't take too much time.

poljvd commented 10 years ago

I finally checked what the commands are: service hyperion start service hyperion stop

Only problem is that those commands most probably need root priviledges to be executed...

bradcornford commented 10 years ago

Hi again,

No worries, I've added the logic around being able to support any command controller type. As for the requirement of running as root, this may be an issue I'm unable to resolve. Do you have any ideas around that issue?

Also made a few updates / changed to various things to make the app more usable.

I've pushed it to bcornford-master for you to take a look at before i merge it into master.

poljvd commented 10 years ago

I trust you that the changes you made are useful. So please make the changes you feel are useful and push them to the master.

The root issue probably needs to be solved in the OS by providing the execution rights for initctl or service to the user running the webserver.

bradcornford commented 10 years ago

Okay, will do. Once i have done so, I'll close this pull request.

I now have a copy of Rasbian on another pi that i can use to do some system based testing against, and also a copy of Raspbmc too, so I'll try formulate the necessary steps to achieve starting / stopping from the web app on both.

If for any reason you need to contact me you can do so at bradcornford{at}hotmail(dot)co(dot)uk