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

customViewer API: Registry to enable extension developers to add CustomViewers #6172

Merged
merged 3 commits into from
Dec 6, 2013

Conversation

couzteau
Copy link
Member

@couzteau couzteau commented Dec 4, 2013

This change adds a public API to EditorManager that enables extension developers to add custom viewers to Brackets following the same pattern as implemented by ImageViewer: When a file is opened EditorManager is consulted for available CustomViewers for the given file type. If available the custom viewer is used by EditorManager to display the file.

A custom viewer would have to implement the following:

  • render(fullPath, $editorHolder)
    • @param {!string} fullPath Path to the image file
    • @param {!jQueryObject} $editorHolder The DOM element to append the view to.
  • onRemove()
    signs off listeners and performs any required clean up when editor manager closes
    the custom viewer

To register with EditorManager registerCustomViewerProvider(langId, provider) needs to be called where
@param {!String} languageId, i.e. image, audio, ... to identify a language known to LanguageManager
@param {!Object} provider Custom view provider instance

@ghost ghost assigned RaymondLim Dec 4, 2013
* @param {!string} fullPath Path to the image file
* @param {!jQueryObject} $editorHolder The DOM element to append the view to.
* function render(fullPath, $editorHolder)
* - function onRemove()
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this is inconsistent with the previous bullet. Maybe just "render()" and "onRemove()" (with no "function") for both?

And also remove the extra copy of render() on the line just above here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@peterflynn
Copy link
Member

@couzteau I've added a few comments but I think this looks great overall!

@couzteau
Copy link
Member Author

couzteau commented Dec 4, 2013

@peterflynn Thanks for the review

@RaymondLim
Copy link
Contributor

Done reviewing and no more to add since @peterflynn already done a nice review and comments.

@couzteau
Copy link
Member Author

couzteau commented Dec 5, 2013

back to reviewers. Thanks

@@ -34,7 +34,8 @@ define(function (require, exports, module) {
ProjectManager = require("project/ProjectManager"),
Strings = require("strings"),
StringUtils = require("utils/StringUtils"),
FileSystem = require("filesystem/FileSystem");
FileSystem = require("filesystem/FileSystem"),
ImageViewer = require("editor/ImageViewer");
Copy link
Contributor

Choose a reason for hiding this comment

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

It is weird to require itself. I don't think we want this way to pass ImageViewer module. I was expecting that you create a constructor here and convert all functions to methods of the new object, then pass to register function call with a new instance of the new object.

Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative would be to just pass exports as the ImageViewer module, since the provider just needs some sort of object with 2 methods. Which means that we can just pass it a simple object instead of creating a function and its prototypes.

EditorManager.registerCustomViewer("image", exports);

or

EditorManager.registerCustomViewer("image", {
    render: function () { ... },
    onRemove: function () { ... }
});

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, either of those seems fine. I'd lean toward the 2nd form since we use that pattern in a number of other modules (and it doesn't force you to export APIs that you don't actually want anyone other than registerCustomViewer() to call).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks guys, I'll go with Tom's suggestion.

@couzteau
Copy link
Member Author

couzteau commented Dec 5, 2013

Back to reviewers. Thanks.

@RaymondLim
Copy link
Contributor

Looks good! Merging.

RaymondLim added a commit that referenced this pull request Dec 6, 2013
customViewer API: Registry to enable extension developers to add CustomViewers
@RaymondLim RaymondLim merged commit 1c9d5b0 into master Dec 6, 2013
@RaymondLim RaymondLim deleted the couzteau/custom-viewer-registry branch December 6, 2013 00:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants