Expose TimelineHighlightRanges

[jadthegerbil]

When using timeline.addHighlightRange, the color passed to it is ignored, and instead a blue is displayed.

Poking into it, it seems that TimelineHighlightRange.render is doing this._color when it should be this._color.toCssColorString()

Sandcastle example: here

Browser: Firefox 68.7 Operating System: Windows 10

[YonatanKra]

@jadthegerbil - addHighlightRange is a private function. It has no documentation and it is not an exposed API. Hence, this is not an issue since it is not supposed to work with cesium colors ATM. Maybe one of the core team members could you tell if there is any plan of exposing this feature? @hpinkos @mramato ? If so, I could send a PR that exposes it, adds doc and relevant tests for this feature.

[mramato]

So the Timeline widget is the oldest and least documented or tested widget in CesiumJS. It's one of those things where we always planned on rewriting and cleaning up, but never got around to it. It has been relatively unchanged since it was first introduced years ago.

@YonatanKra While I can't speak for the entire team, if you want to clean up what's there and doc/test/expose it, that would be great. Even though it has private functions, I know the community has made some heavy use of it over the years, so I'm sure everyone would welcome to formalization.

@emackey may have some thoughts or be able to provide some advice here if you have questions, since he wrote originally wrote it.

[emackey]

addHighlightRange has always taken a CSS string as a parameter, not a Cesium.Color. Actually I think the timeline predates Cesium.Color, or at least wasn't in the same project as it when first written.

If you want to change it to take a Cesium.Color, that's probably a good idea. It's a breaking API change, not a bugfix. Private/undocumented API is allowed to change without a deprecation period in Cesium, but this has been around from the start and is used across a variety of products as there's no documented alternative currently. Perhaps there could be a deprecation period where the function takes either a CSS string or a Cesuim.Color for a little while.

[mramato]

Timeline has definitely been around long enough that if we tweak the API to make it more inline with CesiumJS, I think we should treat it as a deprecation.

[YonatanKra]

@mramato @emackey I'll be happy to help with this one. Just so we are on the same page here - the change is going to be:

1. Allow to send addHighlightRange Cesium.Color
2. Cover with tests and expose as public
3. Add documentation Correct?

[jadthegerbil]

@YonatanKra Would this include documenting--or at least explaining the difference between--addTrack?

[YonatanKra]

@jadthegerbil It will document the exposed API. If you add addTrack to this issue, I'll expose and document it as well :)

[YonatanKra]

@jadthegerbil Looking at this more thoroughly, I think this issue is big enough to be left only for the highlighter. So addTrack will take another issue and PR. @mramato @emackey Some things need to be decided here:

1. Do we want to expose TimelineHighlightRange as well? Currently the API returns it but it is private, so if we do not want to create a breaking change, we have to assume this is exposed as well. The other solution would be to create a new API that will be a level of abstraction for TimelineHighlightRange.
2. Currently, just setting the range is not enough to actually render the highlight - should we expose a different API for this? For instance - add a 4th parameter renderNow that will invoke the render upon creation?

[emackey]

For 2, don't add an extra parameter. My vote would be to change it so it renders the highlight immediately.

I don't think I have an opinion on 1.

[YonatanKra]

@emackey - Regarding 2: I dived a bit deeper here. There's a line of code that's supposed to render the highlight (the whole timeline actually) - but it will work only if the timeline resized. That's why it did not render immidiately. I've created another method render that resize is using under the hood. The actual rendering is done via render. resize calls render only if the size really changed.

/**
 * Rerenders the widget only if the size has changed
 */
Timeline.prototype.resize = function () {
  var width = this.container.clientWidth;
  var height = this.container.clientHeight;

  if (width === this._lastWidth && height === this._lastHeight) {
    return;
  }
  this.render();
};

The addHighlightRange will use render instead of resize.

That's actually a bug, that's not related to this issue originally, but pretty easy to fix so can be implemented with this change.

[YonatanKra]

@emackey @mramato : We have 2 main options here.

Option 1

The API for this would have to change if I do not expose the highlightRange object itself. Proposed changed:

1. addHighlightRange will accept a range (start/end times) and return a highlightRangeId.
2. I'll add a removeHighlightRange that will accept and ID and remove it.
3. I'll add an updateHighlightRange that will accept and ID, range, height and color and update the relevant highlight.
4. Change TimelineHighlightRange to accept Cesium.Color.

Option 2

The other option would be to just leave it as is (return the TimelineHeighlightRange object) and just allow to send it a Cesium.Color.

I've started implementing option one (changing the whole API) - let me know what you prefer.

[mramato]

I expect this to be a "pull one thread" situation from the beginning, so I'm perfectly fine with you cleaning up and tweaking the API, as long as we follow the deprecation policy as if it were already public. I also would be shocked if you did not find several bugs in the process :smile:

Like I said, this code is ancient by web standards and hasn't been touched in years.

[mramato]

Also, thank you! Love to see someone deciding to look more into this.

[emackey]

Yes, thanks for looking into this!

Just curious, what's the down side of making TimelineHighlightRange public? The render function on it would presumably need to stay private, but it looks like most of the rest could be public, I think.

With Option 1, if the highlight range gains new features, you'll need more proxy functions on the main timeline to update ranges. I'm fine with either option though.

[mramato]

I think exposing more of the Timeline as public is okay as well. I'd definitely rather see us expose the objects that are there rather than make changes to the API to expose functionality without exposing objects.

[YonatanKra]

@emackey @mramato I've added a PR (definitely not passing) of just the tests for the new API. https://github.com/CesiumGS/cesium/pull/8845 I'll start implementation on weekend probably.

Edit: Already implemented. You can check :)

[ggetz]

This ability was requested on the forum: https://community.cesium.com/t/i-would-like-to-highlight-the-timeline/34038/2