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

use es6-style class for some classes. #6075

Merged
merged 10 commits into from
Mar 22, 2023

Conversation

asukaminato0721
Copy link
Contributor

Resolves #3758

make up for #3874

Changes:

use es6-style class for some classes.

Screenshots of the change:

PR Checklist

@welcome
Copy link

welcome bot commented Mar 17, 2023

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

@asukaminato0721
Copy link
Contributor Author

p5.Renderer.js and p5.Renderer2D.js seem have some confusing errors, so I didn't port them.

p5.Renderer = class extends p5.Element {
  constructor(elt, pInst, isMainCanvas){
    super(elt, pInst);
    this.canvas = elt;
    this._pixelsState = pInst;
    if (isMainCanvas) {
      this._isMainCanvas = true;
      // for pixel method sharing with pimage
      this._pInst._setProperty('_curElement', this);
      this._pInst._setProperty('canvas', this.canvas);
      this._pInst._setProperty('width', this.width);
      this._pInst._setProperty('height', this.height);
    } else {
    // hide if offscreen buffer by default
      this.canvas.style.display = 'none';
      this._styles = []; // non-main elt styles stored in p5.Renderer
    }

    this._textSize = 12;
    this._textLeading = 15;
    this._textFont = 'sans-serif';
    this._textStyle = constants.NORMAL;
    this._textAscent = null;
    this._textDescent = null;
    this._textAlign = constants.LEFT;
    this._textBaseline = constants.BASELINE;
    this._textWrap = constants.WORD;

    this._rectMode = constants.CORNER;
    this._ellipseMode = constants.CENTER;
    this._curveTightness = 0;
    this._imageMode = constants.CORNER;

    this._tint = null;
    this._doStroke = true;
    this._doFill = true;
    this._strokeSet = false;
    this._fillSet = false;
    this._leadingSet = false;
  }
};

will throw a lot of errors.


p5.Renderer2D = class extends p5.Renderer {
  constructor(elt, pInst, isMainCanvas){
    super(elt, pInst, isMainCanvas);
    this.drawingContext = this.canvas.getContext('2d');
    this._pInst._setProperty('drawingContext', this.drawingContext);
    return this;
  }
};

will give

  1782 passing (20s)
  10 pending
  1 failing
  1) Global Error Handling
       identifies errors happenning internally:

      AssertionError: expected 2 to equal 1
      + expected - actual

      -2
      +1
      
      at assert.strictEqual (http://localhost:9001/node_modules/chai/chai.js:2329:32)
      at http://localhost:9001/test/unit/core/error_helpers.js:564:14

@limzykenneth
Copy link
Member

I'll try to have a look later today but for the most part I'm inclined to trust that since the CI tests are passing, these changes should be fine.

@davepagurek
Copy link
Contributor

@wuyudi if after this you feel like making another PR with the p5.Renderer classes converted, I can take a look into the issues you were seeing. I suspect it has to do with the processStack function in src/core/friendly_errors/fes_core.js and the stacktrace looking different, and hopefully just a matter of updating it to use whatever slightly different format we see in the stack traces after converting.

@limzykenneth
Copy link
Member

@wuyudi I'm getting a test error when running locally

1) loading images
       animated gifs animate correctly:
     AssertionError: expected 0 to equal 1
      at http://localhost:9001/test/unit/image/loading.js:138:14

I'll see if I can find out what it is. The reason CI is passing but not locally might be related to the very old version of nodejs being used in CI. I'll be doing some CI update today as well.

@limzykenneth
Copy link
Member

Just test things out more thoroughly, the test fail I'm seeing seems to be coming from and fixed in the main branch. I'll merge this then. @wuyudi Feel free to look into working on converting p5.Renderer as well with @davepagurek if you wish. Thanks!

@limzykenneth limzykenneth merged commit 39d0f48 into processing:main Mar 22, 2023
@asukaminato0721 asukaminato0721 deleted the to-es6 branch March 22, 2023 16:15
@asukaminato0721 asukaminato0721 mentioned this pull request May 31, 2023
3 tasks
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.

ES6 Transition: Concerns & Progress
3 participants