-
-
Notifications
You must be signed in to change notification settings - Fork 226
Fallback smart device alias to model when nickname is empty #1521
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
base: master
Are you sure you want to change the base?
Fallback smart device alias to model when nickname is empty #1521
Conversation
Link to the original comments on the home-assistant/core PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should improve this by using device type based user-friendly strings, e.g., "Power strip" for DeviceType.Strip
etc. to help users who may have many of such devices, making it hard to distinguish among them.
Maybe we should additionally use some device-specific information (Mac address? Device id?) as a part of this generic alias. As this is only used when the device has no name, we can be verbose and rather relaxed about this.
What do you think, @sdb9696 ?
385a798
to
5036e7d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1521 +/- ##
=======================================
Coverage 92.66% 92.66%
=======================================
Files 150 150
Lines 9538 9541 +3
Branches 974 975 +1
=======================================
+ Hits 8838 8841 +3
Misses 499 499
Partials 201 201 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@rytilahti that sounds even better. I copied the The result with the P304M is the name becomes I believe adding the mac address or device id would not be ideal, as you would be stuck with long "ugly" ids. Out of curiosity: what happens if a user adds another identical power strip, or two identical power plugs? What's added to the ids to keep them unique? I don't have duplicates to test this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for taking the time to respond, I added a couple of comments but this is looking good with a couple of minor nits! 👍
kasa/smart/smartdevice.py
Outdated
@@ -598,6 +616,8 @@ def alias(self) -> str | None: | |||
"""Returns the device alias or nickname.""" | |||
if self._info and (nickname := self._info.get("nickname")): | |||
return base64.b64decode(nickname).decode() | |||
elif label := DEVICE_TYPE_TO_LABEL.get(self._device_type): | |||
return label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return label | |
return f"Unnamed {label} ({self.model} {self.device_id})" |
How about something like this? As this is just a fallback when there is no name set, I think it makes sense to give the user as much information about the device as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great thank you! I fixed the other nits too, take a look whenever you get a chance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually wait, I did push this but I believe this is exactly what causes the problem I was trying to fix.
The while string, including the Unnamed
part, will be used as the device id as well as the beginning of all the connected entities' ids. It's a pain to rename all the ids to something more meaningful. And not renaming the ids is annoying when working with manual templates.
Since the power strip is unnamable on the Tapo app, what I'm trying to achieve is an acceptable default id so we don't have to rename all the entity ids. Can we use a simpler string that would be fine as a default device/entity id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, sorry, indeed... I'm starting to wonder if it would be better to fix this somehow inside the homeassistant integration instead when a None
is encountered? For the cli tool, we can do a pretty-printing otherwise. Would that make sense to you? This will also allow adapting the naming scheme in case homeassistant changes its scheme at some point.
5036e7d
to
164e37a
Compare
tests/smart/test_smartdevice.py
Outdated
@@ -90,7 +94,7 @@ async def test_device_type_no_update(discovery_mock, caplog: pytest.LogCaptureFi | |||
assert dev.device_type is DeviceType.Plug | |||
assert ( | |||
repr(dev) | |||
== f"<DeviceType.Plug at {DISCOVERY_MOCK_IP} - None ({short_model}) - update() needed>" | |||
== f"<DeviceType.Plug at {DISCOVERY_MOCK_IP} - Plug ({short_model} {disco_id}) ({short_model}) - update() needed>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rytilahti I removed the Unnamed
part from your suggested string, but I still see that the returned string doesn't match with the test unless I add ({short_model})
again, so maybe it's not needed on line 620 of kasa/smart/smartdevice.py because it's appended at a later step anyway?
In kasa/smart/smartdevice.py, how about we just do return f"{label} ({self.device_id})"
?
Fallback smart device alias to
model
ifnickname
is empty.This will apply to any device, not just the P304M smart strip. If we prefer to have this exception just for the P304M I can add
and model == "P304M"
in the elif on line 601.The end result:

Notice that the name will be

P304M P304M
, however this can easily be edited in the UI:Entity ids look great:

Let me know if this solution is good or you would like to tweak it :)
EDIT: I see tests need fixing, I'll wait on your decision between having this fallback globally or only for P304M before fixing them.