[console] Add command 'manage' to show/set per-torrent options

[eirikba]

This patch adds a command to deluge-console to show and set per-torrent options. I have called the command "torrent_option" and mostly modeled it on the "config" command. A better name for the command would be nice, but I have no better suggestion at this time.

[nogare]

What is the opinion on naming this command manage?

The code needs a quick dusting, and the addition of tab completion on the torrent ID(critical).The adding support for a torrent name isn't critical, but would be nice. Over all, this could be merged into *Master* and *1.3-stable* without much hassle.

[eirikba]

"manage" sounds good to me. I'll see if I can get around to whip up an updated patch.

[nogare]

Replying to eirikba:

"manage" sounds good to me. I'll see if I can get around to whip up an updated patch.

That would be great. I imagine taking a look in info.py, should illustrate how to implement tab completion.

[eirikba]

Here's an updated version. Please criticize :)

I've done some minor clean-up and I've added support for multiple torrent ids/names, as well as interpreting no names as "all torrents" for get (but not for --set, obviously). Did I miss anything?

Would you believe the file started out as a pure copy of config.py? (I've also fixed the comment at the top of the file. It now says "manage.py" instead of "config.py")

[nogare]

:) I looked at it quickly, and tested just as quick. But one thing I noticed is that when you run help manage or manage --help, it doesn't print the possible keys which you can set. So to know which options I can change, I have to run manage torrentX then manage -s *option* torrentX. While that isn't too much of a hassle, it would make sense to briefly list the keys you can set when you run manage --help.

Does that make sense...?

[Calum]

I think there should be an option to set all torrents but you should have a prompt to ask the user before setting.

Can you test all the options as is_auto_managed does not work.

The private flag is not an really an option as it cannot be set so shouldn't be included. Might be worth creating a ticket to add this to verbose info.

[nogare]

Test results:

Fails to set:

• move_on_completed_path
• is_auto_managed [as Cas said]
• prioritize_first_last [I tried on a seeding torrent first, then on a download }}}

All else works;

I don't think that when manage is called alltorrents should be listed. Let's say a *user* has a couple thousand torrents and does a typo, that would be a real pain to sit and watch 2k torrents print their options... instead manage * should print all torrents and corresponding values. Also having manage -s move_on_completed true * is a duplication of config -s move_on_completed true.. is it not? As is setting stop_ratio, remove_at_ratio, stop_at_ratio, each is a duplication of functionality that config offers...

[Calum]

The torrent names rather than ids would be better in success output for the user:

-        self.console.write('Setting %s to %s for torrents %s...' % (key, val, torrent_ids))
+        self.console.write('Setting %s to %s for torrent(s): %s' % (key, val,
+            ', '.join(self.console.get_torrent_name(ids) for ids in torrent_ids)))

I think nogare is correct in that using * to explicitly set all torrents makes more sense but it may not be a crucially needed addition so could maybe just leave it.

I discussed with nogare in irc about the distinction between config and manage setting all torrents.

Using * to list all torrents does make sense but it would need to documented and the same change should be applied to other commands such as info.

[eirikba]

Listing valid options in --help sounds like a good idea.

Displaying torrent names rather than ids, likewise.

Displaying all by default is something that I copied from 'info'. Requiring being explicit about it is probably better. If we agree that '*' is a good idea for general use, I can implement that here and SomeOne can fix the other commands later. (I might make a patch for that afterwards. Who knows.)

Setting all makes me a bit uneasy, but being explicit and requiring confirmation would probably be ok. I'll look into it.

I didn't really test many of the options. There's not that many that I myself generally want to modify. I think I just grabbed the list from what the implementation of set_torrent_option() seemed to accept. But I see the list in Torrent.set_options() is different than what I've got. (e.g. "is_auto_managed" seems to be called "auto_managed" now). I'll update the list. (Or maybe I pulled the names from the getter rather than the setter. Hmm, I must remember to double-check that...)

Whether 'manage * --set' is equivalent to 'config -s' I have absolutely no idea. I would expect 'manage' to set it for all current torrents but not for any future torrents. While 'config' would set it for all future torrents, but I don't know if I'd expect it to set it for the current torrents. But even if it is equivalent, I don't see the harm. I would think it more harmful if a straightforward, natural command doesn't work just because you should have used a different command instead.

[eirikba]

I've made a quick investigation, and all these options seems to be more-or-less settable. The ones that aren't commented out looks like they probably have a predictable effect when being set:

set_torrent_options = {
    # These are handled by torrent.py's Torrent.set_options()
    'auto_managed': bool,
    #'download_location': str, # Probably not useful to set
    #'file_priorities': ???, # Not a simple value to set, probably needs its own command
    'max_connections': int,
    'max_download_speed': float,
    'max_upload_slots': int,
    'max_upload_speed': float,
    'prioritize_first_last_pieces': bool, # Only has effect if torrent contains a single file

    # These are "handled" by being set directly on the options dict
    'stop_at_ratio': bool,
    'stop_ratio': float,
    'remove_at_ratio': bool,
    'move_completed': bool,
    'move_completed_path': str,

    #'compact_allocation': bool, # Unclear whether this has any effect
    #'add_paused': ???, # Not returned by get_status, unclear what the effect would be
    #'mapped_files': ??? # Not returned by get_status, unclear what the effect would be
    }

And the getter renames some of the values, like this:

get_torrent_options = {
    'max_download_speed': 'max_download_speed',
    'max_upload_speed': 'max_upload_speed',
    'max_connections': 'max_connections',
    'max_upload_slots': 'max_upload_slots',
    'prioritize_first_last': 'prioritize_first_last_pieces',
    'is_auto_managed': 'auto_managed',
    'stop_at_ratio': 'stop_at_ratio',
    'stop_ratio': 'stop_ratio',
    'remove_at_ratio': 'remove_at_ratio',
    'move_on_completed': 'move_completed',
    'move_on_completed_path': 'move_completed_path',
    #'file_priorities': 'file_priorities',
    #'compact': 'compact_allocation',
    #'save_path': 'download_location'
    }

I think I'll accept all of these, and I think I'll have the command only accept (and display) the setter's names.

[nogare]

'prioritize_first_last_pieces': bool, # Only has effect if torrent contains a single file
}}}[[BR]]


I tried to set it on something that has multiple files(dumb me), I will test --set(ting) on a single file torrent . When a user runs `manage -s prioritize_first_last_pieces` on a torrent which contains multiple files, a error message should be printed informing the user that it can only be set on single file torrents.[[BR]]


Also if you're trying to figure out implementation for '*', check resume.py [[BR]]

Somehow `resume` currently supports '*'[as Cas said in IRC, it is the black sheep :P ]

  

[eirikba]

Yes, helpful error messages are good :)

It took me half an hour to figure out that Screen.add_line() requires the line to start with a colour specification. When splitting the line (because it may be too wide) it will drop all text before the first '{!'. I expect this is a bug?

[eirikba]

I think this version covers all changes except for requesting confirmation on setting for all torrents. Any hints on how to do that properly?

On second thoughts, I think I forgot to check for single-file torrents on setting prioritize_first_last_pieces.

[eirikba]

Looking at the file I notice there are some strange uses of Deferred. I know very little about twisted, but after a quick look at the documentation of Deferred, I've come to the conclusion that this code probably doesn't do anything useful. So I'll delete all uses of Deferred from manage.py.

I believe this code was something I copied from config.py, so I went back and had a look at that file. And it seems to me that all use of Deferred in config.py is also entirely pointless. Am I missing something?

[eirikba]

That adds some checking for prioritize_first_last_pieces (and a little bit of cleanup).

I think the only thing missing now is confirmation request for 'manage * --set'.

[nogare]

Eirikba: I talked to Cas on IRC regarding adding the prompt, and he said that if we are using '*' to specify "all loaded torrents", then a prompt of confirmation isn't needed.

I've just started testing your code, and have some ideas on how to improve --help

                                                               
>>> manage --help                                                                                                                                     
Usage: manage <torrent-id> [<torrent-id> ...] [<key1> [<key2> ...]]                                                                                   
       manage <torrent-id> [<torrent-id> ...] --set <key> <value>                                                                                     
                                                                                                                                                      
       torrent-id can be "*" to signify all loaded torrents'       
       max_connections, max_download_speed, max_upload_slots, max_upload_speed can be set to -1 to signify unlimited                                                                          
                                                        
Available keys:                                                                                              
       auto_managed: Makes torrent obey deluge's queue settings.(see FAQ for details)                                                                                  
       max_connections:  Maximum number of connections to use for this torrent                                                                        
       max_download_speed:  Maximum total download speed to allow this torrent to use (KiB/s)                                                         
       max_upload_slots:  Maximum number of connections to use for uploading for this torrent                                                         
       max_upload_speed:  Maximum total upload speed to allow this torrent to use (KiB/s)                                                             
       move_completed:  Whether to move the downloaded data when downloading is complete.                                                             
       move_completed_path:  Where to move completed data to (if "move_completed" is True)                                                            
       remove_at_ratio:  Whether to remove torrent when share ratio reaches "stop_ratio".                                                             
       stop_at_ratio:  Whether to stop seeding when share ratio reaches "stop_ratio".                                                                 
       stop_ratio:  The ratio at which to stop seeding (if "stop_at_ratio" is True)  

the above is suggested new output, you can run it(above) and current help through ​http://www.quickdiff.com/ to see changes

Also a quick comment on your code:

11:28      nogare| There is no need to have options which cannot be set commented out e.g the following comments
 11:28      nogare| #'compact_allocation': bool, # Unclear what setting this would do 
 11:29      nogare| #'add_paused': ???, # Not returned by get_status, unclear what setting it would do 
 11:29      nogare| etc..

I'll post when i'm done testing

[eirikba]

... if we are using '*' to specify "all loaded torrents", then a prompt of confirmation isn't needed.

Yes, I've been leaning towards that myself. Besides, it is much easier to implement :)

Thanks for input on the help text. Looks sensible. Maybe "Use -1 to set maximum limits to unlimited (max_connections, ....)"? Or "... to disable maximum limits"? I'll try some variations and see what I like best.

There is no need to have options which cannot be set commented out

I'm a bit on the fence on this one. On the one hand it doesn't do anything directly useful, but on the other hand it documents which values have been considered and why they were rejected. Though if I'm to leave them in, I should probably improve the comments a bit.

[eirikba]

I see you removed the help text for prioritize_first_last_pieces, by the way. Was that intentional?

By the way, that setting has another quirk, I believe: you can only set it, not clear it. Also, I think it will actually reset the priority of all pieces except the first and the last.

[nogare]

What about "for all keys which take a integer, -1 can be used to signify unlimited"?

Removing help text for text for prioritize_first_last_pieces, wasn't intentional... TBH I don't recall it being printed when I ran manage --help

[eirikba]

Of course! That would be ​http://dev.deluge-torrent.org/ticket/2062 (all text before the first "{!" is discarded when line-wrapping)

Hmm, this is my best so far:

Use the torrent-id "*" to mean "all loaded torrents". Use the value -1 to mean unlimited (for max_connections, max_download_speed, max_upload_slots and max_upload_speed).

Yes, I'm trying to avoid words like "signify". They just don't sound good to me. I'm not sure about "loaded" either. Does that actually improve the sentence?

[eirikba]

Or even:

The torrent-id * (a single asterisk) means "all loaded torrents".

The value -1 means unlimited (for max_connections, max_download_speed, max_upload_slots and max_upload_speed).

[eirikba]

Only improvements to the help text in this version (I believe).

[Calum]

Now in git master.

[Calum]

Milestone renamed