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

Markers API #87

Closed
2 tasks
thom4parisot opened this issue Sep 25, 2014 · 7 comments · May be fixed by #180
Closed
2 tasks

Markers API #87

thom4parisot opened this issue Sep 25, 2014 · 7 comments · May be fixed by #180
Labels
enhancement

Comments

@thom4parisot
Copy link
Contributor

thom4parisot commented Sep 25, 2014 •
edited
Loading

tl;dr

This API replaces the Segments and Points API.

Why?

Segments and points contains basically the same data. They differ only in their rendering aspect.
Hence a very similar codebase.

Also, we want to deal with multiple/optional layers of data, to toggle them.

Goals

  • light object structure
  • data to rendering approach
  • a marker = timing informations + free structure (meta)data + rendering function
  • view system to narrow the selection per waveform view

Example

var p = Peaks.init({
  // ...
});

// add a marker
p.markers.add({
  timestamp: 30.5,
  duration: 20,
  data: { name: 'John Reith', url: 'http://dbpedia.org/page/John_Reith,_1st_Baron_Reith', labels: ['speaker'] }
});

// add another one which is non-editable, with a custom shape
p.markers.add({
  timestamp: 30,
  editable: false,
  data: { comment: '...', pid: 'b01mxx3h' }
  shape: (segment, view) => {
    const shape = new Konva.Shape({ ... });
    shape.sceneFunc(...);

   if (view.name === 'overview') {
     return false;
   }

    return shape;
  },
});

// add views ...
p.markers.views.speakers = (marker) => marker.data.labels.indexOf('speaker') !== -1);
p.markers.views.comments = (marker) => 'comment' in marker.data;

// fetched by view
p.markers.views.speakers.forEach(marker => { ... });

// all of them
p.markers.forEach(marker => { ... });

// change some data in markers and force update their rendering
p.on('marker.click', (event, view, markers) => {
  markers
    .filter(view.objectInView)
    .forEach(marker => { ... });

  markers.render();
});

Work

  • Definition of the features
  • Definition of the API

closes #64
@chainlink
Copy link
Contributor

👍

@thom4parisot
Copy link
Contributor Author

@chrisn I amended the API definition to make it more lightweight and able to fit in people existing data structures, possibly by referencing data rather than having to duplicate them in memory.

What do you think?

@chrisn
Copy link
Member

chrisn commented Dec 21, 2016

Looks good, a few thoughts below:

  • Another goal is to allow the number of views to be configurable (Disabling the zoom view #2).
  • Specifing startTime and endTime for segments is preferable to time and duration, although for convenience we could allow both. I also suggest renaming timestamp to time for point markers, for consistency.
  • To allow applications to select which markers are shown in which views, we could introduce a type attribute, then add a setSegmentVisibility(type, boolean) method to the view classes. This is simpler than the callback approach for the most common use case. (Allow configuration of segment appearance #88).
  • We should still allow applications to set marker IDs, as per the existing API.
  • We should still provide some default rendering behaviour for label text.
  • In making this change, we should also try to fix the performance issue in updateSegments (updateSegments function is too slow #117).

There are a few other issues in the rendering that we should fix:

  • Point markers a currently not visible at all unless editable.
  • The start position of a segment whose start time is zero cannot be changed, because the handle is drawn outside of the canvas.
  • The handles for adjacent segments overlap each other, making editing difficult.

This was referenced Dec 21, 2016
Closed
Closed
Closed
Closed
Open
Closed
Open
@amrfaisal
Copy link

Will this also help with attaching something like editor notes/comments? it may change dynamically after the initialization of the segment.

@chrisn
Copy link
Member

chrisn commented Jan 25, 2018

@amrfaisal I suggest adding your requirements as a new issue. We may want to address that separately to the proposed change described in this issue.

@zachsa zachsa mentioned this issue Dec 18, 2018
Closed
@chrisn
Copy link
Member

chrisn commented Feb 2, 2020

I'm going to close this, as we have a new marker API (although somewhat different to this proposal) in v0.18.0, which is intended to give applications more control over marker rendering - see Customizing Peaks.js. Please re-open or raise a new issue if there are any limitations or use cases the new API doesn't support.

@chrisn chrisn closed this as completed Feb 2, 2020
@thom4parisot
Copy link
Contributor Author

Amazing 🙂 Thanks for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[WIP] Unify Points and Segments API as Marker API
4 participants
@chrisn @thom4parisot @chainlink @amrfaisal