search

savdagod / TidbytAssistant

Send custom apps, notifications or plain text to a Tidbyt device from HomeAssistant!
MIT License
33 stars 2 forks source link

Pass `lang` parameter only for built-in content #12

Closed IngmarStein closed 1 month ago

IngmarStein commented 1 month ago

The. description says "Language to display built-in content", but the lang parameter is actually also set for custom content. I think we shouldn't set the lang parameter for custom content which may support more than the two languages provided by the built-in apps.

I'm running a custom app which happens to use the lang parameter and I've been passing this as part of the arguments list and was wondering why it wasn't passed to the add-on: it's because I didn't check the Language checkbox which then causes the lang parameter to be set to an empty value.

savdagod commented 1 month ago

Ah yeah that makes sense, it’s an easy fix.

I keep wondering if now it would be better to split the services between built-in and custom rather than Push and Publish. To me it now makes more sense to group the services by the parameters they use (ie only built-in params will be in the built in service, same with custom). Problem with that is it’s going to break everyone’s automations.

Edit: could also just consolidate them into one service and add content Id. If the user enters a content id, then it will publish.

savdagod commented 1 month ago

Fixed in 1.0.12

IngmarStein commented 1 month ago

Yes, the distinction between "push" and "publish" isn't very useful. We can consolidate that also on the server side and only keep "publish" which is a strict superset in terms of functionality to "push".

savdagod commented 1 month ago

Yeah, given its pixlet push <app> I will remove the publish service and consolidate everything into the push service. That way, users will only have to edit their automations that use publish to push, which should be as easy as just changing the name as the variables will be the same.

IngmarStein commented 1 month ago

"Push" makes more sense than "publish" in the user-facing component. Should we require another mandatory add-on update and rename the /tidbyt-publish endpoint to /push? This would only be to align terminology with the integration.

And while we're at it: the integration could use the /push endpoint for the text apps as well, so that this endpoint could also be removed.