show the latest snapshot of a camera connected to Home Assistant

[pfefferle]

Description

This PR allows to ask for the latest snapshot of a camera connected to Home Assistant

Type of PR

• [ ] Bugfix
• [X] Feature implementation
• [ ] Refactor of code (without functional changes)
• [ ] Documentation improvements
• [ ] Test improvements

Testing

Say "Show me the latest snapshot of {Camera Name}."

[pfefferle]

@krisgesling I am not sure if I should also add a sound output.

[krisgesling]

maybe if not self.gui.connected then we want to say something?

otherwise the most I'd go for is a self.acknowledge() tone

[pfefferle]

@krisgesling what about playing that sound if connected or not. I liked the idea and added it also to the "open Home Assistant" command.

[krisgesling]

Yeah, I think it's worth us considering what we want the behaviour to be with or without a display connected. There's a few options as I see it:

• act as if a display is connected - and nothing will show because there's no display
• report that no display exists so they won't get the full experience
• provide different experiences if a display exists
• disable display focused intents if there is no gui connected

They last one I think might have merit here. If there's no display, should it even respond to utterances like "show me the latest snapshot"?

On the other hand, does having different functionality on different devices make it more confusing for users? eg If I say the same thing to a different Mycroft device with the same Skill installed and it says: "I'm sorry I don't know what that means" - that's kind of weird.

[pfefferle]

Now I see! I totally agree! To be honest, I never thought about the case that there might be no display connected (and don't ask what I thought self.gui.connected is checking)!

I would go with

report that no display exists so they won't get the full experience

and add an error message for that.

I will also add it to "open HomeAssistant"!

[Tony763]

I do not have display so from my point of view I would rather wanted to have intents which use booth to respond only with voice without info about no display. And intents that are only for gui to respond with info message that display is not connected.

[pfefferle]

@krisgesling @Tony763 I added an error if there is no gui available

[Tony763]

I will send translations tomorrow.

[pfefferle]

@krisgesling can you please re-review?

[krisgesling]

Oh, it would be great if we could add a test for this too

[pfefferle]

@Tony763 can you support me by testing the camera GUI response?

[Tony763]

@pfefferle I could try to write some tests, but I can't test gui as I run my HA and Mycroft on my headless intel server. I will look for some way to use RDP or similar protocol to stream video output.

[pfefferle]

@Tony763 @krisgesling the test will also run in headless mode doesn't it? Is it even possible to test for GUI output? so should I only test for the error case?

[Tony763]

In headless but give me a week or two, I will think about it ;)

út 7. 12. 2021 v 6:52 odesílatel Matthias Pfefferle < @.***> napsal:

@Tony763 https://github.com/Tony763 @krisgesling https://github.com/krisgesling the test will also run in headless mode doesn't it? Is it even possible to test for GUI output? so should I only test for the error case?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/MycroftAI/skill-homeassistant/pull/90#issuecomment-987591225, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKW4HUCBBZZVBSVFDZUGO3UPWOJXANCNFSM5GUVT3YA .

[krisgesling]

Currently there's no built-in tests to guarantee that it's actually rendered. However intent clashes are the bigger concern, so it would be good to check that the intent handler triggers for what it's meant to and not things like:

show me a picture of a cat

[pfefferle]

@krisgesling makes sense.

@Tony763 I added some basic tests, but the GitHub Action seems to be broken on my fork, any ideas? https://github.com/pfefferle/skill-homeassistant/runs/4463932766?check_suite_focus=true

[Tony763]

Yes, there was a breaking change in mycroft-core installation, fix for it is in PR #89.

[pfefferle]

I added tests, to check for an existing and non existing entity.

[pfefferle]

@krisgesling can you re-check?

[pfefferle]

@Tony763 @krisgesling this is weird... it has no issues on my fork https://github.com/pfefferle/skill-homeassistant/runs/4481764469

[Tony763]

It's okay, workflow change is not merged to HA repo yet, so it's fail there. Workflow on PR is always used a one from repository into which you sended a PR. It's a security feature. So you are unable to run malicious code before approve. When my change is merged, this will also work there.

[Tony763]

Pylint is not working as it is not installed. :slightly_smiling_face: In Your repo its okay.

Wait for @krisgesling to check my PR and merge. :slightly_smiling_face:

[Tony763]

Hi @pfefferle, to incorporate (activate) latest change in workflow to this PR, You have to rebase it with latest commit made to branch 20.08. Ping me if You need help with commands :slightly_smiling_face:

[pfefferle]

@Tony763 @krisgesling should be ready to review again :)