Allow textAlign(CENTER, CENTER) with max width but no max height

[sawaisinghh]

Resolves #5933

Changes:
Now we can use textAlign(CENTER, CENTER) and then call text('something', x, y, w) without specifying a height parameter.

Screenshots of the change:

Before:

After:

PR Checklist

• npm run lint passes
• Inline documentation is included / updated
• Unit tests are included / updated

[sawaisinghh]

@davepagurek, please review it.

[dhowe]

@sawaisinghh @davepagurek can you clarify the specification for this, specifically for multi-line text? For the CENTER/CENTER case without height, this PR appears to center on the y-position, and then cut-off all the text above that (left image), rather than the previous behavior (right image)

   

let txt = "Lorem Ipsum has been the industry's standard dummy text ever since the 1500s, when an unknown printer took a galley of type and scrambled it to make a type specimen book. It has survived not only five centuries, but also the leap into electronic typesetting, remaining essentially unchanged.";
let x = 20, y = 50, w = 250;
function setup() {
  createCanvas(300, 300);
  background(245);

  textAlign(CENTER, CENTER);
  textFont("georgia", 12);
  rect(x, y, w, height - y);

  text(txt, x, y, w);
}

[sawaisinghh]

@dhowe okay, I will try to resolve it.

[sawaisinghh]

@dhowe, But, I think no text were visible in previous behavior specifically in multiline.
you can see here:

[dhowe]

Correct -- there was only a warning. The upper right image is from 1.4.2, which is (I think) the behavior you were trying to restore

[sawaisinghh]

@dhowe yeah correct, according to issue, textAlign(CENTER, CENTER) without specifying height were working fine in 1.4.2, but not working in current version. Okay thanks for the review, I will try to fix it.

[sawaisinghh]

@davepagurek @limzykenneth please review my pull request.

[davepagurek]

Right, to clarify the expectations here, we want the textAlign example to look like it did in 1.4.2:

For multiline behaviour, @dhowe's observation is correct, we'd want it to center the text about the middle y position and not cut off anything vertically and match its behaviour in 1.4.2. Here's a test case (p5 editor):

function setup() { background(200); rectMode(CENTER); textAlign(CENTER, CENTER); text( 'The quick brown fox jumps over the lazy dog', width/2, height/2, width ); }

[davepagurek]

Thanks for your work on this! I tested it out and it looks like it meets those requirements in from my last comment 🙂 The functionality seems good to go! I just have some final code style comments before merging.

[davepagurek]

We probably shouldn't edit p5.sound.js here, as it comes from builds in https://github.com/processing/p5.js-sound that we periodically pull from. If we want to make a change to it, we'd have to make it in that repo for it to not get overwritten in the next build.

[davepagurek]

It looks like this._textLeading is always initialized here

p5.js/src/core/p5.Renderer.js

Line 40 in 4e231d9

this._textLeading = 15;

and also set when the user calls textSize(), so I think it's safe to just use this._textLeading here.

If there's a case I'm not considering though, and we need the || 1.25, can we replace 1.25 with constants._DEFAULT_LEADMULT like we use here?

p5.js/src/core/p5.Renderer.js

Line 163 in 4e231d9

this._setProperty('_textLeading', s * constants._DEFAULT_LEADMULT);

[sawaisinghh]

@davepagurek I made the changes. Can you please review it.

[davepagurek]

Thanks for making those changes! One last very minor thing and then we're good to go!

[davepagurek]

One last tiny change, we can take out the brackets around this._textLeading now 🙂

[davepagurek]

Thanks again, great work!

[sawaisinghh]

@davepagurek done 🙂

[davepagurek]

@all-contributors please add @sawaisinghh for bug, code

[allcontributors]

@davepagurek

I've put up a pull request to add @sawaisinghh! 🎉