Opened 10 years ago

Closed 9 years ago

Last modified 3 years ago

#1858 closed patch (Fixed)

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

Reported by: eirikba Owned by: Cas
Priority: minor Milestone: 2.0.0
Component: Console UI Version: 1.3-stable
Keywords: Cc:

Description

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.

Change History (31)

comment:1 Changed 10 years ago by Cas

  • Component changed from other to console
  • Milestone changed from Future to 1.3.x
  • Owner set to Cas
  • Status changed from new to assigned

comment:2 Changed 10 years ago by 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.

comment:3 follow-up: Changed 10 years ago by eirikba

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

comment:4 in reply to: ↑ 3 Changed 10 years ago by 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.

comment:5 Changed 10 years ago by 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")

comment:6 Changed 10 years ago by 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...?

comment:7 Changed 10 years ago by Cas

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.

comment:8 Changed 10 years ago by 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...

comment:9 Changed 10 years ago by Cas

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.

comment:10 Changed 10 years ago by 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.

comment:11 Changed 10 years ago by 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.

comment:12 Changed 10 years ago by 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 ]

  

comment:13 Changed 10 years ago by 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?

comment:14 Changed 10 years ago by 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.

comment:15 Changed 10 years ago by 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?

comment:16 Changed 10 years ago by 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'.

comment:17 Changed 9 years ago by nogare

  • Summary changed from deluge-console: Add command to show and set per-torrent options to [console] Add command 'manage' to show/set per-torrent options

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

comment:18 Changed 9 years ago by 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.

comment:19 Changed 9 years ago by 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.

comment:20 Changed 9 years ago by 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

comment:21 Changed 9 years ago by 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?

comment:22 Changed 9 years ago by 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).

comment:23 Changed 9 years ago by eirikba

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

comment:24 Changed 9 years ago by Cas

  • Milestone changed from 1.3.x to 1.4.0
  • Resolution set to fixed
  • Status changed from assigned to closed

Now in git master.

comment:25 Changed 6 years ago by Cas

  • Milestone changed from 2.0.x to 2.0

comment:26 Changed 3 years ago by Cas

  • Milestone changed from 2.0 to 2.0.0

Milestone renamed

Note: See TracTickets for help on using tickets.