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

Ability to disable media browser #262

Merged
merged 19 commits into from May 28, 2022

Conversation

NickM-27
Copy link
Sponsor Collaborator

Fix for #218

@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #262 (f926c2a) into master (6df9ee5) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #262   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines         1438      1445    +7     
=========================================
+ Hits          1438      1445    +7     
Impacted Files Coverage Δ
custom_components/frigate/config_flow.py 100.00% <ø> (ø)
custom_components/frigate/const.py 100.00% <100.00%> (ø)
custom_components/frigate/media_source.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6df9ee5...f926c2a. Read the comment docs.

@NickM-27
Copy link
Sponsor Collaborator Author

Screen Shot 2022-05-23 at 11 53 06 AM

@dermotduffy dermotduffy added the enhancement New feature or request label May 24, 2022
NickM-27 and others added 3 commits May 23, 2022 21:39
@@ -47,6 +47,11 @@

async def async_get_media_source(hass: HomeAssistant) -> MediaSource:
"""Set up Frigate media source."""

for config_entry in hass.config_entries.async_entries(DOMAIN):
Copy link
Collaborator

Choose a reason for hiding this comment

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

[With apologies in advance for re-thinking this] I think I have a much better idea. Leave this method the way it used to be, and check the option in FrigateMediaSource.

Suggestion on how to do this (pardon the verbosity as this file is quite complex). You'll need to add a new method such as _isAllowedAsMediaSource that takes a frigate_instance_id and returns True if for that instance_id, in the config entry (which can you get by calling get_config_entry_for_frigate_instance_id) the option is set correctly.

Then call that verification method in these places (in all cases you can get the frigate_instance_id from the identifer as identifier.frigate_instance_id).

  • async_resolve_media [here]
  • async_browse_media [here]
  • async_browse_media [here]

This is much better because if I have 3 Frigate instances, and I disable the media for 1 of them, media will still work as usual for the other 2 rather than 'all or nothing'.

Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

No problem, that's a much better solution, will jump on that tomorrow.

The test will probably need to be tweaked as well

@NickM-27
Copy link
Sponsor Collaborator Author

Not sure why HACS is failing. I am seeing that on my integration as well but unfortunately not seeing much help with what changes are actually needed.

@dermotduffy
Copy link
Collaborator

Not sure. This in the build logs (but you didn't change the manifest):

  Error:  <Validation hacs_manifest> failed:  extra keys not allowed @ data['domains']
  Error:  <Integration blakeblackshear/frigate-hass-integration> 1/5 checks failed
  Error: HACS load check

What happens when you load the integration?

@Vaskivskyi
Copy link
Contributor

Not sure why HACS is failing. I am seeing that on my integration as well but unfortunately not seeing much help with what changes are actually needed.

This happens to all the integration, that are still using hacs.json values, which are not supported anymore. Not connected to this PR, but to the HACS itself. #263 will fix it (checked on my HA integration recently)

@NickM-27
Copy link
Sponsor Collaborator Author

Seems the check is further up stream and the lower stream exception doesn't get tested, not really sure of a good way to test that unless it is called directly

@NickM-27 NickM-27 requested a review from dermotduffy May 28, 2022 21:39
@NickM-27
Copy link
Sponsor Collaborator Author

@dermotduffy Thank you!

@dermotduffy dermotduffy merged commit 37489e2 into blakeblackshear:master May 28, 2022
@NickM-27 NickM-27 deleted the disable-media-browser branch August 13, 2023 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants