Open rsekman opened 3 years ago
Hi, thanks,
fopen mpv
in the tui with files selected the intuitively expected behaviour is that the files are opened withmpv
. This means that the two first arguments as the command is defined for the cli need to be filled in, so fopen mpv
should "alias" to ~fopen torrentid=x file=movie.mkv mpv
. Maybe it is better to have two base command classes? I am not super familiar with the structure of this code base yet so I don't know if that's possible or good.mpv
so I did not want the logging input in stig
. I'll look into your suggestion. Perhaps it would also be good to have a --quiet
flag for fopen
, but then we'd need a way to separate it from arguments to passed through (e.g. one could call fopen --quiet mpv --fullscreen
and this would open the file fullscreened in mpv
while suppressing all output.)transmission
right?I have some minor style complaints (read the next point first):
@.: Missing empty line above the method @.: Missing empty line above the method @.: Missing empty line above the method @.: Missing empty line below the method @.: Surplus empty line @.: Missing two empty lines above the class
It's been a long time since I've worked on this. After reading my own code and docs for a while, I think my plan for this was to make a stupid "run" command that just executes a command and logs the output. The "interactive" command should be able to supply the file/torrent path to "run". Something like this:
:bind --context file interactive "run --quiet mpv -f '{location}/{path}'" :bind --context torrent interactive "run --quiet mpv -f '{location}'"
This approach should drastically reduce the added code. The downside is that this doesn't work on the CLI. Is "stig fopen ..." important to you?
I agree that a --quiet/-q option would be best to ignore stdout. Maybe count the number of --quiet arguments and also silence stderr if >= 2.
Check out the "bind" command, which also accepts options and a command.
The tests were written in the Wild West. No docs or even a guide. You can look at existing tests for inspiration, but don't look too closely, I didn't really know what I was doing. :)
Thank you. I've had this on my todo list aswell.
Please squash your fixup commit so it is easier for me to review.
"Ugly hack to make the command behave as expected in the tui" doesn't tell me much. Can you explain what this hack does exactly and why it is needed?
It doesn't look like you need mixin.polling_frenzy. This is only needed if you make a request that changes state in transmission-daemon (e.g. stop a torrent) and you want to display that change ASAP in the TUI.
I haven't tried, but it looks like a failing command doesn't tell me what went wrong, and a command that only prints to stdout does nothing.
Maybe you can subclass something like TextIOWrapper[1] and delegate write() calls to self.info() for stdout and self.error() for stderr.
Once I'm happy with the PR, tests would be nice if you have the time.
[1] https://docs.python.org/3/library/io.html#io.TextIOWrapper