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.

Sign up for GitHub

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

Jump to bottom

Add stream preview to options flow in generic camera #133927

Merged
merged 5 commits into from
Dec 30, 2024
Merged

Add stream preview to options flow in generic camera #133927

merged 5 commits into from
Dec 30, 2024
+148 −71

Conversation

davet2001
Copy link
Contributor

@davet2001 davet2001 commented Dec 24, 2024 •
edited
Loading

Proposed change

#122563 added a preview of the video stream while setting up an IP camera via a config flow.
This PR adds the same preview to the options flow

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

@davet2001 davet2001 marked this pull request as ready for review December 24, 2024 23:15
@davet2001 davet2001 force-pushed the generic_camera_options_flow_stream_preview branch from a6c0bb5 to 9b0fa57 Compare December 25, 2024 10:08
@davet2001 davet2001 mentioned this pull request Dec 25, 2024
Closed
allenporter
allenporter requested changes Dec 26, 2024
View reviewed changes
homeassistant/components/generic/config_flow.py Show resolved Hide resolved
homeassistant/components/generic/config_flow.py
user_input[CONF_CONTENT_TYPE] = still_format
still_url = user_input.get(CONF_STILL_IMAGE_URL)
if still_url is None:
# If user didn't specify a still image URL,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this just be handled inside of async_test_still? This seems like extra detail spilling out.

(I realize this is not changing in this PR, but it shows up as a diff)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a copy/paste of what is in the existing config flow, so designed to be as similar as possible.
I would like to rationalise these into one combined function for config_flow/options_flow in future.

Yes, the image format assignment could probably go into async_test_still, but I would prefer to do that as a separate PR rather than make this one bigger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow up PR to address this point: #134330

homeassistant/components/generic/config_flow.py
),
**user_input,
CONF_CONTENT_TYPE: still_format
or self.config_entry.options.get(CONF_CONTENT_TYPE),
Copy link
Contributor

Choose a reason for hiding this comment

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

This also seems like it could be handled there given there is already code doing stuff like this and determining the defaults

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. I suggest it is put in a follow up PR though.

homeassistant/components/generic/config_flow.py Show resolved Hide resolved
homeassistant/components/generic/config_flow.py Outdated Show resolved Hide resolved
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft December 26, 2024 00:32
allenporter
allenporter reviewed Dec 28, 2024
View reviewed changes
Copy link
Contributor

@allenporter allenporter left a comment

Choose a reason for hiding this comment

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

Thanks for the detail. Looks good to me though we still have a pending question about GenericOptionsFlowHandler cast.

allenporter
allenporter approved these changes Dec 28, 2024
View reviewed changes
Copy link
Contributor

@allenporter allenporter 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. There is a test coverage warning so perhaps you can double check the config flow has 100% test coverage?

@davet2001 davet2001 force-pushed the generic_camera_options_flow_stream_preview branch from a8c4278 to 24b1d88 Compare December 28, 2024 21:37
@davet2001 davet2001 mentioned this pull request Dec 29, 2024
Merged
19 tasks
@davet2001 davet2001 force-pushed the generic_camera_options_flow_stream_preview branch from 24b1d88 to 418b9d2 Compare December 29, 2024 11:28
@davet2001 davet2001 marked this pull request as ready for review December 29, 2024 11:51
@home-assistant home-assistant bot requested a review from allenporter December 29, 2024 11:51
allenporter
allenporter reviewed Dec 29, 2024
View reviewed changes
Copy link
Contributor

@allenporter allenporter left a comment

Choose a reason for hiding this comment

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

I think it would be better to delete the dead code than add this given this can't happen yet today. Or leave it since the gap was added in my PR (sorry about that)

@davet2001
Copy link
Contributor Author

I think it would be better to delete the dead code than add this given this can't happen yet today. Or leave it since the gap was added in my PR (sorry about that)

Ok. done in 32ca8e5

@allenporter allenporter merged commit bf59241 into home-assistant:dev Dec 30, 2024
34 checks passed
@davet2001 davet2001 added this to the 2025.1.0 milestone Dec 31, 2024
@davet2001 davet2001 mentioned this pull request Dec 31, 2024
Merged
19 tasks
balloob pushed a commit that referenced this pull request Dec 31, 2024
@davet2001 @balloob
Add stream preview to options flow in generic camera (#133927)
e303a9a 
* Add stream preview to options flow

* Increase test coverage

* Code review: use correct flow handler type in cast

* Restore test coverage to 100%

* Remove error and test that can't be triggered yet
@github-actions github-actions bot locked and limited conversation to collaborators Jan 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@allenporter allenporter allenporter approved these changes

Assignees
No one assigned
Labels
by-code-owner cherry-picked cla-signed has-tests integration: generic new-feature Quality Scale: No score
Projects
None yet
Milestone
2025.1.0
Development

Successfully merging this pull request may close these issues.

Preview image missing during options flow on generic camera
3 participants
@davet2001 @allenporter @balloob