Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Enable opening of remote(http|https) files in Brackets #14153

Merged
merged 10 commits into from
May 4, 2018

Conversation

swmitra
Copy link
Collaborator

@swmitra swmitra commented Mar 10, 2018

This PR adds File protocol adoption model to support various file protocols which mainly differs in read and write mechanism. With this PR we are adding a reference implementation for this protocol adapter model by enabling HTTP|HTTPS protocols. We are introducing a new protocol adapter registration hook in FileSystem module as function registerProtocolAdapter(protocol, adapter) where protocol can be any valid file protocol string like

HTTP: | HTTPS: | FTP: | SFTP:

for which we want to add support. adapter is the the actual protocol adapter object which should expose canRead(filePath) function handle and fileImpl which is the File interface implementation prototype handle, this will be be used by FileSystem to instantiate this new type of File if the protocol adapter is selected based on the filePath.

This change list also adds a plugin in QuickOpen, enabling user to input remote file path (HTTP | HTTPS ) in QuickOpen file search dialog to open a file.

TODO

  • Unit tests
  • Ux workflow
  • List remote files in Quick open file search list

@swmitra swmitra changed the title [WIP]Enable opening of remote(http|https) files in Brackets [WIP] Enable opening of remote(http|https) files in Brackets Mar 10, 2018
@vickramdhawal
Copy link
Collaborator

How are the users supposed to trigger opening of a remote file ? Is there any UI associated with it ?
Currently, users can open files only from the mounted file system.

@swmitra
Copy link
Collaborator Author

swmitra commented Mar 20, 2018

@vickramdhawal It can be done by file open command.

CommandManager.execute(Commands.FILE_OPEN, {
    fullPath: "<remote-file-uri here>"
});

[update]
When someone clicks on a remote file link, above snippet can be used to open that remote file. Currently there isn't any scope or feasibility in brackets sidebar to list remote files as part of project tree, in fact there isn't any use case unless we go ahead and support FTP site root mapping as our working file tree. We can then assume that any listing of remote files would be through some other auxiliary UI element, in which case the select/click handler for that UI element could be programmed to utilize this command execution approach to open a remote file.

@abose
Copy link
Contributor

abose commented Mar 31, 2018

"protocol can be any valid file protocol string like HTTP: | HTTPS: | FTP: | SFTP:"
Is FTP and SFTP supported in this change?

@abose
Copy link
Contributor

abose commented Mar 31, 2018

Also would be good if this could be hooked into some UI. maybe the quick open cmd+o hotkey.
This feature would help to adapt code from the internets.

@swmitra
Copy link
Collaborator Author

swmitra commented Apr 2, 2018

@abose This PR provides implementation only for http/https, but a plugin framework is also being added in this PR, where other extensions can hook onto and provide custom read/write handles, or add support for a new protocol.

Let me see if this can be hooked to quickopen where user can type/paste a remote path to open a file.

@abose
Copy link
Contributor

abose commented Apr 2, 2018

Now that quick open works on http/s URL , normal file[file:///c/a.txt or c:\a.txt etc] paths cannot be opened with the same. Would be good if this behaved consistently for file path open as well.

[swmitra] - @abose I guess it never worked earlier with file path even without this PR. My take on this - can be taken as a separate PR! I will still do some quick check...


var SESSION_START_TIME = new Date();

/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: /**

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in latest commit 👍

HTTPS_PROTOCOL = "https:";

AppInit.htmlReady(function () {
var protocolAdapter = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is an extension shouldn't the definition of ProtocolAdapter be provided by Brackets-Core for consistency.

Copy link
Collaborator Author

@swmitra swmitra Apr 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vickramdhawal Sorry for the confusion. protocolAdapter is just a variable to hold the custom imprint of the adapter definition to be passed as protocol adapters for HTTP: and HTTPS:. Registration happens by invoking this part -

// Register the custom object as HTTP and HTTPS protocol adapter
FileSystem.registerProtocolAdapter(HTTP_PROTOCOL, protocolAdapter);
FileSystem.registerProtocolAdapter(HTTPS_PROTOCOL, protocolAdapter);

I can't really define an interface (which if was possible, would have been part of core) and Duck Typing would be a gimmick as type checking is not something what we are interested about here.

For better understanding I have added a JSDoc @typedef

/**
 * Typical signature of a file protocol adapter.
 * @typedef {Object} FileProtocol~Adapter
 * @property {Number} priority - Indicates the priority.
 * @property {Object} fileImpl - Handle for the custom file implementation prototype.
 * @property {function} canRead - To check if this impl can read a file for a given path.
 */

/**
 * FileSystem hook to register file protocol adapter
 * @param {string} protocol ex: "https:"|"http:"|"ftp:"|"file:"
 * @param {...FileProtocol~Adapter} adapter wrapper over file implementation
 */

@abose
Copy link
Contributor

abose commented Apr 3, 2018

LGTM.

@swmitra swmitra changed the title [WIP] Enable opening of remote(http|https) files in Brackets Enable opening of remote(http|https) files in Brackets Apr 18, 2018
/**
* Helpful toString for debugging and equality check purposes
*/
RemoteFile.prototype.toString = function () {
return "[" + (this._isDirectory ? "Directory " : "File ") + this._path + "]";
return "[" + (this._isDirectory ? "Directory " : "RemoteFile ") + this._path + "]";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a remote URI be described as a directory ? Looks weird to me.

@swmitra swmitra added this to the Release 1.13 milestone Apr 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants