Opened 10 years ago

Last modified 7 years 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)

remote-path.patch (8.5 KB) - added by eerorika 10 years ago.
path-mapping.patch (7.7 KB) - added by eerorika 10 years ago.
path-mapping-dict.patch (7.6 KB) - added by eerorika 10 years ago.
path-mapping-multihost-noui.patch (3.9 KB) - added by eerorika 10 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 10 years ago by eerorika

This is a feature request, but I have a patch. Did I choose the correct ticket type?

Changed 10 years ago by eerorika

Changed 10 years ago by eerorika

comment:2 Changed 10 years ago by Cas

That is way more complicated than it needs to be, path_mapping should be a dictionary of paths.

Changed 10 years ago by eerorika

comment:3 Changed 10 years ago by eerorika

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:4 Changed 10 years ago by Cas

You don't expose the dictionary to the users, you use gtk ui elements.

comment:5 Changed 10 years ago by eerorika

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 Changed 10 years ago by Cas

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 Changed 10 years ago by andar

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 Changed 10 years ago by eerorika

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

Last edited 10 years ago by eerorika (previous) (diff)

comment:9 Changed 10 years ago by Cas

  • Milestone changed from 1.3.x to 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 Changed 10 years ago by andar

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 Changed 10 years ago by eerorika

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.

Version 0, edited 10 years ago by eerorika (next)

comment:12 Changed 10 years ago by eerorika

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.

Changed 10 years ago by eerorika

comment:13 Changed 10 years ago by eerorika

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.

comment:14 Changed 7 years ago by Cas

  • Milestone changed from 2.0.x to 2.x

Milestone renamed

Note: See TracTickets for help on using tickets.