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.

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

"for... in" should not be used with arrays. This causes bugs! #1081

Merged
merged 26 commits into from
May 15, 2017
Merged

"for... in" should not be used with arrays. This causes bugs! #1081

merged 26 commits into from
May 15, 2017

Conversation

agamemnus
Copy link
Contributor

See: #1014.

I also changed "entry" to "canvas" since they are canvases. And, uh, just a few styling ideas.

@katspaugh
Copy link
Owner

Could you just replace the for-in with a forEach without the other stylistic changes? Btw, forEach takes a third argument which is the context of the callback, so you could pass this and not introduce another var my = this to keep the context.

@agamemnus
Copy link
Contributor Author

agamemnus commented May 6, 2017

I already ended up removing many of those changes due to your draconian style checker! The rest is as follows --

There are only four differences now, other than the forEach fixes, and corresponding "my" variables -- the "my" variables isn't something I came up with as it was present in an existing function.

  1. Some brackets had an errant starting space: e.g.: { width: elementWidth + 'px'}. and the getImage function didn't have a space between it and the parameters.
  2. entry -> canvas and lastEntry -> lastCanvas. IMO, if you have this.canvases, then one of them should be a canvas and not an entry.
  3. Some breaking up of a multi-line var:
var elementWidth = Math.round(width / this.params.pixelRatio),
totalWidth = Math.round(this.width / this.params.pixelRatio);

to

var elementWidth = Math.round(width / this.params.pixelRatio);
var totalWidth   = Math.round(this.width / this.params.pixelRatio);

and

var entry = {},
leftOffset = this.maxCanvasElementWidth * this.canvases.length;

to

var canvas = {};
var leftOffset = this.maxCanvasElementWidth * this.canvases.length;
var endCanvas = Math.min(Math.ceil((x + width) / this.maxCanvasWidth) + 1,
this.canvases.length);

to

var endCanvas = Math.min(Math.ceil((x + width) / this.maxCanvasWidth) + 1, this.canvases.length);

Thanks for your work!

@katspaugh
Copy link
Owner

While some of your changes make sense (I'll mark them with a thumbs-up in the changeset), some are arbitrary, and I would rather not create more maintenance work for the next branch.

@@ -52,24 +53,24 @@ WaveSurfer.util.extend(WaveSurfer.Drawer.MultiCanvas, {
this.removeCanvas();
}

for (var i in this.canvases) {
my.canvases.forEach (function (canvas, i) {
Copy link
Owner

Choose a reason for hiding this comment

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

👍 except for the extra space after forEach – it's not needed.

var my = this;
my.canvases.forEach (function (canvas) {
my.clearWaveForEntry(canvas);
});
Copy link
Owner

Choose a reason for hiding this comment

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

👍

this.canvases.forEach(function (canvas) {
    this.clearWaveForEntry(canvas);
}, this);

this.drawLineToContext(entry, entry.progressCtx, peaks, absmax, halfH, offsetY, start, end);
}
var my = this;
my.canvases.forEach (function (canvas) {
Copy link
Owner

Choose a reason for hiding this comment

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

The same comment as above.

*
* @param {String} type - an optional value of a format type. Default is image/png.
* @param {Number} quality - an optional value between 0 and 1. Default is 0.92.
*
*/
getImage: function(type, quality) {
getImage: function (type, quality) {
Copy link
Owner

Choose a reason for hiding this comment

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

👍

@agamemnus
Copy link
Contributor Author

OK, give me a few minutes to make the appropriate changes.

@agamemnus
Copy link
Contributor Author

Ok, I removed almost all of the other changes.

I also implemented your suggestion about inserting a this variable in forEach.

@katspaugh
Copy link
Owner

I still see a lot of other changes in the diff.

@agamemnus
Copy link
Contributor Author

agamemnus commented May 6, 2017

I fixed some errors.

The only other change now is removing space between opening/closing brackets and text. You don't really even have that consistent... those spaces are really terrible...

@agamemnus
Copy link
Contributor Author

Ok, a few more I didn't notice.

@agamemnus
Copy link
Contributor Author

I really hate those extraneous spaces, but I added them back. I also added some extra extraneous spaces in style definition calls for it to be consistent.

return;
} else {
peaks = channels[0];
}
}

// Support arrays without negative peaks
var hasMinValues = [].some.call(peaks, function (val) { return val < 0; });
var hasMinValues = [].some.call(peaks, function (val) {return val < 0;});
Copy link
Owner

Choose a reason for hiding this comment

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

Could you revert this change, too?

@agamemnus
Copy link
Contributor Author

Ok.

@katspaugh katspaugh merged commit 7d8c103 into katspaugh:master May 15, 2017
@katspaugh
Copy link
Owner

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants