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

Expose 'manufacturer' and 'product_name' properties in 'Chromecast' class. #895

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Expose 'manufacturer' and 'product_name' properties in 'Chromecast' class. #895

wants to merge 1 commit into from
+31 −2

Conversation

pjgrochowski
Copy link

Expose 'manufacturer' and 'product_name' properties in 'Chromecast' class.

[Justification]:
Field 'model_name' evaluates to 'Chromecast' for all device models.
Field 'product_name' allows to actually distinguish between Chromecast models:

  • Chromecast 3 => 'caprica'
  • Chromecast Ultra => 'steak'
  • Chromecast Android TV => 'sabrina'

@pjgrochowski
Expose 'manufacturer' and 'product_name' properties in 'Chromecast' c…
7adc1c4 
…lass.

[Justification]:
Field 'model_name' evaluates to 'Chromecast' for all device models.
Field 'product_name' allows to actually distinguish between Chromecast models:
- Chromecast 3 => 'caprica'
- Chromecast Ultra => 'steak'
- Chromecast Android TV => 'sabrina'
elupus
elupus previously approved these changes Aug 29, 2024
View reviewed changes
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.

I think this make sense. Would map to new field in HA too.

emontnemery
emontnemery reviewed Aug 30, 2024
View reviewed changes
Copy link
Collaborator

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

I'm not fully sure about the benefit of this, why is it better to expose this in a property, instead of accessing via the public cast_info?

pychromecast/__init__.py
def manufacturer(self) -> str:
"""Returns the manufacturer of the Chromecast device."""
if TYPE_CHECKING:
# get_cast_type is guaranteed to return a CastInfo with a non-None model
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment is misleading

pychromecast/__init__.py
def product_name(self) -> str:
"""Returns the product name of the Chromecast device."""
if TYPE_CHECKING:
# get_cast_type is guaranteed to return a CastInfo with a non-None model
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same

elupus
elupus reviewed Aug 30, 2024
View reviewed changes
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.

Hmm. Didn't realize cast_info was public already. Seem pointless yes. One can wonder why we expose the others then.

@elupus elupus dismissed their stale review August 30, 2024 21:08

No longer valid

elupus
elupus requested changes Aug 30, 2024
View reviewed changes
Copy link
Collaborator

@elupus elupus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Missing addition of the new field here:
https://github.com/home-assistant-libs/pychromecast/pull/895/files#diff-c7c448ef8026704bcb4f958857c133335170bbeda706cea0bfeaa033ad829a7dR265
and
https://github.com/home-assistant-libs/pychromecast/pull/895/files#diff-c7c448ef8026704bcb4f958857c133335170bbeda706cea0bfeaa033ad829a7dR251
and
dial.py

@emontnemery
Copy link
Collaborator

Hmm. Didn't realize cast_info was public already. Seem pointless yes. One can wonder why we expose the others then.

It's due to some old design choice to forward selected attributes and methods from sub-objects to the Pychromecast object.

See for example here:

pychromecast/pychromecast/__init__.py

Lines 344 to 356 in 4917898

# Forward these methods
self.set_volume = receiver_controller.set_volume
self.set_volume_muted = receiver_controller.set_volume_muted
self.play_media = self.socket_client.media_controller.play_media
self.register_handler = self.socket_client.register_handler
self.unregister_handler = self.socket_client.unregister_handler
self.register_status_listener = receiver_controller.register_status_listener
self.register_launch_error_listener = (
receiver_controller.register_launch_error_listener
)
self.register_connection_listener = (
self.socket_client.register_connection_listener
)

It's currently applied very inconsistently; new functionality has not been forwarded like this for several years.

Notice how media_controller.play_media is forwarded, but no other methods of it are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emontnemery emontnemery emontnemery left review comments

@elupus elupus elupus requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pjgrochowski @emontnemery @elupus