[MERGEABLE] Splitview With Image and Custom View Support #8753
Conversation
|
|
||
|
|
||
| /*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */ | ||
| /*global define, $, window, Mustache */ |
There was a problem hiding this comment.
This module doesn't use Mustache
|
@JeffryBooher Done with initial code review. |
| "use strict"; | ||
|
|
||
| /** | ||
| * @typedef {canOpenFile:function(path:string):boolean, openFile:function(path:string, pane:Pane)} Factory |
There was a problem hiding this comment.
This is changes a few things from the old implementation:
- It's keyed off of arbitrary path filtering rather than Language, which is more flexible but less efficient (and a little more work for extension developers). Any particular reasoning behind that change?
- The old terminology was "custom view provider" and the new term is "view factory." I sort of like the old naming better -- in particular, "provider" is a term we already use elsewhere in Brackets.
Also, this doesn't document what openFile() is expected to do, or what it returns. It looks like the expectation is that it constructs a View and calls pane.addView() with it, returning a Promise that is resolves when that's done (or is rejected with a FileSystemError object). Would it be cleaner to just have it return a Promise that resolves with a View, and then Bracket core takes care of adding it to the right pane? That would be a little more in line with how other provider extensibility points work...
(This should also reference where to find docs for the View interface -- it took me a min to find it at the top of the Pane.js module body).
There was a problem hiding this comment.
I was also a little thrown again by how MainViewManager.addView() takes a File, while Pane.addView() takes a View. That naming feels confusing. But if we change the provider to not call Pane.addView() directly, then maybe that API can be more hidden, reducing the potential for confusion...
| @@ -96,6 +96,8 @@ define(function (require, exports, module) { | |||
| * adjustScrollPos: function(state:Object=, heightDelta:number) - called to restore the scroll state and adjust the height by heightDelta | |||
| * switchContainers: function($newContainer:jQuery} - called to reparent the view to a new container | |||
| * getContainer: function() - called to get the current container @return {!jQuery} - the view's parent container | |||
| * getViewState: function() @return {?*} - Called when the pane wants the view to save its view state. Return any data you needed to restore the view state later or undefined to not store any state | |||
| * restoreViewState: function(!viewState:*) - Called to restore the view state. The viewState argument is whatever data was returned from getViewState() | |||
There was a problem hiding this comment.
We should specify that these must be JSONifiable -- otherwise serialization will fail
[MERGEABLE] Splitview With Image and Custom View Support
https://trello.com/c/A22yIIal/1282-splitview-add-view-provider-mechanics
This PR is can be merged into jeff/splitview-1x2 when ready