Skip to content

Conversation

@Floyer007
Copy link
Contributor

While using the library in HA I just realized I forgot to add getRecordings() to the update function.
Sorry about that.

@Floyer007
Copy link
Contributor Author

Are you fine with this changes, especially with the test?
I just own a API-level 6 TV, so I made the assumption the structure is the same for API-level 1.

@elupus
Copy link
Collaborator

elupus commented Feb 28, 2023

V1 does not have this API, so you must make it conditional.

@Floyer007
Copy link
Contributor Author

Just one thing before I start reverting the changes for v1 and make it conditional.

Line 57 in data/v1.py for the test indicates the recordings feature like in v6:

        "jsonfeatures": {
            "recordings": ["List", "Schedule", "Manage"],
            "ambilight": ["LoungeLight"],
            "textentry": ["context_based", "initial_string_available"],
            "inputkey": ["key", "unicode"],
            "pointer": ["context_based"],
            "activities": ["browser"],
        },

Is this mock-data wrong then?

@elupus
Copy link
Collaborator

elupus commented Mar 1, 2023

Unclear. At least we don't know how the recording api would look on a v1 series tv. It could be that that section is fully wrong and that TV actually is some higher API version, but the reports we got for it was wrong.

@Floyer007
Copy link
Contributor Author

I see. I limited the feature to API level 6 now, as I can verify it working with my TV.

@Floyer007
Copy link
Contributor Author

Hi, any chance to get this merged?
Am I still missing something?

@Floyer007
Copy link
Contributor Author

May I ask for a second review?

Copy link
Collaborator

@elupus elupus left a comment

Choose a reason for hiding this comment

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

Looks good. I think you might need to fix the event function too for this to work when when a user is using eventing to update. But we can look at that later.

@elupus elupus merged commit 24ca6ac into danielperna84:master Apr 27, 2023
@Floyer007
Copy link
Contributor Author

You mean "notifyChange"?
I will look into it.

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