core icon indicating copy to clipboard operation
core copied to clipboard

Upgrade qBittorrent integration to show torrents

Open finder39 opened this issue 1 year ago • 17 comments

This brings the QqBittorrent integration to be more in line with the Transmission integration. It adds sensors for Active Torrents, Inactive Torrents, Paused Torrents, and All Torrents.

Breaking change Proposed change The QBIttorent integration only had sensors for Status, Download speed, and Upload Speed. This update adds sensors for Active Torrents, Inactive Torrents, Paused Torrents, All Torrents.

Breaking change

Proposed change

The QBIttorent integration only had sensors for Status, Download speed, and Upload Speed. This updates adds sensors for Active Torrents, Inactive Torrents, Paused Torrents, and All Torrents.

image

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [X] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [X] Code quality improvements to existing code or addition of tests

Additional information

  • Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/30348

Checklist

  • [X] The code change is tested and works locally.
  • [X] Local tests pass. Your PR cannot be merged unless tests pass
  • [X] There is no commented out code in this PR.
  • [X] I have followed the development checklist
  • [X] I have followed the perfect PR recommendations
  • [ ] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [ ] Tests have been added to verify that the new code works.

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

If the code communicates with devices, web services, or third-party tools:

  • [ ] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [ ] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • [ ] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

finder39 avatar Dec 15 '23 05:12 finder39

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Dec 15 '23 05:12 home-assistant[bot]

Hey there @geoffreylagaisse, mind taking a look at this pull request as it has been labeled with an integration (qbittorrent) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of qbittorrent can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign qbittorrent Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

home-assistant[bot] avatar Dec 15 '23 05:12 home-assistant[bot]

Awaiting a review

finder39 avatar Dec 18 '23 09:12 finder39

I'm asking around for your service response card problem, In the meantime

Thanks! Another possible option I came across (but can't find any current uses of) is _unrecorded_attributes, which could possibly be added like so (but like I said, can't find any current uses so not sure if it works):

class QBittorrentTorrentsSensor(QBittorrentSensor):
    """Representation of a qBittorrent torrents sensor."""

    _unrecorded_attributes = {STATE_ATTR_TORRENT_INFO}
...

finder39 avatar Dec 20 '23 08:12 finder39

Well the thing is, I'm expecting this to be a use case since we really want people to avoid dumping data in attributes.

joostlek avatar Dec 20 '23 08:12 joostlek

Totally understand that idea. I agree that just data dumping in attributes is bad but i think a system of "temporary storage" or the like has to be built out for handling lovelace cards. Data retention is one of the goals for some people with HA, but I know another goal people have is displaying current information about what is going on in their home or integrations).

So yeah, hopefully something exists already to use service responses in cards, but I couldn't find anything on it. Looking forward to hearing what people get back to you with info wise.

finder39 avatar Dec 20 '23 08:12 finder39

Do you have a link to the custom card?

joostlek avatar Dec 20 '23 09:12 joostlek

The one I made to work with this update isn't uploaded to github yet as i wanted to wait til this was merged in. It is a modified version of this card for transmission though, so that should be enough to go off of.

I can upload it shortly if you'd like, i'm working on changing my entities to work off of value_fn real quick :)

finder39 avatar Dec 20 '23 09:12 finder39

@joostlek I have changed this over to being a service with a response for the torrents. I still need to test if this works in a car as stated above, but this change should be more to your liking :)

Let me know any thoughts or comments you have.

finder39 avatar Dec 22 '23 23:12 finder39

Let's move the service to a new PR. Let's have this PR only for the extra sensors

joostlek avatar Dec 22 '23 23:12 joostlek

There are no new sensors here anymore, I have removed them and the torrent info is now only accessible via the service call, like you originally asked for.

So it should be ready to go as is :)

finder39 avatar Dec 22 '23 23:12 finder39

Though you may be right that it'd be worth still having new sensors to give the "count" of current total torrents as its state

finder39 avatar Dec 22 '23 23:12 finder39

Like I like both options and I'd maybe prefer the sensors first

joostlek avatar Dec 22 '23 23:12 joostlek

Let me add sensors for now that don't have the torrent list attributes (for database size concerns). So it'll just have a state with the count of torrents, then we can add the service for returning the list of torrents.

I can then attempt to build the custom card off the service and see if it works like we want it to.

How does that sound?

I've got a couple hours to get through this now and then I may not have time for quite a while. You by chance going to be around for a couple hours to help look and review? If not no worries and I understand

finder39 avatar Dec 22 '23 23:12 finder39

I mean you already have all the pieces.1 PR for the sensors, 1PR for the service call. I should be sleeping right now lol

joostlek avatar Dec 23 '23 00:12 joostlek

haha get some rest.

This should be review now once you're ready for it

finder39 avatar Dec 23 '23 00:12 finder39

I don't think I can do stacked pull requests here, so I'll have to wait til we get this merged before I can submit the other pull request

finder39 avatar Dec 23 '23 00:12 finder39