chore(TS): migrate brushes #8182

ShaMan123 · 2022-08-25T10:28:38Z

I will migrate eraser on a separate PR
It will wait...

github-actions · 2022-08-25T10:30:57Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.13 |    74.64 |   84.43 |   80.77 |                                               
 fabric.js |   82.13 |    74.64 |   84.43 |   80.77 | ...-27701,27828-27829,27851-27888,27905-28065 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-08-25T10:35:31Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.12 |     74.6 |   84.43 |   80.76 |                                               
 fabric.js |   82.12 |     74.6 |   84.43 |   80.76 | ...-27699,27826-27827,27849-27886,27903-28063 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-08-25T11:59:40Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.17 |    74.62 |   84.36 |    80.8 |                                               
 fabric.js |   82.17 |    74.62 |   84.36 |    80.8 | ...-27689,27805-27806,27827-27868,27883-28042 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-08-25T12:04:44Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.14 |    74.62 |   84.36 |   80.76 |                                               
 fabric.js |   82.14 |    74.62 |   84.36 |   80.76 | ...-27689,27805-27806,27827-27868,27883-28042 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123

See comments for major changes
I propose we add an options object in the constructor passing initial state. I have been wanting this for a while

READY

ShaMan123 · 2022-08-25T10:30:57Z

index.js

@@ -93,5 +88,7 @@ import './src/mixins/itext_key_behavior.mixin'; // optional itext
 import './src/mixins/itext.svg_export'; // optional itext
 import './src/shapes/textbox.class'; // optional textbox
 import './src/mixins/default_controls'; // optional interaction
+
+import './src/brushes'; // optional freedrawing


moved to end because brushes depend on shapes

ShaMan123 · 2022-08-25T10:31:35Z

src/brushes/base_brush.class.ts

+    const canvas = this.canvas,
+      shadow = this.shadow,
+      ctx = canvas.contextTop,
+      zoom = canvas.getZoom() * canvas.getRetinaScaling();


changed this line

yes but it should be 100% equivalent

ShaMan123 · 2022-08-25T11:45:56Z

src/brushes/base_brush.class.ts

+   */
+  canvas
+
+  constructor(canvas) {


How about accepting options?

ShaMan123 · 2022-08-25T11:47:00Z

src/__types__.ts

+ */
+export type Shadow = any;
+export type Canvas = any;
+export type Rect = any;


transient types

ShaMan123 · 2022-08-25T11:59:35Z

src/brushes/pencil_brush.class.ts

-     * @param {(string|number)[][]} pathData SVG path commands
-     * @returns {boolean}
-     */
-    _isEmptySVGPath: function (pathData) {


extracted to standalone function

ShaMan123 · 2022-08-25T12:03:38Z

src/brushes/pencil_brush.class.ts

-     * @private
-     * @param {Object} pointer Actual mouse position related to the canvas.
-     */
-    _captureDrawingPath: function(pointer) {


removed
this was a redundant wrapper around _addPoint

github-actions · 2022-08-25T12:10:49Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.13 |    74.59 |   84.36 |   80.74 |                                               
 fabric.js |   82.13 |    74.59 |   84.36 |   80.74 | ...-27689,27805-27806,27827-27868,27883-28042 
-----------|---------|----------|---------|---------|-----------------------------------------------

asturur · 2022-08-25T15:21:57Z

I have no issue with adding initial state to a brush ( even if it can probably be done outisde the constructor ).
The point is that the brush interface as it is now does not make sense, they are way similar to objects and have this issue of starting and ending with mouse down/up.
I want to change that and i m not sure how things will translate, but in general we can assume is safe to think brushes can have options when created.

ShaMan123 · 2022-08-25T15:25:44Z

I don't mind waiting and returning to it later.
I want to hear more of your thoughts regarding brushes

github-actions · 2022-08-25T15:47:19Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.14 |    74.62 |   84.36 |   80.76 |                                               
 fabric.js |   82.14 |    74.62 |   84.36 |   80.76 | ...-27689,27805-27806,27827-27868,27883-28042 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123 · 2022-08-25T16:12:16Z

let's merge this. enough changes for one iteration

asturur · 2022-08-26T07:36:30Z

yes this will get merge with no additional changes today.
Regrding brushes, i would like a brush to expose an even handler for every mouse or touch event.
down, click, up, dblclick and so on.
Those events will get executed as today before standard canvas logic, and will by default block the event from running the canvas logic afterward, but is optional to the dev to do so.
The brush will be in control to terminate its own lifecycle on one of those events.

This should let current brushes work as they are working today, but would also allow brushes to work on clicks and collect points like to build a polygon and end when the polygon is closed or end on dblclick.

It doesn't change much but offer so much more flexibility.

ShaMan123 · 2022-08-26T08:17:07Z

sounds great
and even not block events (with a return value) so things can run together creating complex UX.
This can be leveraged to many things like you suggested and a custom pointer for example or a laser.

ShaMan123 added 3 commits August 25, 2022 12:25

init

6b42166

Update pencil_brush.class.ts

eb0c73d

es6 + types

c7aa58d

Update base_brush.class.ts

b41de6e

ShaMan123 added 7 commits August 25, 2022 13:38

types

505fbb4

ts

306bae8

dep(): _captureDrawingPath

4c1614b

remove _captureDrawingPath

2d062a9

Update freedraw.js

3ca01f0

rename

35778ef

points

79c7f68

Update pencil_brush.class.ts

ddee4dd

ShaMan123 requested a review from asturur August 25, 2022 12:02

ShaMan123 commented Aug 25, 2022

View reviewed changes

remove fabric keyword

7c618e0

Merge branch 'master' into ts-brushes

65083c4

asturur approved these changes Aug 26, 2022

View reviewed changes

ShaMan123 merged commit f52f1c4 into master Aug 26, 2022

ShaMan123 deleted the ts-brushes branch September 11, 2022 23:36

ShaMan123 mentioned this pull request Nov 7, 2022

chore(TS): BaseBrush abstract methods #8428

Merged

frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023

chore(TS): migrate brushes (fabricjs#8182)

e4dc308

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(TS): migrate brushes #8182

chore(TS): migrate brushes #8182

ShaMan123 commented Aug 25, 2022 •

edited

Loading

github-actions bot commented Aug 25, 2022 •

edited

Loading

github-actions bot commented Aug 25, 2022 •

edited

Loading

github-actions bot commented Aug 25, 2022 •

edited

Loading

github-actions bot commented Aug 25, 2022 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

ShaMan123 Aug 25, 2022

ShaMan123 Aug 25, 2022

asturur Aug 25, 2022

ShaMan123 Aug 25, 2022

ShaMan123 Aug 25, 2022

ShaMan123 Aug 25, 2022

ShaMan123 Aug 25, 2022 •

edited

Loading

ShaMan123 Aug 25, 2022

github-actions bot commented Aug 25, 2022 •

edited

Loading

asturur commented Aug 25, 2022

ShaMan123 commented Aug 25, 2022

github-actions bot commented Aug 25, 2022 •

edited

Loading

ShaMan123 commented Aug 25, 2022

asturur commented Aug 26, 2022

ShaMan123 commented Aug 26, 2022 •

edited

Loading

chore(TS): migrate brushes #8182

chore(TS): migrate brushes #8182

Conversation

ShaMan123 commented Aug 25, 2022 • edited Loading

github-actions bot commented Aug 25, 2022 • edited Loading

Code Coverage Summary

github-actions bot commented Aug 25, 2022 • edited Loading

Code Coverage Summary

github-actions bot commented Aug 25, 2022 • edited Loading

Code Coverage Summary

github-actions bot commented Aug 25, 2022 • edited Loading

Code Coverage Summary

ShaMan123 left a comment • edited Loading

Choose a reason for hiding this comment

ShaMan123 Aug 25, 2022

Choose a reason for hiding this comment

ShaMan123 Aug 25, 2022

Choose a reason for hiding this comment

asturur Aug 25, 2022

Choose a reason for hiding this comment

ShaMan123 Aug 25, 2022

Choose a reason for hiding this comment

ShaMan123 Aug 25, 2022

Choose a reason for hiding this comment

ShaMan123 Aug 25, 2022

Choose a reason for hiding this comment

ShaMan123 Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

ShaMan123 Aug 25, 2022

Choose a reason for hiding this comment

github-actions bot commented Aug 25, 2022 • edited Loading

Code Coverage Summary

asturur commented Aug 25, 2022

ShaMan123 commented Aug 25, 2022

github-actions bot commented Aug 25, 2022 • edited Loading

Code Coverage Summary

ShaMan123 commented Aug 25, 2022

asturur commented Aug 26, 2022

ShaMan123 commented Aug 26, 2022 • edited Loading

ShaMan123 commented Aug 25, 2022 •

edited

Loading

github-actions bot commented Aug 25, 2022 •

edited

Loading

github-actions bot commented Aug 25, 2022 •

edited

Loading

github-actions bot commented Aug 25, 2022 •

edited

Loading

github-actions bot commented Aug 25, 2022 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

ShaMan123 Aug 25, 2022 •

edited

Loading

github-actions bot commented Aug 25, 2022 •

edited

Loading

github-actions bot commented Aug 25, 2022 •

edited

Loading

ShaMan123 commented Aug 26, 2022 •

edited

Loading