Skip to content

Add transmission info about torrents that is accessible with templating#27111

Merged
MartinHjelmare merged 17 commits into
home-assistant:devfrom
JPHutchins:tm_add_torrent_list
Oct 28, 2019
Merged

Add transmission info about torrents that is accessible with templating#27111
MartinHjelmare merged 17 commits into
home-assistant:devfrom
JPHutchins:tm_add_torrent_list

Conversation

@JPHutchins
Copy link
Copy Markdown
Contributor

@JPHutchins JPHutchins commented Oct 2, 2019

Description:

Added is a new sensor that contains a dictionary of "started torrents" in its state attribute. The inspiration comes from the add_torrent service implemented with an input_text entity. Ultimately I believe Home Assistant can act as a simple interface for adding, sorting, and observing torrent progress, very useful to friends and family that have a hard time managing their Linux ISOs in the regular interface.

This example is just to see if there is interest. I am trying to learn HA architecture while improving my programming ability. To this end I would like to know if a better implementation would be for torrents to be registered as entities (like how discovery or device tracker does)?

Example state attribute:

Here is what you can expect in Developer Tools->States->sensor.transmission_started_torrents->Attributes.

torrent_info: {
  "torrent_file_1_as_added_to_transmission": {
    "added_date": 1569812023,
    "percent_done": "5.83",
    "eta": "02:04:30"
  },
  "torrent_file_2_as_added_to_transmission": {
    "added_date": 1569812048,
    "percent_done": "68.03",
    "eta": "00:05:14"
  }
}

example torrent list

Example templating:

Add a Markdown card like this one. This took me forever Jinja and I do not understand one another.

content: >
  {% set payload = state_attr('sensor.transmission_started_torrents', 'torrent_info') %}

  {% for torrent in payload.items() %} {% set name = torrent[0] %} {% set data = torrent[1] %}
  
  {{ name|truncate(20) }} is {{ data.percent_done }}% complete, {{ data.eta }} remaining {% endfor %}
type: markdown

templating

Clicking on the started_torrents sensor for "more info" yields inadequate results - can this be templated?
more info

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the [development checklist][dev-checklist]

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

Cheers,
J.P.

@JPHutchins JPHutchins changed the title Add info about torrents accessible with templating Add info about torrents that is accessible with templating Oct 2, 2019
@frenck
Copy link
Copy Markdown
Member

frenck commented Oct 2, 2019

Thank you for your contribution thus far! 🎖 Since this is a significant contribution, we would appreciate you'd added yourself to the list of code owners for this integration. ❤️

Please, add your GitHub username to the manifest.json of this integration.

For more information about "code owners", see: Architecture Decision Record 0008: Code owners.

Comment thread homeassistant/components/transmission/sensor.py Outdated
Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread homeassistant/components/transmission/__init__.py Outdated
Comment thread homeassistant/components/transmission/sensor.py Outdated
Comment thread homeassistant/components/transmission/sensor.py Outdated
@JPHutchins
Copy link
Copy Markdown
Contributor Author

Documentation has been updated, not sure how to remove the docs-missing tag. Thanks!
home-assistant/home-assistant.io#10574

@MartinHjelmare
Copy link
Copy Markdown
Member

Thanks!

@engrbm87 ok?

@engrbm87
Copy link
Copy Markdown
Contributor

engrbm87 commented Oct 6, 2019

I suggest that we use the already existing started_torrents sensor and assign the torrent_info to its attribute instead of creating a new sensor. If the sensor state is > 0 then the attributes will show the info about the started torrents.
Same can be done for completed_torrents.
@MartinHjelmare One concern is that if there are a lot of started torrents (downloading or pending download) will that be a problem to have an attribute with a lot of data?

@MartinHjelmare
Copy link
Copy Markdown
Member

Generally we prefer individual sensors over many sensor attributes.

I don't known what's best in this case.

@JPHutchins
Copy link
Copy Markdown
Contributor Author

Ultimately I intended to use the date_added attribute to truncate the dictionary of started torrents to the five (or some number) most recent. The most important thing is that the user sees quick feedback when they call the add_torrent service - new torrent appears at the top of the list with ETA - and I love the idea of seeing a few of the most recent completions as well.

OK to change to started_torrents attr.

@engrbm87 You have also added a config flow that eliminates the need for the monitored_conditions: entry in the configuration.yaml? I would love to learn how to dynamically create an entity for each torrent. Ultimately that would make it easier for the end user to make a GUI. We would definitely want to carefully truncate the number of torrents that could be registered in this way. Thanks for your help!

@engrbm87
Copy link
Copy Markdown
Contributor

engrbm87 commented Oct 7, 2019

@JPHutchins It will be nice to see the torrent details in the GUI especially for torrents being downloaded. I don't think it is a good idea to create a new entity for each torrent. We should be in control of which entities are added to HA.
If you can manage to show the torrent info for the torrents that are downloading only as put them as an attribute for the started_torrents sensor that would be good as the torrents being downloaded are usually limited.
It would be good idea to test adding 5 to 7 torrents and see how the attribute looks in the more-info dialog of the sensor.

@JPHutchins
Copy link
Copy Markdown
Contributor Author

@engrbm87 I have made a small update and added photos. Obviously this needs further thought. Sorry if I missed any leftovers in the code, I will continue tomorrow. For some reason my development hass kept loading an old IP and Port for transmission (I wanted to setup a phony transmission for testing) even though hass was otherwise parsing the configuration.yaml. It did not even respect a complete removal of the transmission component from the yaml. Does this have to do with the new config flow?

frenck pushed a commit to JPHutchins/home-assistant.io that referenced this pull request Oct 10, 2019
@JPHutchins
Copy link
Copy Markdown
Contributor Author

The trouble I was having did have to do with the new config flow. An edit of the transmission entry in the configuration.yaml would not have any effect on the integration - it was necessary to delete transmission from the integrations frontend and then re-add. Registering was smooth, good work on that config flow!

@MartinHjelmare
Copy link
Copy Markdown
Member

@engrbm87 please approve when this is ready and I'll merge.

@engrbm87
Copy link
Copy Markdown
Contributor

Hi @MartinHjelmare,
I would like to get PR #28136 approved and merged first then @JPHutchins can update his code as discussed to get this merged as well.

@JPHutchins
Copy link
Copy Markdown
Contributor Author

I don't think there should be any need to make changes since PR #28136. I'd like to get it out in this form, displaying a dictionary of the downloading torrents as a state_attr of the started_torrents sensor, so that hopefully a front end person will be inspired.

@engrbm87
Copy link
Copy Markdown
Contributor

Hello @JPHutchins ,
The new PR has some changes so you will need to rebase your branch and apply your commits again.

@JPHutchins
Copy link
Copy Markdown
Contributor Author

@engrbm87 Thank you for your patience and help, I am learning so much!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please explain the need for this? in the SENSOR_TYPES you specified all as None.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is removed as it was leftover from the original torrent_info sensor.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can directly use self._transmission_api.started_torrent_dict without the need to store it in self._torrent_info

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have removed this - now when the sensor started_torrents is updated it checks for torrent info:

@property
    def device_state_attributes(self):
        """Return the state attributes, if any."""
        if self._tm_client.api.started_torrent_dict and self.type == "started_torrents":
            return {STATE_ATTR_TORRENT_INFO: self._tm_client.api.started_torrent_dict}
        return None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You are only using the downloading status. I recommend removing _TRPC and use downloading directly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have removed these - if transmissionrpc ever changed their codes they can be found here: https://bitbucket.org/blueluna/transmissionrpc/src/default/transmissionrpc/torrent.py

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I recommend removing this as it would increase the list of torrents in the attribute if the user has many pending torrents. We should only focus on torrents being downloaded.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed!

"""Get the number of completed torrents."""
return len(self.completed_torrents)

def get_started_torrent_info(self):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unused method. Either remove it or use it in the sensor attributes check.

Old method to display boolean for the removed torrent_info sensor.
@engrbm87
Copy link
Copy Markdown
Contributor

Great @JPHutchins
Hello @MartinHjelmare the code changes are approved.

Copy link
Copy Markdown
Contributor

@engrbm87 engrbm87 left a comment

Choose a reason for hiding this comment

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

Approved

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks!

@MartinHjelmare MartinHjelmare changed the title Add info about torrents that is accessible with templating Add transmission info about torrents that is accessible with templating Oct 28, 2019
@MartinHjelmare MartinHjelmare merged commit 54342d2 into home-assistant:dev Oct 28, 2019
@lock lock Bot locked and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants