Action items from meetings

[asturur]

** previous **

Layout manager

#9144 -> this i have to re read.

#9142 -> revisit and add activeSelection specs.

#9068 -> goes in with test, priority 1 done merged

#9043 -> i have to form an opinion. moved to post release

#8950 -> request a line change - shachar put back the line. done

Then after #8951 -> i will read it. done

6 november 2023

exit editing on deselection:
#8950 (comment)
Make proposal to handle this coherently across fabric, collect all the existing select/deselect side effects and plan how they could look like. -Shachar

absolute: we merge as it is, we follow up with better math for the padding handling - Andrea done

Gradient is using isPath() we want to stop that and verify path, polygon, polyline work all same with a svg visual test: PR open #9488

24 November 2023

• merging chore(): cleanup type assertions, expose Object#parent => rm isActiveSelection #8951 surface some need for documenting add/remove in group and some semplification possible

[ShaMan123]

exit editing on deselection:
#8950 (comment)
Make proposal to handle this coherently across fabric, collect all the existing select/deselect side effects and plan how they could look like. -Shachar

Insights from inspecting the code:

• onSelect has no side effects in the code
• onDeselect side effects can easily turn into a deselected event handler:
  • IText exits editing
  • ActiveSelection removes all objects

We should state in the jsdocs of these methods they should not contain any side effects

[asturur]

@ShaMan123 https://github.com/fabricjs/fabric.js/pull/9525/files figure out what is wrong or merge it.

[ShaMan123]

I commented there

[asturur]

@ShaMan123 take a decision/plan on the active selection reference on the canvas

[ShaMan123]

I looked into the code and I think it is best to change to ref concept.
I think the only change we need to do is to make the ref optional and clear it once the selection is discarded
That should be enough to satisfy our needs
We can remove the ref entirely and use instance ActiveSelection (since we import ActiveSelection in the canvas files there is not problem with imports)

[ShaMan123]

updated comment...

so actually we can remove the ref entirely and expose Canvas#getActiveSelection and that will replace all equality checks

- target === this._activeSelection
+ target === this.getActiveSelection()

getActiveSelection() {
   return this._activeObject instanceof ActiveSelection ? this._activeObject : undefined
}

[ShaMan123]

I did a draft #9561
We can discuss there

[asturur]

#9043 -> this is large and moved to post release. this is clean up internal state category

[asturur]

@ShaMan123 #9142 to write down what he wants to see improved in the interaction with active selections: if the changes come from a multiselect,a shift click, a drag select and so on.

[ShaMan123]

can be after the release
What is needed is context.
There are cases there is a need to know what triggered the object add/remove so custom logic can change the stack order for instance.
Maybe it is good enough to use the selection events

[asturur]

@asturur #9515 review and possibly merge

[asturur]

this is done

[asturur]

@asturur reopen and merge this: #9470 done

[ShaMan123]

we need to discuss async disposing as well before the release because of react

[ShaMan123]

I don't understand why you say it is not async.
#9554 states that react18 dev mode calls every useEffect twice to ensure it is cleaned up.
Since it is async at the second call fabric will throw the canvas is already initialized

[asturur]

As soon as canvas.dispose is called, our cleanup of the dom is sync.
If react run a piece of effect twice i hope it will call also the clean up function hopefully.
Whay is async is some later task we do on disposing the resoruces but has nothing to do with the dom cleanup.

  /**
   * Waits until rendering has settled to destroy the canvas
   * @returns {Promise<boolean>} a promise resolving to `true` once the canvas has been destroyed or to `false` if the canvas has was already destroyed
   * @throws if aborted by a consequent call
   */
  dispose() {
    !this.disposed &&
      this.elements.cleanupDOM({ width: this.width, height: this.height });

the part that matters to react, cleanupDOM is not async, this is what i mean.

[ShaMan123]

I understand now
So there should be no problem at all because I remember I added a test calling dispose more than once before it completes

[ShaMan123]

[ShaMan123]

Maybe what happens is that the dev is alarmed because dispose throws if it is called more than once.
Maybe we should change that?

[ShaMan123]

I think #8952 was forgotten

[asturur]

I think we can close this and replace isTextObject with something that doesn't use the type.
That refactor at this stage is off the table for me.