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.

Sign up for GitHub

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

Jump to bottom

feat(IText): Swap flashing Rate of Editing Cursor for better feedback #9823

Closed
wants to merge 11 commits into from
Closed

feat(IText): Swap flashing Rate of Editing Cursor for better feedback #9823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
facb18b
Adjust Flashing Rate of Editing Cursor
milonic Apr 24, 2024
b9c9be3
Delete src/shapes/IText/ITextBehavior.ts.bak
milonic Apr 24, 2024
58cd659
Update ITextBehavior.ts
asturur Apr 26, 2024
81790e3
Update CHANGELOG.md
asturur Apr 26, 2024
90af573
Update CHANGELOG.md
asturur Apr 26, 2024
c73cf73
Merge branch 'master' into Adjust-flashing-rate-of-editing-cursor
asturur Apr 26, 2024
2b33b1a
remove the ambiguity of delay || 100
asturur Apr 29, 2024
69c753a
Merge branch 'master' into Adjust-flashing-rate-of-editing-cursor
asturur Apr 29, 2024
a837258
Merge branch 'master' into Adjust-flashing-rate-of-editing-cursor
asturur Apr 30, 2024
facd6a5
Merge branch 'master' into Adjust-flashing-rate-of-editing-cursor
asturur Apr 30, 2024
94ac362
Merge branch 'master' into Adjust-flashing-rate-of-editing-cursor
asturur May 3, 2024
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: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

## [next]

- chore(FabricObject): pass `e` to `shouldStartDragging` [#9843](https://github.com/fabricjs/fabric.js/pull/9843)
- feat(IText): Adjust cursor blinking for better feedback [#9823](https://github.com/fabricjs/fabric.js/pull/9823)
- feat(FabricObject): pass `e` to `shouldStartDragging` [#9843](https://github.com/fabricjs/fabric.js/pull/9843)
- ci(): Add Jest coverage to the report [#9836](https://github.com/fabricjs/fabric.js/pull/9836)
- test(): Add cursor animation testing and migrate some easy one to jest [#9829](https://github.com/fabricjs/fabric.js/pull/9829)
- fix(Group, Controls): Fix interactive group actions when negative scaling is involved [#9811](https://github.com/fabricjs/fabric.js/pull/9811)
Expand Down
17 changes: 11 additions & 6 deletions src/shapes/IText/ITextBehavior.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,21 +145,26 @@ export abstract class ITextBehavior<
});
}

/**
* changes the cursor from visible to invisible
*/
private _tick(delay?: number) {
this._currentTickState = this._animateCursor({
toValue: 1,
duration: this.cursorDuration,
delay,
toValue: 0,
duration: this.cursorDuration / 2,
delay: Math.max(delay || 0, 100),
onComplete: this._onTickComplete,
asturur marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel strongly against recursive/looping methods calling each other.
IMO a recursive/looping method should call ONLY itself.
Spreading recursion across multiple methods is confusing and is prone for inifinte loops.

Copy link
Contributor

Choose a reason for hiding this comment

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

Therefore, I suggest to make it a single function accepting a boolean flagging the state

Copy link
Member

Choose a reason for hiding this comment

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

Just open an issue with label enanchement, the first one that can do it will do it if there is a real improvement of some kind, i can't see one that make me jump at it.
If you look it from a different point of view tick and tickComplete are 2 wrappers around animateCursor, and is just animateCursor calling itself with configuration A or B.

The issue with not having 2 methods is that you have to use that boolean to determine the 3 conditions every time you call the single method and you don't have a single place where to determine the first delay ( delay || 100 ) so it will be harder to track which is the start of the animation.
Today that is done implicitly by calling _onTick bound without parameters by onComplete

Copy link
Member

@asturur asturur Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have other questions on this code more than the double methods:

  • why do we need to track the 2 animations with 2 separate states ( they are running only in sequence )
  • why onTickComplete does a pre abort check while onTick doesnt

but also those are out of scope for the animation timing tweak

Copy link
Member

Choose a reason for hiding this comment

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

here is a tracking issue #9835

});
}

/**
* Changes the cursor from invisible to visible
*/
private _onTickComplete() {
this._currentTickCompleteState?.abort();
this._currentTickCompleteState = this._animateCursor({
toValue: 0,
duration: this.cursorDuration / 2,
delay: 100,
toValue: 1,
duration: this.cursorDuration,
onComplete: this._tick,
});
}
Expand Down
Loading