Opened 11 years ago
Last modified 3 months ago
#2411 new feature-request
Open Folder action to remote client
Reported by: | eerorika | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | 2.x |
Component: | GTK UI | Version: | 1.3-stable |
Keywords: | remote, open folder | Cc: |
Description
Currently, in remote mode, deluge does not show the "Open Folder" menu button. It would be nice to be able to open shared folders where deluge has downloaded torrents using this button.
In case the shared folder is mounted in a different path than it is on remote server, this also requires a setting form path mapping between remote and local.
I've created a simple implementation for this feature in the attached patch.
Attachments (4)
Change History (18)
comment:1 by , 11 years ago
by , 11 years ago
Attachment: | remote-path.patch added |
---|
by , 11 years ago
Attachment: | path-mapping.patch added |
---|
comment:2 by , 11 years ago
That is way more complicated than it needs to be, path_mapping
should be a dictionary of paths.
by , 11 years ago
Attachment: | path-mapping-dict.patch added |
---|
comment:3 by , 11 years ago
I made a dict version. Though I find that
/path/on/server=/local/path /another/remote/path=/another/local/path
is easier for users to understand than
{'/path/on/server': '/local/path', '/another/remote/path': '/another/local/path'}
comment:5 by , 11 years ago
That, on the other hand, will be way more complicated than just splitting the lines when files are opened. At least I think so; I've never done gtk stuff besides the text area in the patch.
comment:6 by , 11 years ago
There are examples in the code of using a popup to add two strings, e.g. Add host in connection manager.
Avoid regex where ever possible as it's an expensive call. You should be able to do this in situ without a new common.py function:
if client.is_localhost: ... elif self.gtkui_config["pathmapping"]: try: deluge.common.open_file(path_mapping[save_path]) except KeyError: log.warning("No match found")
One caveat is to ensure no trailing slashes when storing the paths and strip them from save_path before test.
comment:7 by , 11 years ago
You should probably also be using a proper gtk list view with columns (Local Path, Remote Path) to display the configuration, and as Cas mentions maybe some pop-up windows for adding entries. Additionally, some way of removing entries from the list will need to be available as well.
The look should follow the same feel that we have in our Preferences dialog and other places in Deluge.
It seems that the 'pathmapping' configuration should probably be saved with the host in the connection managers config, not the gtkui config as you could have different configurations based on which Deluge daemon you are connecting to.
comment:8 by , 11 years ago
@Cas The mappings are supposed to work with all the subfolders of remote path. For example, with mapping: '/home/deluge': '/mnt/server/deluge'
A folders path should be mapped: /home/deluge/distros/ubuntu/ => /mnt/server/deluge/distros/ubuntu/
I don't think that's possible with your code unless the user maps all the possible subfolders manually, which would be silly. I didn't edit the description or mention it in comments, but I also do the mapping for "Open File" button. Mapping all downloaded files manually would be even sillier :)
There might be a cheaper way than the regex but it won't be as elegant.
In any case, I doubt the regex is going to have much of an impact. It's very simple (no backtracking or wildcards) and there isn't likely going to be more than a few mappings; usually just one. I'd be surprised if it were significantly expensive unless you open thousands of folders at the same time. And even then, the regex would be much less expensive than opening thousands instances of file managers that access a remote filesystem.
@andar Yeah, the right way would be to have a mapping dict in client for each host. A dict of dicts that is.
The mapping could be added to the "Add Host" dialog but it seems that currently you cannot edit the host settings after it has been added.
Feel free to improve the user interface if you want to. A text area is the same way Transmission does it (at least last time I used it) and it's good enough for me.
comment:9 by , 11 years ago
Milestone: | 1.3.x → 1.4.0 |
---|
I forgot to say that this code would be for 1.4 (git develop) and you are now able to edit the hosts.
comment:10 by , 11 years ago
If you want this patch committed then you will need to do some more work to address the current issues.
- Preference page added instead of in ConnectionManager
- Adhere to visual style
- Config should be host-specific (dict of dict mentioned)
If your goal was not to have this committed then that's fine, we can just leave this ticket open so others can apply this patch if they wish and maybe someone out there may wish to improve the patch and get it to a committable state.
comment:11 by , 11 years ago
Yes, I opened this ticket since I thought it might be useful to some people. You (anyone) may ignore it, use it or build on top of it as you like in the true GPL spirit.
In the current host agnostic state, preference page makes sense but if hosts are editable in 1.4, I'll make the config host specific and move the it in the host dialog (just my preference). I probably won't do that until 1.4 is released and I get it from the package manager. Unless this feature is implemented by someone else by then.
comment:12 by , 11 years ago
If it makes the patch more useful, I can make it host specific and remove any additions to ui. That way it'll still usable, the ui will stay clean and a better ui designer can add the visuals later.
by , 11 years ago
Attachment: | path-mapping-multihost-noui.patch added |
---|
comment:13 by , 11 years ago
I've attached a version that supports multiple hosts and does not include additions to gui.
The mappings can be configured like this:
"path_mapping": { "host1": [ ["/remote/path", "/local/path"], ["/another/path", "/yet/another"] ], "host2": [ ["/remote/path", "/local/path"] ] }
Path mappings for a host are now lists of pairs rather than dicts to avoid confusion since dict lookups cannot be used anyway. It would be nice if the [remote, local]
pairs could be tuples instead but settings are encoded in json and json doesn't differentiate between lists and tuples.
The replacing of the base path now uses os.path.join so it shouldn't matter whether trailing directory separators are inconsistent.
This is a feature request, but I have a patch. Did I choose the correct ticket type?