New feature discussion

[ffxsam]

Hey @chrisn!

I wanted to start a discussion here about some new stuff I've been thinking about adding to Peaks.js. Let me know your thoughts on these:

Mousewheel/trackpad scrolling

We've talked about this and agree it's a good idea. I'm not sure if it necessarily needs to be disabled by default, though. As long as the Peaks.js version number is increased so as to be a breaking change, I think it would be fine. I think most people would expect that mousewheel/trackpad gestures would scroll the waveform (I was surprised to find that it required clicking and dragging).

Allowing click/drag to draw a region

Adding new regions is a little clunky at the moment. I find it more intuitive to be able to click and drag to draw a region. This option would obviously need to be off by default because it would conflict with click/drag scrolling. Unless gesture scrolling becomes the default, then maybe it would be ok.

More events for zoomview interaction

In my fork, I've added zoomview.mousedown, zoomview.mouseup, and zoomview.drag to allow me to create the region drawing feature mentioned above. IMO, adding more events for flexibility is never a bad thing! So if we didn't add region drawing, people could implement it themselves the way I have by using these events.

More precise player.timeupdate events

Now that we're using requestAnimationFrame(), it might be good to have that loop emit the player.timeupdate event rather than the HTML audio timeupdate native event. Use case: I'd like to add the ability for segment playback to fade in and out, but to do so, I need more fine-grained reports of the playback position so I can make really fast volume adjustments. Alternatively, we could add a new event like player.fastupdate (or whatever) that would be emitted by the requestAnimationFrame() loop.

Ability to hide the axis completely

I added a hideAxis option to my fork which hides the axis from the waveform, which simply skips any self._createAxisLabels() calls. Any objections to making this option official?

[ffxsam]

Oh, and long-term: what do you think of the idea of modernizing the repo and refactoring to 100% TypeScript?

[chrisn]

Possibly, yes. Not necessarily TypeScript, but modernising the build system could be good. I'm ambivalent on using modern JS features, and introducing transpilers makes for slower builds. I'd much prefer to do this myself than review a PR, though.

[ffxsam]

For sure, and I understand! If I were going to take on a huge endeavor like that, I'd want to do it myself too.

[chrisn]

I have work in progress that replaces Browserify with Rollup and ES modules

[chrisn]

Yes, those all sound like good additions. I need to look at rAF usage. Because of the extra CPU load it introduces I only really want to use it where needed. For example, we have code that switches the playhead animation between rAF and timeupdate, depending on whether the extra precision is needed.

[ffxsam]

Just to clarify: I didn't mean adding more rAF calls. Since you're already using requestAnimationFrame() here:

peaks.js/src/player.js

Lines 194 to 213 in cffed82

	Player.prototype._playSegmentTimerCallback = function() {
	if (!this.isPlaying()) {
	this.pause();
	this._playingSegment = false;
	return;
	}
	else if (this.getCurrentTime() >= this._segment.endTime) {
	if (this._loop) {
	this.seek(this._segment.startTime);
	}
	else {
	this.pause();
	this._peaks.emit('player.ended');
	this._playingSegment = false;
	return;
	}
	}
	window.requestAnimationFrame(this._playSegmentTimerCallback);
	};

I thought we could slip a this._peaks.emit('player.fasttimeupdate') for people like me who want to get more rapid/precise position updates. Maybe that emitter would only fire if there's an option passed to Peaks.init() so that it would have to be manually enabled. There could be a warning in the docs about potential CPU usage—though I thought the whole idea behind rAF was that it's easy on CPU because the rate of looping varies based on the current system load.

Thanks for your feedback! I hope to get to these changes in the near'ish future. I mostly wanted to have a discussion on record that I could refer to later when I have time.

[jgentes]

I'll add one to the list - it's an easy patch but seems like a good default or option to provide:

// adjust zoom view when mouse wheel is used
peakOptions.containers.zoomview.onwheel = e => {
  e.preventDefault()
  e.deltaY === 100 ? waveform.zoom.zoomOut() : waveform.zoom.zoomIn()
}

[chrisn]

We have two proposals for mousewheel: either scrolling the waveform or adjusting the zoom, so there'll need to be a way to configure which to use.

[ffxsam]

My two cents: mousewheel should be assigned to scroll/panning the waveform, hands down. Having worked with audio editing software for ages, this is how they all function, and most pro audio users would expect this. Shift+mousewheel could be used for zoom, as this is not uncommon. I had this in my own fork originally, but it was rather slow so I commented it out:

        // Vertical scroll? If so, zoom
        // if (event.shiftKey) {
        //   var seconds = self._peaks.player.getDuration();
        //
        //   if (!Utils.isValidTime(seconds)) {
        //     return;
        //   }
        //
        //   var maxScale = self._getScale(seconds);
        //
        //   self.throttledSetZoom({
        //     scale: Utils.clamp(self._scale + event.deltaY * 16, 64, maxScale)
        //   });
        // }
        // else {

[jgentes]

I like the idea of having both options. It doesn't matter to me what the default is, I think it will differ based on the use case.

[chrisn]

I like the idea to add an option to hide the axis, but the options become awkward if there's a need to configure this differently for the zoomview and overview waveforms. I'm thinking of changing the options structure anyway to support other features, so I'll raise another discussion about that.

[ffxsam]

Sounds good!

[chrisn]

Discussion on options structure: #387. I've gone ahead and implemented in 93c7024. Next step is to update the TypeScript declarations.

[jdelStrother]

Allowing click/drag to draw a region

Any thoughts on handling markers? I like click-drag for drawing segments, so single-click seems like the obvious choice for adding a marker, except it's overloaded by the single-click to set the play head.
Maybe shift-click for setting a marker?

[jdelStrother]

Allowing click/drag to draw a region

I hacked together a POC for this here - jdelStrother@drag-segments.

• It assumes you've enabled { zoomview: { wheelMode: "scroll" } } - if you try to scroll by dragging, you're going to just create a segment instead.
• I've skipped configurable options for simplicity. At the very least it would need something like options.zoomview.dragBehaviour = "new-segment" | "drag-waveform"

I'm not totally sure how it ought to ideally work, though. My app expects to be able to control the ids of peaks' segments, so here the drag handler is creating a temporary segment with an arbitrary id, broadcasting a segments.drag-segment-complete event on mouseup, and then deleting the temporary segment. In the meantime, my app watches for the segments.drag-segment-complete event, and creates the "real" segment which instantly replaces the temporary drag one.

I could see arguments for other ways, though. eg some clients might want to be able to specify other attributes (color, id, etc) on the new segment immediately as the drag starts, but we'd then need to add a callback from zoomview's drag handler back out to client code.

Or we forget about creating segments within zoomview's drag handler, and instead emit zoomview.dragstarted, zoomview.dragging, & zoomview.dragended events. Then clients could disable drag-to-scroll in a config option, and listen to the drag events and create segments themselves.

Any thoughts?

[jdelStrother]

OK, I've updated my drag-segments branch with a slightly more fit-for-purpose proposal. It no longer does the auto-deletion of the segment once the drag finishes, and has options to opt-in to the new behaviour.

The names could definitely use some bikeshedding, but I've added two new options:

• zoomview.dragBehavior: "scroll" | "create-segment" | "none". Defaults to 'scroll' (the original behaviour), but can be set to 'create-segment' to allow the drag-to-create-a-segment behaviour.

• zoomview.dragSegmentAttributes: (attrs: SegmentAddOptions) => SegmentAddOptions | null - an optional callback that's fired when the drag-to-create-a-segment starts. It can return properties for the new segment (eg specifying a color or id), or it can return null to cancel the drag.

It doesn't currently emit segments.dragStart / segments.dragged / segments.dragEnd events. Do we think it should? Or should it have separate events for these?

(Aside: I'm not sure if you had strong feelings about peaks.segments.add({...}) returning undefined. If you're relying on an auto-generated segment id, it was kind of tricky to then do much with the new segment after (because you don't know what the id was unless you added an event listener to the segments.add event), so I changed it to return the new segment here: 4f9d3ba)

I'm not sure I follow how your alternative approach with just events would work though.

I worry that this approach is going to get 90% of the way there for many people, but everyone's going to want their own individual conflicting tweaks on it. For example, in my own app I'm thinking of having it so that the dragged-out-segment is only presented as a placeholder selection, and it wouldn't be added as a 'real' segment until the user clicks a button. Maybe it would be easier and more flexible to forget about creating segments within Peaks' zoomview code, emit events like zoomview.dragging, and let the user create their own segments if they want.

[chrisn]

One initial thought: This probably shouldn't be an option setting. I'd prefer to enable 'create-segment' mode via a method call, because I would ordinarily want to use drag to scroll the waveform

[jdelStrother]

Yeah, maybe. zoomview.dragBehaviour could be a callback, then? zoomview.dragBehavior: () => "scroll" | "create-segment" | "none". Not sure if there's useful arguments that we'd pass to the callback - x/y position & time, maybe?

Would that just be called on the initial mousemove of the drag event? Or would we have multiple callbacks for dragStart/dragged/dragEnd? (IMO, just a callback on initial mousemove to decide whether we want to scroll or create a segment, but would still like to emit some sort of dragStart/dragged/dragEnd events)

[chrisn]

I was considering this as another mode option, e.g: view.setDragMode('scroll' | 'segment'). Then the app UI could provide buttons to toggle between these modes. Callbacks would be needed if the drag/scroll decision needs to be made dynamically, but I'm not sure that it would.

On segments.add, it's fine to change this to return the Segment objects that were added (points.add should then do similar).

A minor thing, I noticed trying out your code (using the default segment markers) is that the time label appears and disappears while dragging, depending on if you hover the mouse over the drag handle. I wonder if we should force the label to be shown while it's being dragged.

[jdelStrother]

A minor thing, I noticed trying out your code (using the default segment markers) is that the time label appears and disappears while dragging, depending on if you hover the mouse over the drag handle. I wonder if we should force the label to be shown while it's being dragged.

Yeah, this turns out to be a little tricky. The segment.update here triggers the segments.update event, which results in the segment-shape being destroyed and recreated here

I'm not entirely sure of a good way of fixing it. It would be nice if ZoomView could pass off the drag-operation to the newly created segment (so that SegmentShape receives the subsequent mousemove & mouseup events as if they were a continuation of a normal marker drag), but that looks quite hard.

Alternatively, markers (and anyone writing custom markers) need to be aware of whether they're currently being dragged within the Marker.init function, so that they can immediately show their label. Then we can destroy & recreate SegmentShapes & their markers all the time, but it will look as though it's a single continuous marker.

[chrisn]

Allowing click/drag to draw a region

I'm starting to work on this, will follow up in issue #499.

[michelmaldonado]

Hi @chrisn, I'm a big fan of Peaks.js. I'm using it in a hobby project. I saw that you're working on the insert-segment mode, that's great.

I've got 2 questions:

• Is the insert-segment mode already usable? (I've tried to update the package, but the setWaveformDragMode wasn't there, not sure what I'm doing wrong, tried 3.0.0-beta.12 and v2.1.0)
• For my project it would be great if the Overview Highlight layer can be dragged at the left or right edge, (see video attachment) so the user can zoom the Zoomview with these Highlight layer edges. Is this an idea that you like?

I've managed both of these wishes by hacking around your code, it works but it is not ideal. Thank you very much for this great library!

peaks.mp4

[chrisn]

Thanks! I haven't released a new version to npm yet. It's usable if you clone the repo and build locally. I want to add a few test cases before releasing.

On dragging the highlight region, I'd be interested to see your code. We had a similar feature on an old branch, but we never get it into a state I felt was ready to release. The main concern I had was that this triggers a lot of resampling of the waveform data, which may be slow, particularly for long files.

[michelmaldonado]

Thanks for your quick answer! I'm using React hooks
react-hook.txt

The ugly override:
draghandler-override.txt