Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(track): ensure places node list will update when track name changes #1116

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

fix(track): ensure places node list will update when track name changes #1116

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/plugin/track/track.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,7 @@ plugin.track.updateTrackZIndex = os.track.updateTrackZIndex;
*
* @param {!os.track.CreateOptions} options The options object for the track.
* @return {os.track.TrackFeatureLike|undefined} The track feature.
* @suppress {accessControls} for onFeatureChange access
*/
plugin.track.createAndAdd = function(options) {
var track = os.track.createTrack(options);
Expand All @@ -479,6 +480,7 @@ plugin.track.createAndAdd = function(options) {
var trackNode = plugin.file.kml.ui.updatePlacemark({
'feature': track
});
ol.events.listen(track, goog.events.EventType.PROPERTYCHANGE, trackNode.onFeatureChange, trackNode);
Copy link
Contributor

@jsalankey jsalankey Dec 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

At a glance, the updatePlacemark call is creating a plugin.file.kml.ui.KMLNode which is already listening internally to this event in plugin.file.kml.ui.KMLNode.prototype.setFeature. That feature change handler seems like the place where you would want to listen for updates to the name property.

One point of potential confusion here: we have out own PROPERTYCHANGE events that we handle independently from the Openlayers events that fire whenever a property on an ol.Object is changed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I was thinking of the feature in plugin.file.kml.ui.KMLNode.prototype.setFeature as being the underlying linestring, rather than conceptually being the track. Will take another look.

On the second point, is there any guidance / conventions on which events get used where? Failing that, for a name or other track-level attribute, which should I use?

Copy link
Contributor

@jsalankey jsalankey Dec 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The feature in a KML node is sort of a data wrapper around a geometry, style, and a bunch of other metadata and utility for drawing the geometry to the map and interacting with it. Generally speaking, features and geometries have a 1 to 1 mapping, though through the use of styles, you can have multiple geometries on a single feature. I don't think tracks ever do this, but they do use a special feature class called DynamicFeature which is how they update their geometries with changes in the timeline.

We generally have a convention of not using Openlayers' internal events whenever possible. They fire a lot of them, they add listeners for everything, and are generally unoptimized as a result. By default, Openlayers fires an event on every change to a property, which is a behavior we disable via mixin because it's just unnecessary overhead that we don't care about 99% of the time. In lieu of that, we'll fire our own property change events for things we know we care about.

In this case:

  1. Set the name property on the Feature instance.
  2. Call feature.dispatchEvent(new os.events.PropertyChangeEvent('label').
  3. Add handling for a 'label' property change in KMLNode that sets the new label value on the node from the feature.
  4. Fire the PropertyChangeEvent onward from the KMLNode. This bubbles up the tree to the root, where the tree itself listens and should re-render itself on label change.


var rootNode = plugin.places.PlacesManager.getInstance().getPlacesRoot();
if (rootNode) {
Expand Down