stig icon indicating copy to clipboard operation
stig copied to clipboard

Add a command `fileopen` to open files in external programs

Open rsekman opened this issue 2 years ago • 3 comments

rsekman avatar Jul 29 '21 00:07 rsekman

Thank you. I've had this on my todo list aswell.

  1. Please squash your fixup commit so it is easier for me to review.

  2. "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?

  3. 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.

  4. 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.

  5. 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

rndusr avatar Aug 01 '21 10:08 rndusr

Hi, thanks,

  1. Fixed.
  2. When running 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.
  3. Fixed.
  4. Yes -- my use case was opening files in 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.)
  5. Naturally -- is there documentation for writing tests? I'd need a mock transmission right?

rsekman avatar Aug 09 '21 15:08 rsekman

  1. 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

  2. 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?

  3. 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.

  4. 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. :)

rndusr avatar Aug 09 '21 17:08 rndusr