Split channels: overlay param now properly displays a single canvas

[johnpaulmedina]

fixes #2161

When setting overlay: true on split channels the height of the waveform becomes equal to a single channel height but does not overlay the second channel.

The issue is that the offsetY value is set based on height multiplied by the drawIndex or default to zero. In this case we should check for the splitChannelOptions.overlay and set the offsetY to zero regardless of the drawIndex.

[coveralls]

Coverage decreased (-0.05%) to 82.407% when pulling 151365a on johnpaulmedina:master into 8cc4bbd on katspaugh:master.

[thijstriemstra]

Thanks! I am slightly frustrated that the split channel option is adding all these 'workarounds' and conditionals to MultiCanvas, and cluttering the API. This is not an issue to solve in this PR but something to think about..

So, I'll let others review this PR first.

[johnpaulmedina]

@thijstriemstra I see what you mean. It sure did feel like a work around when adding the conditional statement. However, I was doing it based on previous conditionals within the prepareDraw method.

Just for thought. It looks like the split channel interacts with:

filteredChannels
overallAbsMax
offsetY

What if there are separate methods to that specifically handle these logics based on the option params? Doing so would allow for the prepareDraw method to be less conditional, keeping each action contained within it’s own method but still handling the correct output based on option params.

So having a class for channels and then each channel can have its own prototype inheritance

channel.isFiltered()
channel.getOverallAbsMax()
channel.getOffsetY()

Additionally, splitChannelOptions feels unnecessary in my opinion.

splitChannels could be a Boolean || String = ‘overlay’
filteredChannels should work at the higher param option level altogether because correct me if I am wrong here - but I don’t believe it supports nested channels?
channelColors could work from the higher param option waveColor and progressColor respectively. So those params should be either a String or Array where their index position corresponds to a channelIndex.
relativeNormalization also seems redundant since at least in my initial first thought because either we normalize or we don’t.

These are subjective opinions since I am new to wavesurfer and audio handling altogether but might help clean up options and the MultiCanvas API as you pointed out.

[thijstriemstra]

Thanks for the feedback. What about adding an additional subclass of MultiCanvas where all SplitChannel stuff happens, e.g. SplitChannelMultiCanvas or someting like that. All your suggestions could be implemented there.

[sundayz]

@thijstriemstra I'm trying to test this but when i run npm run start:plugins I get an error, is this caused by #2093 or is it just me?

> wavesurfer.js@4.4.0 build:htmlinit F:\dev\wavesurfer.js
> webpack --config ./build-config/webpack.prod.htmlinit.js

asset wavesurfer-html-init.js 13.3 KiB [compared for emit] (name: html-init) 1 related asset
./src/html-init.js 9.41 KiB [built] [code generated]
./node_modules/load-script/index.js 1.6 KiB [built] [code generated]
webpack 5.17.0 compiled successfully in 2981 ms
[webpack-cli] Invalid configuration object. Object has been initialized using a configuration object that does not match the API schema.
 - configuration has an unknown property 'publicPath'. These properties are valid:
   object { bonjour?, client?, compress?, dev?, firewall?, headers?, historyApiFallback?, host?, hot?, http2?, https?, injectClient?, injectHot?, liveReload?, onAfterSetupMiddleware?, onBeforeSetupMiddle
ware?, onListening?, open?, openPage?, overlay?, port?, proxy?, public?, setupExitSignals?, static?, stdin?, transportMode?, useLocalIp? }

[thijstriemstra]

You're using webpack 5.17.0 compiled successfully in 2981 ms but package.json states webpack 5.13.0. Can you use the deps as specified by wavesurfer.js instead?

[sundayz]

I wiped node_modules and installed everything again and it gave me 5.17 again. I'll try again later...

The PR is 1 line so I trust that if John says it works then it works 😃 the code looks good to me.

[johnpaulmedina]

I think the issue with the subclass just for split channels might be too much because it would need to be instantiated from the original prepareDraw of multicanvas ?

I would vote to favor a ChannelClass where it has its own logic that can clean up the original prepareDraw.

Keeping channel conditionals and logic specifically with a channel.

I’ll take a look a little deeper later, I was just throwing out some ideas arbitrarily without looking at the entire code base

[thijstriemstra]

instantiated from the original prepareDraw of multicanvas ?

Haven't looked into specifics yet.

I would vote to favor a ChannelClass where it has its own logic that can clean up the original prepareDraw.

Can you elaborate, maybe some pseudo-code example?

[sundayz]

Build works fine it's probably just a problem on my end, I'll clone everything again later, didnt mean to derail your PR with that

[thijstriemstra]

Build works fine it's probably just a problem on my end, I'll clone everything again later, didnt mean to derail your PR with that

Please open a new ticket, I'm not able to run it either, something with an ipv6 address..

<i> [webpack-dev-server] Project is running at http://[::]:8080/

[johnpaulmedina]

Ah ok. Well it runs for me using npm run start. However I noticed you have a double /dist/ on the ./build-config/fragments/dev.js because when using path.resolve you are going up the tree twice which brings you to the root but on your publicPath you are expecting /dist/ so that would mean: http://localhost:8080/dist/dist

@thijstriemstra in regards to the IPV6 that serving from there should work for localhost,127.0.0.1, or ipv6 of the machine

Just change the path.resolve to: path.resolve(__dirname, '..', '..', 'dist')

/* eslint-env node */

const path = require('path');
const rootDir = path.resolve(__dirname, '..', '..');

module.exports = {
    mode: 'development',
    devtool: 'eval-source-map',
    output: {
        publicPath: 'localhost:8080/dist/'
    },
    devServer: {
        static: [
            {
                directory: path.join(rootDir, 'dist'),
                staticOptions: {},
                publicPath: '/dist/',
                serveIndex: true,
                watch: {
                    ignored: [
                        /.chrome/,
                        /node_modules/,
                        /bower_components/,
                        /coverage/,
                        /docs/,
                        /spec/
                    ]
                }
            }
        ]
    }
};

[thijstriemstra]

@johnpaulmedina thanks for the feedback, please open a separate pr for that fix if you can.

[johnpaulmedina]

@thijstriemstra So I had opened a separate PR, however the forked repo was originally just for the bug fix so I had pushed on the master branch. Since I had opened this PR essentially my master branch already contains the above changes and since we had already discussed the webpack issue here - I closed the other PR #2172 and and pushed here.

Sorry if that messes up your flow.

[EDIT] - I'll do it the right way. Then get back to my regular work.
[EDIT] - PR for webpack #2173

[thijstriemstra]

thanks, can you also add a changelog entry (with reference to #2161)?

[johnpaulmedina]

All set

[johnpaulmedina]

Can we get this merged in?

[thijstriemstra]

thanks.