Selection indicator and info box #1419

emackey · 2014-01-29T15:38:44Z

This adds a selection indicator and information box to viewerDynamicObjectMixin.

NOTE: This includes #1417, "Description property for CZML." It also introduces Google Caja for HTML sanitization.

This is "a slightly tweaked version of the JsHtmlSantizer object from https://github.com/mapbox/sanitize-caja"

The icon has 3 modes: 1. Enabled, for untracked objects with known positions. 2. Disabled but with the same icon as enabled, for objects being tracked. 3. Disabled and crossed out, for objects without positions. Note that the selection graphic is able to show un-trackable "scratch" positions, such as access lines, but the camera icon will appear disabled for these kinds of objects.

mramato · 2014-01-29T15:52:14Z

To provide context for others, this is our solution to HTML Balloon pop-ups you see in other apps like Google Earth and Leaflet. This code is actually based on the original work @hpinkos did in the balloon branch. While we originally had planned on using a balloon, it turns out that it's a horrible UI design when time-dynamic moving objects are involved. Since Cesium treats time-dynamic data as a first class citizen, we took a fresh approach and this is what we came up with. And if you ask me it's much better. (The fact that me and @emackey agree either proves it's a great idea or is a sign of the apocalypse, it could go either way).

mramato · 2014-01-29T15:53:31Z

@emackey If an object has no description property and no name property, should we hide the info box completely, or just fall back on id? I would assume the later, but I'll leave it up to you.

emackey · 2014-01-29T15:54:48Z

Good point, some of the code here, particularly for positioning the selection graphic, is directly based on @hpinkos early work on the balloon branch. Thanks Hannah!

I'll add the id fallback.

mramato · 2014-01-29T16:34:35Z

So my last aesthetic related comment is that the selection box is a little big and bulky. Maybe we could make it smaller and not as thick? Another idea would be to just have the 4 corners, with gaps in between them. This is just my personal preference though, I'm fine if other people like it.

mramato · 2014-01-29T18:25:12Z

Is there a reason that both the selection indicator and information window are the same widget? I was under the impression that they would be two separate widgets. The "Information" widget, for lack of a better term, is an overlay widget that any app could use to show information while the SelectionIndicator widget would be the graphic used to indicate selection (whether a DynamicObject or otherwise). They would both be part of the Viewer widget by default and the viewerDynamicObjectMixin would simply wire them up to the DataSource layer. This makes everything much more module and flexible than what it looks like we currently have in this branch and provides flexibility to app writers without changing any high-level functionality. Thoughts?

@shunter I'd be interested in hearing your thoughts on this as well.

pjcozzi · 2014-01-29T20:40:12Z

CHANGES.md!

pjcozzi · 2014-01-29T20:42:09Z

Can we add a Sandcastle example for how to use this? How many lines of code will it be? An older version was a ton of code; I couldn't even figure out how to do it; @mramato had to send me the code.

emackey · 2014-01-29T21:08:09Z

Sure. It's meant to be wired up automatically when using the viewerDynamicObjectMixin, any dynamic objects are then selectable with a left click. See the CZML Sandcastle demo for example. I don't know how strongly we want to encourage its use without the mixin. Its behavior would be much more open-ended without the mixin, left up to the app to update it every clock tick and so on. There will be even more lines of code for this kind of manual use once the info box splits into a separate widget, because both widgets will need to be kept in sync by the mixin.

There was a deliberate design choice to have the new widget(s) not access any internals of dynamic objects, so they are not explicitly tied to the dynamic scene system unless you bring in the mixin. This means you can tie them to something else, but you're then responsible for manually updating positions, visibility, etc., which will be some non-trivial code.

shunter · 2014-01-29T22:22:45Z

Source/Widgets/SelectionIndicator/SelectionIndicatorViewModel.js

+        posY = Math.min(Math.max(position.y + posMin, posMin), posMaxY);
+
+        var positionXPx = toPx(posX);
+        if (viewModel._positionX !== positionXPx) {


These checks are unnecessary. Observables already check their new values for equality with the previous value before notifying subscribers.

emackey · 2014-01-29T22:51:26Z

PAUSE REVIEW.... refactoring into multiple widgets! Sorry!

mramato · 2014-01-29T22:52:21Z

Well, now you've at least got a jump start on some comments 😄

shunter · 2014-01-29T22:56:10Z

Source/Widgets/Viewer/viewerDynamicObjectMixin.js

+                return selectedObjectObservable();
+            },
+            set : function(value) {
+                if (selectedObjectObservable() !== value) {


I know that trackedObject was already the same way, but I wonder if it might be more clear to use a "plain" observable (ko.track) for viewer.selectedObject, and then subscribe to that observable to trigger these other changes, rather than using a computed observable that does nothing except trigger side effects.

Unit tests are broken, and the mixin isn't tolerant of the widgets being turned off yet.

mramato · 2014-02-01T00:09:44Z

@emackey I was running Sandcastle examples on my laptop to make sure everything was kosher and ran into usability issues due to the hard-coded width of the InfoBox. Rather than have it hardcoded to 360, I tweaked the css change cesium-infoBox to have

width: 25%;
max-width: 360px

and got rid of the @media screen and (max-width: 400px) block. This works much better on smaller screens and embedded viewer in my opinion and doesn't change the current behavior on . I didn't want to submit it without running it by you. What do you think?

mramato · 2014-02-01T00:30:47Z

CHANGES.md

@@ -12,6 +12,7 @@ Beta Releases
  * Renamed `GeometryPipeline.createAttributeIndices` to `GeometryPipeline.createAttributeLocations`.
  * Renamed `attributeIndices` property to `attributeLocations` when calling `Context.createVertexArrayFromGeometry`.
  * `PerformanceDisplay` requires a DOM element as a parameter
+* Added new `SelectionIndicator` and `InfoBox` widgets, activated by `viewerDynamicObjectMixin`.


This needs to be updated since they are now part of Viewer itself. Also, in addition to mentioning viewerDynamicObjectMixin using them by default, we should add a code snippet here showing how to turn it off to go back to the old behavior.

Also removed an unused variable.

mramato · 2014-02-01T01:10:39Z

Source/Widgets/SelectionIndicator/SelectionIndicatorViewModel.js

+         * Gets the visibility of the position indicator.
+         * @type {Boolean}
+         */
+        this.showPosition = undefined;


I'm sure I'm missing something but what is this variable for? I only discovered it by looking at the reference doc and I found it confusing since there's already a showSelection.

This could be converted to private, it's a combination of showSelection being true along with position being defined. The visibility of the indicator binds to this, not to showSelection alone. Its name is perhaps a little dated, since it pre-dates the selection/info breakup. Maybe isSelectionIndicatorVisible or something.

It comes into play for things like access lines, that disappear.

That's a little wordy, but isVisible or isSelectionVisible is fine with me.

pjcozzi · 2014-02-03T14:14:56Z

You guys don't want this for b25, right? I'd like to start the release in the next hour or two.

mramato · 2014-02-03T14:22:11Z

There's no need to wait for this. As excited as I am to get this feature out, like with any big feature we should have it in master for a while first to shake out any potential issues we missed.

emackey · 2014-02-03T14:28:26Z

Sadly I agree, this feature could use some extra time on master before release. Also I would like to have the gallery branch in the same release, and it still needs work.

pjcozzi · 2014-02-03T15:00:38Z

I agree completely. Thanks.

mramato · 2014-02-03T16:15:09Z

So other than my width styling suggestion, is there anything else that still needs to be done here? Or are we good to go after b25 ships?

emackey · 2014-02-03T16:16:00Z

I think we're good. @mramato can you add that styling to this branch and I'll test it?

mramato · 2014-02-03T16:20:20Z

@emackey I'm not sure what's changed, but on a clean checkout of this branch, the InfoxBox is coming up with a transparent background. Are you seeing it too?

mramato · 2014-02-03T16:24:07Z

Nevermind, it was something stupid I did.

Rather than a hardcoded width, we now default to 25% or 360px, whichever is smaller.

mramato · 2014-02-03T16:28:05Z

@emackey My changes are in. Assuming you are okay with them, @shunter can give this one final look and merge after b25 ships.

emackey · 2014-02-03T18:34:36Z

@mramato it seems very thin below about 600px wide. Can we do width: 40% instead of 25?

mramato · 2014-02-03T18:54:32Z

I bumped it up to 40% as you asked. We can tweak it later as we start using it more. I agree 25% can be too thin, my thinking was that we should err on the side of thin rather than having the InfoBox obscure too much of the screen.

emackey · 2014-02-03T19:04:19Z

On a thin screen, like a phone in portrait mode, we may have to allow the box to fill most of the screen, and provide some kind of mechanism to slide the box out of the way without canceling the selection. Possibly the "x" in the box's upper-right should just remove the box without removing the selection. Not sure how to get the box back if we did that, maybe single-click the selected item again. Seems like we'll have to play with a few options on mobile at some point.

mramato · 2014-02-03T20:43:53Z

There's a chance we might want that on the desktop as well; we've got a month to tweak things and figure it out.

@shunter as far as me and @emackey are concerned, this is ready for master, please merge if you don't have any further comments.

Selection indicator and info box

emackey added 6 commits January 28, 2014 14:11

Import Google Caja (via @mramato)

d636504

This is "a slightly tweaked version of the JsHtmlSantizer object from https://github.com/mapbox/sanitize-caja"

Show a box-shaped selection indicator.

dd00c6b

Merge remote-tracking branch 'origin/description' into box-select

e4ffc63

CSS tweak for long titles.

19f5877

Merge remote-tracking branch 'origin/master' into box-select

b6b896a

Use the ID (instead of blank) when the name is missing.

fdda84c

emackey added 2 commits January 29, 2014 14:40

Make selection box thinner.

df3e30d

Add separate CSS styles when no description is present.

f59f9ff

shunter reviewed Jan 29, 2014
View reviewed changes

emackey added 5 commits January 29, 2014 18:04

Initial breakup of selection and info box.

c253a51

Unit tests are broken, and the mixin isn't tolerant of the widgets being turned off yet.

Merge remote-tracking branch 'origin/master' into box-select

50ccfc1

Rename events.

4d29201

Size & position code cleanup.

1efa88e

Code cleanup.

b43c82a

mramato reviewed Feb 1, 2014
View reviewed changes

Fix documentation.

f2a0a9c

Also removed an unused variable.

mramato reviewed Feb 1, 2014
View reviewed changes

This was referenced Feb 1, 2014

Entity & Data Sources roadmap #1045

Open

Balloon Browser widget #954

Closed

emackey added 3 commits February 3, 2014 10:03

Rename showPosition to isVisible.

1638785

Merge remote-tracking branch 'origin/master' into box-select

c9d26fc

Update CHANGES.md

e37e536

Tweak InfoBox css

c106ffa

Rather than a hardcoded width, we now default to 25% or 360px, whichever is smaller.

Up InfoBox width to 40%

768a7e4

Remove unused modules.

4e4f645

shunter added a commit that referenced this pull request Feb 3, 2014

Merge pull request #1419 from AnalyticalGraphicsInc/box-select

d8fe83b

Selection indicator and info box

shunter merged commit d8fe83b into master Feb 3, 2014

shunter deleted the box-select branch February 3, 2014 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Selection indicator and info box #1419

Selection indicator and info box #1419

emackey commented Jan 29, 2014

mramato commented Jan 29, 2014

mramato commented Jan 29, 2014

emackey commented Jan 29, 2014

mramato commented Jan 29, 2014

mramato commented Jan 29, 2014

pjcozzi commented Jan 29, 2014

pjcozzi commented Jan 29, 2014

emackey commented Jan 29, 2014

shunter Jan 29, 2014

emackey commented Jan 29, 2014

mramato commented Jan 29, 2014

shunter Jan 29, 2014

mramato commented Feb 1, 2014

mramato Feb 1, 2014

mramato Feb 1, 2014

emackey Feb 2, 2014

emackey Feb 2, 2014

mramato Feb 2, 2014

pjcozzi commented Feb 3, 2014

mramato commented Feb 3, 2014

emackey commented Feb 3, 2014

pjcozzi commented Feb 3, 2014

mramato commented Feb 3, 2014

emackey commented Feb 3, 2014

mramato commented Feb 3, 2014

mramato commented Feb 3, 2014

mramato commented Feb 3, 2014

emackey commented Feb 3, 2014

mramato commented Feb 3, 2014

emackey commented Feb 3, 2014

mramato commented Feb 3, 2014

Selection indicator and info box #1419

Selection indicator and info box #1419

Conversation

emackey commented Jan 29, 2014

mramato commented Jan 29, 2014

mramato commented Jan 29, 2014

emackey commented Jan 29, 2014

mramato commented Jan 29, 2014

mramato commented Jan 29, 2014

pjcozzi commented Jan 29, 2014

pjcozzi commented Jan 29, 2014

emackey commented Jan 29, 2014

shunter Jan 29, 2014

Choose a reason for hiding this comment

emackey commented Jan 29, 2014

mramato commented Jan 29, 2014

shunter Jan 29, 2014

Choose a reason for hiding this comment

mramato commented Feb 1, 2014

mramato Feb 1, 2014

Choose a reason for hiding this comment

mramato Feb 1, 2014

Choose a reason for hiding this comment

emackey Feb 2, 2014

Choose a reason for hiding this comment

emackey Feb 2, 2014

Choose a reason for hiding this comment

mramato Feb 2, 2014

Choose a reason for hiding this comment

pjcozzi commented Feb 3, 2014

mramato commented Feb 3, 2014

emackey commented Feb 3, 2014

pjcozzi commented Feb 3, 2014

mramato commented Feb 3, 2014

emackey commented Feb 3, 2014

mramato commented Feb 3, 2014

mramato commented Feb 3, 2014

mramato commented Feb 3, 2014

emackey commented Feb 3, 2014

mramato commented Feb 3, 2014

emackey commented Feb 3, 2014

mramato commented Feb 3, 2014