refactor(): canvas text editing manager #8543

ShaMan123 · 2022-12-29T09:43:23Z

Motivation

Another step making text classes readable, simpler, Scoped which they are absolutely not

Description

extracted the logic that handles canvas itext instances from itext_behavior to a dedicated manager

Changes

register itext from _set. This will handle cases such as nested itexts.
delegate mouse move invocation to the manager

Gist

In Action

ShaMan123

Progress

src/mixins/itext_behavior.mixin.ts

ShaMan123 · 2022-12-29T09:46:27Z

src/shapes/text.class.ts

@@ -94,7 +94,7 @@ export class Text<
   * @type Array
   * @private
   */
-  _dimensionAffectingProps: (keyof this)[];
+  _dimensionAffectingProps: string[];


TS went mad because of this line

ShaMan123 · 2022-12-29T09:48:00Z

src/canvas/CanvasTextEditingManager.ts

+
+  add(target: IText | Textbox) {
+    if (!this.disposer) {
+      const disposer = this.canvas.on('mouse:up', () => {


I will try to remove the need for the subscription

github-actions · 2022-12-29T09:51:04Z

Build Stats

file / KB (diff)	bundled	minified
fabric	927.379 (-0.987)	298.827 (-0.283)

github-actions · 2022-12-29T10:26:37Z

Coverage after merging itext-canvas-handler into master will be

83.19%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.ts	100%	100%	100%	100%
src
cache.ts	96.97%	90%	100%	100%	56
config.ts	77.27%	66.67%	66.67%	84.62%	130, 138–140, 151–153
constants.ts	100%	100%	100%	100%
env.ts	72.73%	53.33%	100%	79.17%	22, 22–23, 23, 23, 25, 25, 27, 29, 31–32, 62
intersection.class.ts	100%	100%	100%	100%
pattern.class.ts	92.19%	85.71%	100%	96.30%	118, 124, 135, 144, 96
point.class.ts	100%	100%	100%	100%
shadow.class.ts	98.48%	96%	100%	100%	156
typedefs.ts	100%	100%	100%	100%
src/brushes
base_brush.class.ts	100%	100%	100%	100%
circle_brush.class.ts	0%	0%	0%	0%	100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 134, 141, 143, 23, 28–29, 37–41, 45–49, 56–59, 67–71, 73, 81, 81, 81, 81, 81–82, 84, 84, 84–87, 89, 97–98
pattern_brush.class.ts	97.06%	87.50%	100%	100%	21
pencil_brush.class.ts	91.86%	85.42%	100%	93.58%	122–123, 152, 152–154, 276, 280, 285–286, 68–69, 84–85
spray_brush.class.ts	0%	0%	0%	0%	100–101, 103–104, 112, 112, 112, 112, 112–113, 115–116, 123–124, 126, 128–132, 141, 145–146, 146, 154, 154, 154–157, 159–162, 166–167, 169, 171–174, 177, 184–185, 187, 189–190, 192, 199–200, 202–203, 206, 206, 213, 213, 217, 22–23, 25–27, 27, 27–29, 33, 42, 49, 56, 63, 70, 77, 89–91, 99
src/canvas
TextEditingManager.ts	100%	100%	100%	100%
canvas.class.ts	93.07%	89.06%	94%	95.87%	1151, 1151–1152, 1155, 1175, 1175, 1237, 1273–1274, 1293–1294, 1302–1303, 1324, 1332, 1445–1446, 1448–1449, 1469–1470, 507, 574–575, 580, 590, 719–720, 722–723, 723, 723, 769–770, 831–832, 885–887, 917, 922–923, 952–953
canvas_events.ts	78.34%	75.69%	82.35%	79.49%	1005–1006, 1006, 1006–1008, 1010–1011, 1011, 1011, 1013, 1021, 1021, 1021–1023, 1023, 1023, 1029–1030, 1038–1039, 1039, 1039–1040, 1045, 1047, 1077–1079, 1082–1083, 1087–1088, 1204–1206, 1209–1210, 1283, 1403, 147, 1497–1498, 1504, 1508–1509, 1525, 1547, 1594, 1599, 1638, 1647, 172, 320–321, 321, 321–322, 322, 325–329, 334, 336, 349–352, 355, 374, 374, 374–375, 375, 375–376, 384, 389–390, 390, 390–391, 393, 402, 408–409, 409, 409, 445, 449, 449, 449, 449, 449–450, 452, 527, 529, 529, 529–531, 551, 553, 553, 553–554, 554, 554, 557, 557, 557–558, 561, 570–571, 573–574, 576, 576–577, 579–580, 592–593, 593, 593–594, 596–600, 606, 613, 653, 653, 653, 655, 657–661, 667, 673, 673, 673–674, 676, 679, 684, 697, 724, 780–781, 781, 781–782, 784, 787–788, 788, 788–789, 791–792, 795, 795–797, 800–801, 811, 893, 907, 914, 935, 967, 988–989
static_canvas.class.ts	94.67%	89.27%	97.92%	97.10%	1118–1119, 1119, 1119–1120, 1240, 1250, 1304–1305, 1308, 1343–1344, 1422, 1431, 1436, 1485–1486, 1714, 1714–1715, 1764, 1767, 1770, 1770, 1770, 1773, 1776, 1776, 1776, 338, 351, 404–405, 407–408, 417, 421–422, 425–426, 878
src/color
color.class.ts	92.20%	86.49%	100%	94.34%	330–331, 335–336, 339–340, 58, 88–89, 89, 91, 91, 91–92, 94–95
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	100%	100%	100%	100%
src/controls
changeWidth.ts	100%	100%	100%	100%
control.class.ts	93.98%	88.89%	90.91%	97.78%	235, 319, 319, 354
controls.render.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
default_controls.ts	86.67%	66.67%	100%	100%	122, 129
drag.ts	100%	100%	100%	100%
polyControl.ts	6.35%	0%	0%	11.43%	100, 105, 119, 121–124, 124, 127, 134, 17, 25–28, 30, 30, 30, 30, 30, 30, 30, 30, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
rotate.ts	20%	12.50%	50%	22.22%	45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
scale.ts	93.41%	92.68%	100%	93.59%	129–130, 132–134, 148–149, 181–183, 42
scaleSkew.ts	78.79%	64.29%	100%	85.71%	27, 29, 29, 29, 31, 33, 35
skew.ts	91.03%	79.31%	100%	97.67%	130–131, 162–163, 170, 176, 178
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/filters
2d_backend.class.ts	92%	83.33%	100%	93.75%	35–36
FilterBackend.ts	88.89%	88.89%	100%	85.71%	15–16
WebGLProbe.ts	37.14%	40%	60%	30%	28–30, 30,

I think this should not be an issue tests fail because IText is used on StaticCanvas and shouldn't really

ShaMan123

seems ready
tested live as well

ShaMan123

updated from master

ShaMan123

extensive test

ShaMan123

~~I am not sure why the mouse move handler (now renamed to updateSelection) is not called from the object's mousemove event.~~ so moving outside the textbox will still update selection.

It seems selected is handled by mouse up so no need to false it from the manager
rm __isMousedown:
- used for updating selection only when mouse move after mouse down (now handled by the manager)
- used to render the cursor with global alpha = 1, fixed to not run cursor animation in such a case

This PR is becoming less scoped and is about to clash with #8583 #8534

A few fixes so tests pass and I am done hopefully

ShaMan123 · 2023-01-10T00:10:20Z

src/mixins/itext_behavior.mixin.ts

@@ -521,7 +434,6 @@ export abstract class ITextBehaviorMixin<
      this.selectionStart !== currentStart ||
      this.selectionEnd !== currentEnd
    ) {
-      this.restartCursorIfNeeded();


important fix
no reason to do this here, in case of a cursor the animation will be started from mouse up

src/mixins/itext_behavior.mixin.ts

ShaMan123 · 2023-01-10T00:16:29Z

src/mixins/itext_click_behavior.mixin.ts

+      // avoid running this logic when there is an active object
+      // this because is possible with shift click and fast clicks,
+      // to rapidly deselect and reselect this object and trigger an enterEdit
+      return;
    }

    if (this.__lastSelected && !this.__corner) {


this bit is really harsh, hard to understand logic

ShaMan123

READY

src/mixins/itext_key_behavior.mixin.ts

src/mixins/itext_behavior.mixin.ts

asturur · 2023-01-10T11:37:52Z

I think this all in all is better than what we had before, is also a change that will open up to regressions for sure.
If you can do that rename and remove the !this.selected then we can merge

asturur · 2023-01-10T14:08:51Z

Oh also please tell me more about those nexted Itexts
Are you talking about adding a group that has an itext inside?

ShaMan123

applied changes from review

src/mixins/itext_behavior.mixin.ts

asturur · 2023-01-12T00:54:18Z

This is good work imho, just not convinced about that _set usage.
Would really leave set to small things.
I m sure later we can find a way to use onObject added for that.
I don't see an issue with onObjectAdded traversing the objects.
Something is setting canvas prop on every object in a group when we move a group from a canvas to another, so this mechanism already exists somewhere

ShaMan123 · 2023-01-12T01:08:02Z

Very good point
We can move away from set

ShaMan123 added 6 commits December 29, 2022 10:47

itext canvas selection handler

fb24d56

safeguard from removing while editing

8a171b0

Update canvas_events.ts

518a565

Create CanvasTextEditingManager.ts

2386835

Update text.class.ts

8edb605

cleanup

f56fccf

ShaMan123 commented Dec 29, 2022

View reviewed changes

imports

a95f579

ShaMan123 added 2 commits December 29, 2022 12:23

adapt tests

0d3d9b5

verify(): better mouse up

278cd4f

ShaMan123 added 2 commits December 29, 2022 12:44

test(): exitEditing after removed from canvas

73dcbfd

Update CHANGELOG.md

2ac9554

ShaMan123 changed the title ~~chore(TS): canvas text editing manager~~ refactor(): canvas text editing manager Dec 29, 2022

ShaMan123 added 6 commits December 29, 2022 13:00

cleanup

b9542b4

fix(): add/remove from _set canvas

d0bed01

rename

1dcbb4a

TS

e07b9d0

test

e623a13

safeguard using static canvas

cb4b08c

I think this should not be an issue tests fail because IText is used on StaticCanvas and shouldn't really

ShaMan123 requested a review from asturur December 29, 2022 11:48

rename for clarity

8a88120

ShaMan123 commented Dec 29, 2022

View reviewed changes

ShaMan123 added 2 commits December 29, 2022 23:10

Merge branch 'master' into itext-canvas-handler

2030691

Merge branch 'master' into itext-canvas-handler

98f7d23

ShaMan123 commented Jan 6, 2023

View reviewed changes

ShaMan123 added 2 commits January 6, 2023 08:31

fix(): destroy

b5e02bb

Update canvas_events.js

1260c0d

ShaMan123 commented Jan 6, 2023

View reviewed changes

ShaMan123 added the text label Jan 10, 2023

ShaMan123 added 2 commits January 10, 2023 02:05

safety

288041d

where did this go?

5fd48af

ShaMan123 commented Jan 10, 2023

View reviewed changes

This was referenced Jan 10, 2023

fix(): Draggable Text Migration Regression #8534

Merged

refactor(IText): modernize cursor animation #8583

Merged

ShaMan123 added 3 commits January 10, 2023 02:50

fix tests

904fd82

fix

88604f2

final touch up

c10282a

ShaMan123 commented Jan 10, 2023

View reviewed changes

Merge branch 'master' into itext-canvas-handler

536f530

asturur reviewed Jan 10, 2023

View reviewed changes

src/mixins/itext_key_behavior.mixin.ts Outdated Show resolved Hide resolved

asturur reviewed Jan 10, 2023

View reviewed changes

src/mixins/itext_behavior.mixin.ts Show resolved Hide resolved

asturur reviewed Jan 10, 2023

View reviewed changes

src/mixins/itext_behavior.mixin.ts Outdated Show resolved Hide resolved

ShaMan123 added 2 commits January 11, 2023 08:56

rename

a237a9a

revert blur fix

2b8277c

ShaMan123 commented Jan 11, 2023

View reviewed changes

forgot test

900efdf

ShaMan123 commented Jan 11, 2023

View reviewed changes

src/mixins/itext_behavior.mixin.ts Show resolved Hide resolved

ShaMan123 requested a review from asturur January 11, 2023 07:06

ShaMan123 and others added 2 commits January 11, 2023 09:07

pretty

2a8d545

Merge branch 'master' into itext-canvas-handler

25e5a9b

asturur approved these changes Jan 12, 2023

View reviewed changes

asturur merged commit 8b102b7 into master Jan 12, 2023

asturur deleted the itext-canvas-handler branch January 12, 2023 00:54

ShaMan123 added a commit that referenced this pull request Mar 16, 2024

refactor(Text): canvas text editing manager (#8543)

47fb5cc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(): canvas text editing manager #8543

refactor(): canvas text editing manager #8543

ShaMan123 commented Dec 29, 2022 •

edited

ShaMan123 left a comment

ShaMan123 Dec 29, 2022

ShaMan123 Dec 29, 2022

ShaMan123 Dec 29, 2022

github-actions bot commented Dec 29, 2022 •

edited

github-actions bot commented Dec 29, 2022 •

edited

ShaMan123 left a comment

ShaMan123 left a comment

ShaMan123 left a comment

ShaMan123 left a comment •

edited

ShaMan123 Jan 10, 2023 •

edited

ShaMan123 Jan 10, 2023

ShaMan123 left a comment

asturur commented Jan 10, 2023

asturur commented Jan 10, 2023

ShaMan123 left a comment

asturur commented Jan 12, 2023

ShaMan123 commented Jan 12, 2023

refactor(): canvas text editing manager #8543

refactor(): canvas text editing manager #8543

Conversation

ShaMan123 commented Dec 29, 2022 • edited

Motivation

Description

Changes

Gist

In Action

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 Dec 29, 2022

Choose a reason for hiding this comment

ShaMan123 Dec 29, 2022

Choose a reason for hiding this comment

ShaMan123 Dec 29, 2022

Choose a reason for hiding this comment

github-actions bot commented Dec 29, 2022 • edited

github-actions bot commented Dec 29, 2022 • edited

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 left a comment • edited

Choose a reason for hiding this comment

ShaMan123 Jan 10, 2023 • edited

Choose a reason for hiding this comment

ShaMan123 Jan 10, 2023

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

asturur commented Jan 10, 2023

asturur commented Jan 10, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

asturur commented Jan 12, 2023

ShaMan123 commented Jan 12, 2023

ShaMan123 commented Dec 29, 2022 •

edited

github-actions bot commented Dec 29, 2022 •

edited

github-actions bot commented Dec 29, 2022 •

edited

ShaMan123 left a comment •

edited

ShaMan123 Jan 10, 2023 •

edited