Skip to content

Optional characteristics for lightbulbs #70

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

Closed
wants to merge 1 commit into from

Conversation

ccutrer
Copy link
Collaborator

@ccutrer ccutrer commented Feb 14, 2019

No description provided.

@timcharper
Copy link
Contributor

@ccutrer Thanks a lot for this change! This seems to be quite in line with my proposal for #63. @beowulfe how do you feel about that proposal and this change? I'd really like optional characteristics to not require some trait for the reasons outlined in #63.

@timcharper
Copy link
Contributor

@beowulfe I'd really like to get your opinion on this; I'm trying to work towards integrating my openhab2 homekit changes merged in to master; if you think this can make a 1.1.x release, then I'll base my changes on it :)

@timcharper
Copy link
Contributor

@beowulfe gentle nudge here! :) I know you're super busy, if there's anything I can do to help get this stuff merged and a release out, let me know. I'm just eager to integrate my openhab contributions before they diverge too far from master.

@timcharper
Copy link
Contributor

@ccutrer looks like this breaks backwards compat. Let's hold the merge until we release another 1.1.x version, and then we can bump the base version to 1.2.x and break all the things.

/** Unsubscribes from changes in the brightness of the light. */
void unsubscribeBrightness();
@Deprecated
public interface DimmableLightbulb extends Lightbulb, Brightness {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been thinking about it... we should really just do away with all of these interfaces and have of the allowed (but optional) characteristics in the Lightbulb itself.

Copy link
Contributor

@timcharper timcharper left a comment

Choose a reason for hiding this comment

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

Let's get rid of all the optional-characteristic interfaces

@ccutrer
Copy link
Collaborator Author

ccutrer commented Mar 10, 2019

Totes. That was all an attempt to maintain backwards compat (which didn't work out). Breaking that will make it all a lot easier.

@ccutrer ccutrer force-pushed the optional_characteristics branch from 578c8be to e2b07dd Compare March 10, 2019 20:55
@ccutrer
Copy link
Collaborator Author

ccutrer commented Mar 10, 2019

PR updated with just the lightbulb refactor, removing the extra interfaces. I'll separate fan into a new PR.

* Extends {@link Lightbulb} with color settings. This will usually be implemented along with {@link
* DimmableLightbulb}, but not necessarily so.
* Extends {@link com.beowulfe.hap.accessories.Lightbulb} with color settings. This will usually be
* implemented along with {@link DimmableLightbulb}, but not necessarily so.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ooh, this java doc needs updated.

@ccutrer ccutrer changed the title Optional characteristics Optional characteristics for lightbulbs Mar 10, 2019
…ightbulb

maintaining compatibility with the now-deprecated DimmableLightbulb and
ColorfulLightbulb interfaces
@ccutrer ccutrer force-pushed the optional_characteristics branch from e2b07dd to ec92d02 Compare March 10, 2019 21:02
*
* @author Andy Lintner
*/
public interface ColorfulLightbulb extends Lightbulb {

public interface Color {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like where this is going. After some thought, I wonder if this interface is probably more trouble than it's worth.

Perhaps we can just axe it and have the respective characteristics take lambda functions as appropriate. For read-only characteristics, it takes only a getter function and a subscription. For write-only, only a setter. Then the respective accessories can just return an Optional of the characteristic, with the lambda functions defined as needed?

WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, I like that.

@ccutrer ccutrer closed this Jun 1, 2020
@ccutrer ccutrer deleted the optional_characteristics branch October 18, 2022 15:53
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 this pull request may close these issues.

2 participants