[ViewAPI] implement initial view API #1642

larkin · 2017-07-05T20:57:06Z

This is a discussion / develop branch for finalizing the view API.

renamed openmct.mainViews to openmct.objectViews. I think this is more clear that these are views of an object, and separates them from "indicators" and the soon-to-be "inspector view".
Updated view provider to allow metadata definitions and to play nicely with old style views.
Roughed in some updates to ViewProviders and ViewRegistry to support further use of views.

For testing, you can define a view by adding the following to index.html:

openmct.objectViews.addProvider({
    name: 'my view',
    canView: function (d) {
        return d.type === 'folder' && 150;
    },
    view: function (domainObject) {
        return {
            show: function (container) {
                container.innerHTML = '<div> Congratulations, this is a view of: ' + domainObject.name + '</div>';
            },
            destroy: function (container) {

            }
        }
    }
});

@VWoeltjen et all, feedback appreciated. (@luisschubert you may also be interested).

larkin · 2017-08-03T17:39:15Z

TODO: Need to handle editable / separate views for edit mode.

larkin · 2017-08-03T23:39:50Z

TODO: need to address container sizing. If someone wants to inject a view that is 100% of the width and height of the container, they need to intentionally add the "abs" class to the container. Either we document this as a thing view developers need to do, or we add this class to the container so they don't have to worry about it.

VWoeltjen · 2017-08-28T19:11:49Z

TODO: need to address container sizing. If someone wants to inject a view that is 100% of the width and height of the container, they need to intentionally add the "abs" class to the container. Either we document this as a thing view developers need to do, or we add this class to the container so they don't have to worry about it.

I think we should add this class at a container level from the platform whenever that layout behavior is desired. Simpler, meets principle of least surprise (I do not expect to need to worry about the layouts of container elements when I'm writing a view plugin), reduces our effective API surface (insofar as CSS classes are "APIs") and gives us more implementation flexibility in the future.

VWoeltjen · 2017-08-28T19:32:37Z

TODO: Need to handle editable / separate views for edit mode.

Running through some options:

We've talked about using context objects for selections; this is another case where context information would be useful.
editors could be a wholly separate extension point. That looks really good in the view-as-object world (don't show the Edit button if there are no editors registered). With a many-to-one view-to-object relationship, we'd need keys or something to pair editors to their appropriate views.
Editing could be handled in the same view instance by listening for events (e.g. changes between browse and edit modes). In some sense this is the simplest solution, but using events to track application state presents an easy opportunity for things to fall out of sync.

I'm leaning toward Option 2 given current status. Feels clean and straightforward. The many-to-one problem makes it a little difficult, but view-as-object should entirely mitigate that.

akhenry · 2017-10-19T21:14:13Z

@VWoeltjen I like editors as a separate extension point. It makes it super clear to developers how to add a new editing view of an object. It also doesn't actually stop you from using the same view if you want to, you could just check on view initialization whether the object is currently being edited, which avoids the need to listen to event changes.

akhenry

Looks good. Excited to start using the new view API! A couple of comments inline.

akhenry · 2017-10-19T19:33:12Z

src/adapter/capabilities/patchViewCapability.js

+                        // be defaults by returning priority less than 100.
+                    };
+                    if (v.provider) {
+                        vd.priority = v.provider.canView(newDomainObject);


I'm not so sure about canView returning priority. It's kind of nice in that you can treat anything non-zero as boolean true if you don't care about its priority, but it will inevitably lead to type checking in client code.

Also it doesn't seem obvious to me that it's going to return priority, and it feels like it's violating the "Do One Thing" rule.

Separating out priority will complicate the view provider interface, but will improve clarity I think.

per discussion:

canview returns boolean

priority function is optional, when present takes object and returns priority (number)

akhenry · 2017-10-19T19:34:32Z

src/MCT.js

+         * @name objectViews
         */
-        this.mainViews = new ViewRegistry();
+        this.objectViews = new ViewRegistry();


akhenry · 2017-10-19T20:29:34Z

src/ui/ViewRegistry.js

+     * views.
+     * @private
+     */
+    ViewRegistry.prototype._getByVPID = function (vpid) {


I've been tempted in the past to use the _ prefix, but decided against it in lieu of better JSDocs. Since we're potentially moving our API docs away from JSDoc this might be a good convention to use, so long as we use it consistently. The danger is that anything not prefixed with an _ is considered fair game by developers. Eg. .get() is also marked private but doesn't have an underscore.

akhenry · 2017-10-19T20:33:06Z

src/ui/ViewRegistry.js

-     * @returns {boolean} true if this domain object can be viewed using
-     *          this provider
+     * @returns {Number|boolean} if this returns `false`, then the view does
+     *          not apply to the object.  If it returns true or any number, then


See above comments about canView vs priority.

akhenry · 2017-10-19T22:04:43Z

src/ui/ViewRegistry.js

     * @memberof module:openmct.ViewRegistry#
     */
    ViewRegistry.prototype.addProvider = function (provider) {
+        provider._vpid = this._next_id++;


Maybe view providers should just have keys? Or is the vpid just internal wiring for compatibility with legacy views? My jetlag addled brain is having trouble understanding vpid

per discussion, yes view providers should have keys.

akhenry · 2017-10-20T00:48:10Z

Pushed two commits. The first separates out priority and canView. Not sure priority is being used properly at the moment in the new view API, openmct.objectViews.get() should probably return a list of views sorted by priority, no?

The second commit replaces vpid with a view provider key.

Need to do some testing and sanity-checking tomorrow to ensure that what I've committed is not the lunatic ravings of a jetlagged mind, and is at least up to the standard of my usual drivel.

Also, still need to fix those code style errors.

akhenry · 2017-10-20T18:39:59Z

@larkin I'm done with this if you want to take a look.

@VWoeltjen Since this impacts your work on the heatmap view I'm assigning you reviewer :)

@dtailor90 This also affects summary widgets and rover view as view providers now require a 'key' and canView should no longer return priority. If priority is required, a separate priority function can be implemented. A notional view with a low priority defined would look like:

openmct.objectViews.addProvider({
    key: 'my-view',
    name: 'my view',
    canView: function (d) {
        return d.type === 'folder';
    },
    priority: function (d) {
        return 1000;
    },
    view: function (domainObject) {
        return {
            show: function (container) {
                container.innerHTML = '<div> Congratulations, this is a view of: ' + domainObject.name + '</div>';
            },
            destroy: function (container) {

            }
        }
    }
});

larkin · 2017-10-20T19:06:50Z

src/MCT.js


+        this.objectViews.getAllProviders().forEach(function (p) {
+            this.legacyExtension('views', {
+                key: 'provider-namespace-' + p.key,


per discussion: don't prefix

larkin · 2017-10-20T19:08:20Z

src/ui/ViewRegistry.js

+            throw "View providers must have a unique 'key' defined";
+        }
+        if (this.providers[key] !== undefined) {
+            throw "Provider already defined for key '" + key + "'. Keys must be unique.";


per discussion; make this a warning at most, allow overrides.

larkin · 2017-10-20T19:33:29Z

Writing this post so I can link a few developers to it. Can also integrate into API docs later.

changes for existing users

For anyone currently using the view API, the following changes need to be made to your usage of the view API after this pull request (#1642) is merged:

openmct.mainViews.addProvider(provider) is now openmct.objectViews.addProvider(provider)
providers must have a key property, a unique identifier for this view provider.
provider.canView must return a boolean.

For a more complete listing of supported properties, look no further than this handy dandy table.

Object View Provider properties

property	type		description
`key`	string	required	a unique identifier for this view provider.
`canView`	function	required	is called with a single argument, `domainObject` and should return `true` if the view provider can provider a view for the given `domainObject`.
`view`	function	required	is called with a single argument, `domainObject`, and must return a view instance.
`priority`	function	optional	is called with a single argument, `domainObject`, and should return a priority (a number) for the given domain Object. Small values (e.g. 1) are higher priority than large values (e.g. 1000).
`editable`	boolean	optional	defaults to `false`. If `false`, attempting to edit this view will display the edit properties dialog. If `true`, the edit action will trigger edit mode, allowing your view to implement specialized editing behavior within Open MCT's editor sandbox (allowing changes to be made, and then saved or discarded).
`name`	string	optional	a human-readable name for your view, displayed when there is more than one possible view for an object.
`cssClass`	string	optional	display class for your view label, typically used to show an icon when there is more than one possible view for an object.
`description`	string	optional	a human-readable description of your view, displayed when there is more than one possible view for an object.

akhenry · 2017-10-20T19:56:41Z

I've squished all the commits for this branch into one.

larkin · 2017-10-21T00:09:11Z

Leaving an author checklist as a partial author, @akhenry can post review and author checklists as the other partial author.

Author Checklist

Changes address original issue? Y (as described here)
Unit tests included and/or updated with changes? (N/A, none exist)
Command line build passes? Y
Changes have been smoke-tested? Y

akhenry · 2017-10-21T00:09:55Z

Author Checklist

Changes address original issue? Y
Unit tests included and/or updated with changes? N/A
Command line build passes? Y
Changes have been smoke-tested? Y

larkin · 2017-10-21T00:10:39Z

Reviewer Checklist (For Andrew's authorship)

Changes appear to address issue? Y
Appropriate unit tests included? N/A
Code style and in-line documentation are appropriate? Y
Commit messages meet standards? Y

akhenry

Reviewer Checklist

Changes appear to address issue? Y
Appropriate unit tests included? N/A
Code style and in-line documentation are appropriate? Y
Commit messages meet standards? Y

adoubekk mentioned this pull request Aug 8, 2017

Summary Widgets #1668

Merged

4 tasks

larkin mentioned this pull request Sep 18, 2017

view registry is not implemented #1307

Open

larkin mentioned this pull request Oct 19, 2017

[Views] Clear regions on destroy #1781

Merged

VWoeltjen requested review from VWoeltjen and removed request for VWoeltjen October 19, 2017 21:57

akhenry reviewed Oct 19, 2017

View reviewed changes

akhenry requested a review from VWoeltjen October 20, 2017 17:36

akhenry previously approved these changes Oct 20, 2017

View reviewed changes

larkin commented Oct 20, 2017

View reviewed changes

Initial functional view API implementation. See #1642

d7b44f8

akhenry dismissed their stale review via 4623fd7 October 20, 2017 19:42

akhenry force-pushed the view-api-implementation branch from 4623fd7 to d7b44f8 Compare October 20, 2017 19:50

akhenry requested review from akhenry and removed request for VWoeltjen October 21, 2017 00:10

akhenry approved these changes Oct 21, 2017

View reviewed changes

larkin merged commit 7f68d26 into master Oct 21, 2017

akhenry deleted the view-api-implementation branch October 21, 2017 00:11

deeptailor mentioned this pull request Jun 14, 2018

Telemetry Consumer API #2070

Open

[ViewAPI] implement initial view API #1642

[ViewAPI] implement initial view API #1642

Uh oh!

Conversation

larkin commented Jul 5, 2017

Uh oh!

larkin commented Aug 3, 2017

Uh oh!

larkin commented Aug 3, 2017

Uh oh!

VWoeltjen commented Aug 28, 2017

Uh oh!

VWoeltjen commented Aug 28, 2017

Uh oh!

akhenry commented Oct 19, 2017

Uh oh!

akhenry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhenry commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akhenry commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larkin commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

changes for existing users

Object View Provider properties

Uh oh!

akhenry commented Oct 20, 2017

Uh oh!

larkin commented Oct 21, 2017

Author Checklist

Uh oh!

akhenry commented Oct 21, 2017

Author Checklist

Uh oh!

larkin commented Oct 21, 2017

Reviewer Checklist (For Andrew's authorship)

Uh oh!

akhenry left a comment

Choose a reason for hiding this comment

Reviewer Checklist

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

akhenry commented Oct 20, 2017 •

edited

Loading

akhenry commented Oct 20, 2017 •

edited

Loading

larkin commented Oct 20, 2017 •

edited

Loading