Community
Participate
Working Groups
Currently the mountPathMapper extension point only solves mounted path issues for local. When a local windows drive points to a remote location, the mapper allows for custom temp file locations such that if the drive letter changes, the mapping will remain the same. It would make sense to generalize this functionality to include non-local so that custom mappings can be made for all kinds of remote connections.
Would this enhancement require API changes?
As far as I can tell, it shouldn't need API changes... but more investigation is required. I'll put this as 2.0.1 for now.
In order for this to work generically, we need to expand the ISystemMountPathMapper API to include user ids. With user ids, it will allow us to map files from one host differently depending on the user id. For example, if there are two different connections to the same FTP host, each using a different user id, the file systems paths may overlap yet not correspond to the same files - so we'd need to use the user id to distinguish how these are mapped.
Can't we abstract from adding the userID, and rather allow the mountPathMapper to choose a different kind of mapping based on _any_ kind of property in the host that it's free to choose? There could be a default implementation mapping based on host and username, but contributed implementations should also be able to map based on other properties (e.g. port, subsystem kind).
So Martin, are you suggesting that rather than having the framework pass in userId as an addition arg, like the 3rd arg here: /** * Returns the qualified workspace path for a replica of this mounted file. Since the * system path is not unique for mounted files, this allows a vender to make sure it is unique. * The workspace mapping should always be the remote path on the originating host. * * @param hostname the remote host * @param remotePath the remote path as seen by the file subsystem * @param userId the user id on the host * @return the corresponding workspace replica mapping */ public String getWorkspaceMappingFor(String hostname, String remotePath, String userId); /** * Returns the system path that can be used for copying the replica back to remote. When null * is returned RSE the file is treated as no longer available and thus remote uploads do not occur. Vendors * who would like to disable uploads for some period can implement this to return null during that period. * * @param hostname the remote host * @param remotePath the remote path as seen by the local file subsystem * @param userId the user id on the host * @return the local system path that represents the mounted file */ public String getMountedMappingFor(String hostname, String remotePath, String userId); ... we should instead use something more general, perhaps the FileSubSystem?
Passing the IFileServiceSubSystem seems like a good idea. Perhaps even the ISubSystem to make it more general. From the subsystem, code can get the host, port, username and all other properties, right?
The IRemoteFileSubSystem probably makes the most sense since it has access to all the stuff we need (host, port, user id, etc). This mount path stuff is a file-specific thing, so I don't think it helps to generalize to ISubSystem. I'm thinking that the only API we need to change is this: /** * Returns the qualified workspace path for a replica of this mounted file. Since the * system path is not unique for mounted files, this allows a vender to make sure it is unique. * The workspace mapping should always be the remote path on the originating host. * * @param hostname the remote host * @param remotePath the remote path as seen by the file subsystem * @param the remote file subsystem * @return the corresponding workspace replica mapping */ public String getWorkspaceMappingFor(String hostname, String remotePath, IRemoteFileSubSystem subsystem); For download this allows us to customize where the file goes in the temp files. For upload, we wouldn't need this because we already have the remote path, independent of the temp file location.
sounds good to me.
any chance to make it backward compatible? (I'd guess not)
Created attachment 80671 [details] changes to mount path mapper extension point
Martin, can you review this. Let me know what you think.
I've committed the changes to cvs.
*** Bug 193858 has been marked as a duplicate of this bug. ***
ISystemMountPathMapper#getWorkspaceMappingFor() has been changed to expect an additional parameter of type IRemoteFileSubSystem: http://dsdp.eclipse.org/help/latest/topic/org.eclipse.rse.doc.isv/reference/api/org/eclipse/rse/files/ui/resources/ISystemMountPathMapper.html
I think that the change is not sufficient to be practically usable. Looking at SystemRemoteEditManager#getMountPathMapperFor(), where the mount path mapper extension is actually used, it appears that when an ISV defines his own mountPathMapper it would never be used when the DefaultMountPathMapper is also active. It looks like mountPathMappers need a concept of being "more specific" than others, such that the code that gets a mapper iterates by order of priority. Also, it looks like it would make sense if ISystemMountPathMapper#handlesMappingFor() also included the additional IRemoteFileSubSystem parameter. Finally, the Javadoc in ISystemMountPathMapper does not seem sufficient for me. When reading it, I don't really understand how I would declare the extension correctly in order to satisfy, for instance, the following use cases: -) Add username in the mapped path -) Add port in the mapped path -) Support mapping of invalid characters, e.g. :<>?* on Windows
(In reply to comment #15) > I think that the change is not sufficient to be practically usable. > > Looking at SystemRemoteEditManager#getMountPathMapperFor(), where the mount > path mapper extension is actually used, it appears that when an ISV defines his > own mountPathMapper it would never be used when the DefaultMountPathMapper is > also active. Currently, DefaultMountPathMapper.handlesMappingFor() returns false, so it should never actually be used. > > It looks like mountPathMappers need a concept of being "more specific" than > others, such that the code that gets a mapper iterates by order of priority. > A priority field in the xml wouldn't help make one extension more specific but perhaps we could introduce a new API to ISystemMountPathMapper: int getPriority(String hostname, String remotePath, IRemoteFileSubSystem subsystem); It would be nice if the handlesMappingFor could somehow be combined with getPriority(). > Also, it looks like it would make sense if > ISystemMountPathMapper#handlesMappingFor() > also included the additional IRemoteFileSubSystem parameter. > You're right, it probably makes sense to add the IRemoteFileSubSystem parameter here. > Finally, the Javadoc in ISystemMountPathMapper does not seem sufficient for me. > When reading it, I don't really understand how I would declare the extension > correctly in order to satisfy, for instance, the following use cases: > -) Add username in the mapped path > -) Add port in the mapped path > -) Support mapping of invalid characters, e.g. :<>?* on Windows > I'll take a look at the javadoc.
I've made changes as suggested. I've changed handlesMappingFor to: public boolean handlesMappingFor(String hostname, String remotePath, IRemoteFileSubSystem subsystem); I've also added: int getPriority(String hostname, String remotePath, IRemoteFileSubSystem subsystem); And I've put in more javadoc to describe the use cases.
Bug 245260 extends on this API, providing a UI for changing the way the DefaultMountPathMapper works.
