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

Extend and refactor Scene class #2384

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

KodeCR
Copy link
Contributor

@KodeCR KodeCR commented Jan 27, 2020

This is mostly the second commit from #1471 so it can be reviewed and merged in stages, cherry-picked onto the current master. This:

  • extends the scene class with all fields additionally available with the Hue api and adds those to the database, and
  • refactors the scene class.
    This PR should not result in any functional changes to the deCONZ api or behaviour. The only change visible on the api side is that the additional fields are returned when retrieving lightstates for a scene. All previous fields should also still be included.

- add missing hue api fields to class
- update database with new fields
- refactor everything to make use of the new class

(cherry picked from commit faeadde)
@KodeCR KodeCR requested a review from manup January 27, 2020 21:36
@manup
Copy link
Member

manup commented Jan 31, 2020

Hey, thanks for the PR, overall it looks good and feels way cleaner compared to the former implementation. Due the amount of changes I won't merge it for the 2.05.73 since we need more testing with various lights and brands. So hopefully it will be added in 2.05.74.

@KodeCR
Copy link
Contributor Author

KodeCR commented Feb 3, 2020

Thanks for the review. Ok, that’s fine, I appreciate a bit more testing being done first.

@KodeCR
Copy link
Contributor Author

KodeCR commented Mar 11, 2020

Hi @manup, did you have a chance to do some testing? I'm keen to get this in before there are too many conflicts (e.g #2553 might create some). Thanks.

@stale
Copy link

stale bot commented Jun 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 8, 2020
@KodeCR
Copy link
Contributor Author

KodeCR commented Jun 8, 2020

I would really like this to be merged, I started this work over a year ago. What can I do to help? Could perhaps someone else review and test?

@stale stale bot removed the stale label Jun 8, 2020
@stale
Copy link

stale bot commented Jul 1, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 1, 2020
@KodeCR
Copy link
Contributor Author

KodeCR commented Jul 1, 2020

@SwoopX and/or @ebaauw, could you perhaps review? I'm happy of course to resolve conflicts if needed?

@stale stale bot removed the stale label Jul 1, 2020
@Mimiix Mimiix added the Backlog This label is assigned if it is implemented later. label Jul 10, 2020
@Mimiix
Copy link
Collaborator

Mimiix commented Jul 10, 2020

@KodeCR added backlog label so the bot stops bothering you.

@SwoopX
Copy link
Collaborator

SwoopX commented Jul 15, 2020

@KodeCR I'm afraid I won't be much of a help here since I don't use any scenes at all. However, I think it would be a pitty if there are good improvements and we potentially give them away...

@Thomas-Vos
Copy link
Contributor

I need these extra attributes for the next steps with Hue Essentials on a Phoscon bridge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backlog This label is assigned if it is implemented later.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants