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

chore(TS): utils #8230

Merged
merged 37 commits into from
Sep 3, 2022
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
047b4c8
getById
asturur Sep 1, 2022
8696f5e
this is fine
asturur Sep 1, 2022
aa90704
remove toArray
asturur Sep 1, 2022
debadac
addClass removed for classList.add)
asturur Sep 1, 2022
2807ec2
Merge branch 'master' into see-where-i-broke
asturur Sep 1, 2022
77b0727
ok?
asturur Sep 1, 2022
d2ac909
Revert "ok?"
asturur Sep 1, 2022
f46feee
Revert "addClass removed for classList.add)"
asturur Sep 1, 2022
bc9962c
rotate point
asturur Sep 1, 2022
bf8efa2
shadow with real point
asturur Sep 1, 2022
bfe92fd
added point rotation explict test
asturur Sep 1, 2022
0eb7947
Squashed commit of the following:
ShaMan123 Sep 1, 2022
cdf5992
put back some old logic
asturur Sep 1, 2022
a7a3026
handle wrapelement
asturur Sep 1, 2022
6bb4d2b
handle wrapelement
asturur Sep 1, 2022
4e3939f
fix tests
asturur Sep 1, 2022
92bee80
change the export
asturur Sep 1, 2022
d2e8f05
moved limitDimByArea
asturur Sep 1, 2022
3cd1da1
moving isTransparent
asturur Sep 1, 2022
45a46a4
moved mergeClipPaths
asturur Sep 1, 2022
7ecd0c5
fixed export and colon
asturur Sep 1, 2022
0232258
export ease
asturur Sep 1, 2022
33f18bc
animate color
asturur Sep 1, 2022
f363dfe
Update src/util/misc/mergeClipPaths.ts
asturur Sep 2, 2022
c1eb748
night commit
asturur Sep 2, 2022
1ea46cb
Merge branch 'see-where-i-broke' of github.com:fabricjs/fabric.js int…
asturur Sep 2, 2022
26a1acf
moved
asturur Sep 2, 2022
cc95956
clarify breakage
asturur Sep 2, 2022
612b5d1
removed var
asturur Sep 2, 2022
fbe5f75
converted animate
asturur Sep 2, 2022
585b0e6
only change export for createClass
asturur Sep 2, 2022
cabb1c6
fixes
asturur Sep 2, 2022
7c99ce5
ok passes
asturur Sep 2, 2022
cd54755
typed anim_ease
asturur Sep 2, 2022
5ce2a09
use `sendObjectToPlane`
ShaMan123 Sep 3, 2022
a5d5441
Revert "use `sendObjectToPlane`"
asturur Sep 3, 2022
ca6f6b9
add back the optional setTimeout
asturur Sep 3, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,7 @@ import './src/mixins/shared_methods.mixin';
import './src/util/misc/misc';
// import './src/util/named_accessors.mixin'; i would imagine dead forever or proper setters/getters
import './src/util/lang_class';
import './src/util/dom_misc';
import './src/util/animate'; // optional animation
import './src/util/animate_color'; // optional animation
import './src/util/anim_ease'; // optional easing
import './src/parser'; // optional parser
import './src/point.class';
import './src/intersection.class';
Expand Down
16 changes: 16 additions & 0 deletions src/cache.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { config } from './config';

export class Cache {
/**
Expand Down Expand Up @@ -44,6 +45,21 @@ export class Cache {
}
}

/**
* Given current aspect ratio, determines the max width and height that can
* respect the total allowed area for the cache.
* @memberOf fabric.util
* @param {number} ar aspect ratio
* @return {number[]} Limited dimensions X and Y
*/
limitDimsByArea(ar: number) {
const { perfLimitSizeTotal } = config;
const roughWidth = Math.sqrt(perfLimitSizeTotal * ar);
// we are not returning a point on purpose, to avoid circular dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

VERY STRANGE
And shouldn't happen...
HACKY

Copy link
Member Author

Choose a reason for hiding this comment

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

no is not hacky and not everything should be a point.
Before was x and y but wasn't a point.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason for the circular dep cycle is just because of the assignment fabric.Point that will be removed anyways.
If you assign it in index it will not happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you agree this is out of pattern?

Copy link
Member Author

Choose a reason for hiding this comment

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

is a function about caching, i don't think returning an array to return 2 values is better or worse than returnin an object and calling it a point. I just really need the 2 values out of there.

// this is an internal utility
return [Math.floor(roughWidth), Math.floor(perfLimitSizeTotal / roughWidth)];
}

/**
* This object contains the result of arc to bezier conversion for faster retrieving if the same arc needs to be converted again.
* It was an internal variable, is accessible since version 2.3.4
Expand Down
25 changes: 13 additions & 12 deletions src/canvas.class.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1044,19 +1044,20 @@ import { Point } from './point.class';
* @throws {CANVAS_INIT_ERROR} If canvas can not be initialized
*/
_createUpperCanvas: function () {
var lowerCanvasClass = this.lowerCanvasEl.className.replace(/\s*lower-canvas\s*/, ''),
lowerCanvasEl = this.lowerCanvasEl, upperCanvasEl = this.upperCanvasEl;
var lowerCanvasEl = this.lowerCanvasEl, upperCanvasEl = this.upperCanvasEl;

// there is no need to create a new upperCanvas element if we have already one.
if (upperCanvasEl) {
upperCanvasEl.className = '';
}
else {
// if there is no upperCanvas (most common case) we create one.
if (!upperCanvasEl) {
upperCanvasEl = this._createCanvasElement();
this.upperCanvasEl = upperCanvasEl;
}
fabric.util.addClass(upperCanvasEl, 'upper-canvas ' + lowerCanvasClass);
this.upperCanvasEl.setAttribute('data-fabric', 'top');
// we assign the same classname of the lowerCanvas
upperCanvasEl.className = lowerCanvasEl.className;
// but then we remove the lower-canvas specific className
upperCanvasEl.classList.remove('lower-canvas');
// we add the specific upper-canvas class
upperCanvasEl.classList.add('upper-canvas');
Copy link
Contributor

Choose a reason for hiding this comment

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

much better

upperCanvasEl.setAttribute('data-fabric', 'top');
this.wrapperEl.appendChild(upperCanvasEl);

this._copyCanvasStyle(lowerCanvasEl, upperCanvasEl);
Expand All @@ -1082,9 +1083,9 @@ import { Point } from './point.class';
if (this.wrapperEl) {
return;
}
this.wrapperEl = fabric.util.wrapElement(this.lowerCanvasEl, 'div', {
'class': this.containerClass
});
const container = fabric.document.createElement('div');
container.classList.add(this.containerClass);
this.wrapperEl = fabric.util.wrapElement(this.lowerCanvasEl, container);
Copy link
Contributor

Choose a reason for hiding this comment

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

drop wrapElement from utils?

Copy link
Member Author

Choose a reason for hiding this comment

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

wrapElement probably don't belong in utils exported object indeed. will be just a function imported here

Copy link
Contributor

Choose a reason for hiding this comment

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

it is used only here across fabric, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes i think only here to add a div around the canvas

this.wrapperEl.setAttribute('data-fabric', 'wrapper');
fabric.util.setStyle(this.wrapperEl, {
width: this.width + 'px',
Expand Down
3 changes: 2 additions & 1 deletion src/constants.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { TMat2D } from "./typedefs";

export { version as VERSION } from '../package.json';
export const noop = () => {};
export function noop() {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a difference between const = () => {} and function(){}?
Or just styling?
I don't mind

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a difference that a fat arrow can't be bound and can't be used as a class member.
So since noop is used in createClass as an empty method, just to be sure i made it a function

export const halfPI = Math.PI / 2;
export const twoMathPi = Math.PI * 2;
export const PiBy180 = Math.PI / 180;
export const iMatrix = Object.freeze([1, 0, 0, 1, 0, 0]) as TMat2D;
export const DEFAULT_SVG_FONT_SIZE = 16;
Expand Down
4 changes: 2 additions & 2 deletions src/parser/parseSVGDocument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export function parseSVGDocument(doc, callback, reviver, parsingOptions) {
}
parseUseDirectives(doc);

let svgUid = fabric.Object.__uid++, i, len, options = applyViewboxTransform(doc), descendants = fabric.util.toArray(doc.getElementsByTagName('*'));
let svgUid = fabric.Object.__uid++, i, len, options = applyViewboxTransform(doc), descendants = Array.from(doc.getElementsByTagName('*'));
options.crossOrigin = parsingOptions && parsingOptions.crossOrigin;
options.svgUid = svgUid;
options.signal = parsingOptions && parsingOptions.signal;
Expand Down Expand Up @@ -61,7 +61,7 @@ export function parseSVGDocument(doc, callback, reviver, parsingOptions) {
return el.nodeName.replace('svg:', '') === 'clipPath';
}).forEach(function (el) {
const id = el.getAttribute('id');
localClipPaths[id] = fabric.util.toArray(el.getElementsByTagName('*')).filter(function (el) {
localClipPaths[id] = Array.from(el.getElementsByTagName('*')).filter(function (el) {
return svgValidTagNamesRegEx.test(el.nodeName.replace('svg:', ''));
});
});
Expand Down
18 changes: 14 additions & 4 deletions src/point.class.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { fabric } from '../HEADER';
import { TMat2D, TRadian } from './typedefs';
import { sin } from './util/misc/sin';
import { cos } from './util/misc/cos';

export interface IPoint {
x: number
Expand Down Expand Up @@ -350,16 +352,22 @@ export class Point {

/**
* Rotates `point` around `origin` with `radians`
* WARNING: this is probably a source of circular dependency.
* evaluate what to do when importing rotateVector directly from the file
* @static
* @memberOf fabric.util
* @param {Point} origin The origin of the rotation
* @param {TRadian} radians The radians of the angle for the rotation
* @return {Point} The new rotated point
*/
rotate(origin: Point, radians: TRadian): Point {
return fabric.util.rotateVector(this.subtract(origin), radians).add(origin);
rotate(radians: TRadian, origin: Point = originZero): Point {
// TODO benchmark and verify the add and subtract how much cost
// and then in case early return if no origin is passed
const sinus = sin(radians), cosinus = cos(radians);
const p = this.subtract(origin);
const rotated = new Point(
p.x * cosinus - p.y * sinus,
p.x * sinus + p.y * cosinus,
);
return rotated.add(origin);
}

/**
Expand All @@ -378,4 +386,6 @@ export class Point {
}
}

const originZero = new Point(0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

You want to reuse this? That is why you don't inline it in the function signature?

Copy link
Member Author

Choose a reason for hiding this comment

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

i think is more readable defined as a const rather than in the function definition.
Does it get defined every function run if is in the function signature?

Copy link
Contributor

Choose a reason for hiding this comment

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

good question
no idea


fabric.Point = Point;
3 changes: 2 additions & 1 deletion src/shadow.class.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import { Color } from "./color";
import { config } from "./config";
import { Point } from "./point.class";

(function(global) {
var fabric = global.fabric || (global.fabric = { }),
Expand Down Expand Up @@ -119,7 +120,7 @@ import { config } from "./config";
toSVG: function(object) {
var fBoxX = 40, fBoxY = 40, NUM_FRACTION_DIGITS = config.NUM_FRACTION_DIGITS,
offset = fabric.util.rotateVector(
{ x: this.offsetX, y: this.offsetY },
new Point(this.offsetX, this.offsetY),
fabric.util.degreesToRadians(-object.angle)),
BLUR_BOX = 20, color = new Color(this.color);

Expand Down
8 changes: 4 additions & 4 deletions src/shapes/image.class.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@
this._element = element;
this._originalElement = element;
this._initConfig(options);
element.classList.add(fabric.Image.CSS_CANVAS);
if (this.filters.length !== 0) {
this.applyFilters();
}
Expand Down Expand Up @@ -477,7 +478,7 @@
* @param {CanvasRenderingContext2D} ctx Context to render on
*/
_render: function(ctx) {
fabric.util.setImageSmoothing(ctx, this.imageSmoothing);
ctx.imageSmoothingEnabled = this.imageSmoothing;
if (this.isMoving !== true && this.resizeFilter && this._needsResize()) {
this.applyResizeFilters();
}
Expand All @@ -491,7 +492,7 @@
* @param {CanvasRenderingContext2D} ctx Context to render on
*/
drawCacheOnCanvas: function(ctx) {
fabric.util.setImageSmoothing(ctx, this.imageSmoothing);
ctx.imageSmoothingEnabled = this.imageSmoothing;
fabric.Object.prototype.drawCacheOnCanvas.call(this, ctx);
},

Expand Down Expand Up @@ -557,8 +558,7 @@
* @param {Object} [options] Options object
*/
_initElement: function(element, options) {
this.setElement(fabric.util.getById(element), options);
fabric.util.addClass(this.getElement(), fabric.Image.CSS_CANVAS);
this.setElement(fabric.document.getElementById(element) || element, options);
},

/**
Expand Down
13 changes: 6 additions & 7 deletions src/shapes/object.class.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//@ts-nocheck

import { cache } from '../cache';
import { config } from '../config';
import { VERSION } from '../constants';
import { Point } from '../point.class';
Expand Down Expand Up @@ -684,10 +684,9 @@ import { pick } from '../util/misc/pick';
* @return {Object}.zoomY zoomY zoom value to unscale the canvas before drawing cache
*/
_limitCacheSize: function(dims) {
var perfLimitSizeTotal = config.perfLimitSizeTotal,
width = dims.width, height = dims.height,
var width = dims.width, height = dims.height,
max = config.maxCacheSideLimit, min = config.minCacheSideLimit;
if (width <= max && height <= max && width * height <= perfLimitSizeTotal) {
if (width <= max && height <= max && width * height <= config.perfLimitSizeTotal) {
if (width < min) {
dims.width = min;
}
Expand All @@ -696,9 +695,9 @@ import { pick } from '../util/misc/pick';
}
return dims;
}
var ar = width / height, limitedDims = fabric.util.limitDimsByArea(ar, perfLimitSizeTotal),
x = capValue(min, limitedDims.x, max),
y = capValue(min, limitedDims.y, max);
var ar = width / height, [limX, limY] = cache.limitDimsByArea(ar),
x = capValue(min, limX, max),
y = capValue(min, limY, max);
if (width > x) {
dims.zoomX /= width / x;
dims.width = x;
Expand Down
6 changes: 3 additions & 3 deletions src/static_canvas.class.ts
Original file line number Diff line number Diff line change
Expand Up @@ -310,14 +310,14 @@ import { pick } from './util/misc/pick';
this.lowerCanvasEl = canvasEl;
}
else {
this.lowerCanvasEl = fabric.util.getById(canvasEl) || this._createCanvasElement();
this.lowerCanvasEl = fabric.document.getElementById(canvasEl) || canvasEl || this._createCanvasElement();
}
if (this.lowerCanvasEl.hasAttribute('data-fabric')) {
/* _DEV_MODE_START_ */
throw new Error('fabric.js: trying to initialize a canvas that has already been initialized');
/* _DEV_MODE_END_ */
}
fabric.util.addClass(this.lowerCanvasEl, 'lower-canvas');
this.lowerCanvasEl.classList.add('lower-canvas');
this.lowerCanvasEl.setAttribute('data-fabric', 'main');
if (this.interactive) {
this._originalCanvasStyle = this.lowerCanvasEl.style.cssText;
Expand Down Expand Up @@ -752,7 +752,7 @@ import { pick } from './util/misc/pick';
this.cancelRequestedRender();
this.calcViewportBoundaries();
this.clearContext(ctx);
fabric.util.setImageSmoothing(ctx, this.imageSmoothingEnabled);
ctx.imageSmoothingEnabled = this.imageSmoothingEnabled;
ctx.patternQuality = 'best';
Copy link
Contributor

Choose a reason for hiding this comment

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

should this patternQuality be set on Image as well? And all cache canvases?
And maybe quality as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those are node-canvas specific things i m ignoring on purpose. Are their default not good?

I want to have a clean canvas interface as much as possible in case we move to skia-canvas or maybe we have both as an option.

Copy link
Contributor

Choose a reason for hiding this comment

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

good is default
I changed that in a PR to fix a visual test I recall

Copy link
Contributor

Choose a reason for hiding this comment

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

but missed quality

this.fire('before:render', { ctx: ctx, });
this._renderBackground(ctx);
Expand Down
Loading