Add first-class support for vertical orientation #2195

valrus · 2021-02-24T07:13:39Z

Short description of changes:

This PR implements support for rendering the waveform in a vertical rather than horizontal orientation. To do this with as little disruption to the existing code as possible, I use two main approaches:

A utility object, Orientation, has two subclasses. HorizontalOrientation is basically a no-op. VerticalOrientation contains a mapping of "horizontally-oriented" attribute names to "vertically-oriented" ones (e.g. "width" becomes "height"). We can replace references to attributes in the DOM with calls to the attrFor method to ensure we use the correct attributes regardless of orientation, while preserving the "horizontally oriented" variable names in the code and without festooning the codebase with if (vertical) or some such.
Actual canvas drawing is done by transformations on the context, so that we don't need to swap x's and y's every time we draw something in vertical mode.

Breaking in the external API:

Nothing that I'm aware of. Default params all still assume horizontal orientation so none of their names have changed. This code caused no test breakages.

Breaking changes in the internal API:

CanvasEntry now takes an Orientation parameter. This might be avoidable by having entries refer to the parent drawer's orientation instead, if need be.

Todos/Notes:

I'm submitting this PR in an incomplete state because while I tried to keep it minimally disruptive, it does introduce an additional layer of indirection which might be a deal breaker to merge it. I wanted to see if the overall approach seemed mergeable before doing the following:

Porting more plugins. I ported the regions one since it's what I intend to use myself but I'd like to port the rest as well. It's a pretty mechanical process — just add an attrFor everywhere we touch the DOM — but a bit tedious.
Adding tests. Vertical orientation seems likely to cause regressions if future code changes omit an attrFor call somewhere, so tests are important. But I don't know the test framework so wanted to put that off until I know whether this might be merged.

Related Issues and other PRs:

#634 is a request for this feature and the thread contains various hacks people used to get it sort-of working, mostly based on CSS transforms. AFAIK, all these approaches worked only partially at best, and they also have the problem of CSS transforms causing the waveform to be rendered perpendicular to the actual container.

coveralls · 2021-02-24T07:17:59Z

Coverage decreased (-0.2%) to 82.219% when pulling 8b4a2c8 on valrus:vertical-orientation into 861d52a on katspaugh:master.

thijstriemstra

thanks, really cool idea and good to have!

example/vertical/index.html

src/drawer.canvasentry.js

src/drawer.js

src/wavesurfer.js

package.json

src/drawer.canvasentry.js

src/drawer.js

src/drawer.multicanvas.js

src/plugin/regions/index.js

src/util/capitalize.js

src/util/orientation.js

src/drawer.canvasentry.js

src/plugin/regions/index.js

thijstriemstra

Can you also add unit tests?

thijstriemstra

great! one thing left.

src/wavesurfer.js

src/drawer.multicanvas.js

CHANGES.md

basically, all references to width, height, x, y, etc. should pass through the Orientation object to ensure functionality for both horizontal and vertically oriented elements

...and to canvasentry, by applying the appropriate transform

...we only need to translate attributes based on orientation when we're touching the DOM. additionally, we can use canvas transforms when drawing, so we don't need to translate there either. so just use an attribute mapping for DOM access

...also, remove element param to makeOrientation as it's not used

...to avoid re-proxying and to enable us to get the proxied element, since we can't add proxies to the DOM

...they're not used in wavesurfer, but for consistency if "outside" code uses them

...to separate them out into a different PR

thijstriemstra

thanks @valrus for sticking through the review, looks good to me.

ps. can you add @valrus to the contributors/team @katspaugh?

thijstriemstra · 2021-04-05T13:47:35Z

@valrus if you could open the regions plugin PR in next 48 hours, we can include it in 4.7.0, otherwise it'll be in a next release.

katspaugh · 2021-04-05T17:24:30Z

@valrus I've sent you an invite, welcome! 🎉

@SInCE

* Add Orientation object for use in measurements... basically, all references to width, height, x, y, etc. should pass through the Orientation object to ensure functionality for both horizontal and vertically oriented elements * Add orientation handling to multicanvas... ...and to canvasentry, by applying the appropriate transform * a new way of handling orientation... ...we only need to translate attributes based on orientation when we're touching the DOM. additionally, we can use canvas transforms when drawing, so we don't need to translate there either. so just use an attribute mapping for DOM access * Add an example of a vertical waveform * Do all canvas context ops in the same place * Implement orientation for regions plugin... ...also, remove element param to makeOrientation as it's not used * Remove test regions from vertical example * Use Proxy to avoid explicit attr conversions * Fix various PR-flagged issues * Comment on magic number * Explain canvas transform * Add Proxy to linter and no-op withOrientation if we lack it * Add some meta-functionality to withOrientation... ...to avoid re-proxying and to enable us to get the proxied element, since we can't add proxies to the DOM * Convert region plugin to orientation proxy * More docs for orientation * Remove capitalize and add @SInCE to orientation * Add vertical drawer unit tests * Document and add default for vertical parameter * Add vertical plugin compat comment * Move vertical plugin compatibility note to CHANGES.md * Removing regions needs to remove the proxied element * Add a couple more attribute conversions... ...they're not used in wavesurfer, but for consistency if "outside" code uses them * Revert region plugin changes... ...to separate them out into a different PR * Rename proxiedElement -> domElement * Remove plugin note from CHANGES * Add back issue number for vertical param in CHANGES * use var in example

thijstriemstra suggested changes Feb 26, 2021

View reviewed changes

thijstriemstra added the enhancement label Feb 26, 2021