fix(Group): layout manager fromObject
#9614
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
Build Stats
|
| it('should subscribe objects when created `fromObject`', async () => { | ||
| const objectData = { | ||
| width: 2, | ||
| height: 3, | ||
| left: 6, | ||
| top: 4, | ||
| strokeWidth: 0, | ||
| objects: [ | ||
| new Rect({ | ||
| width: 100, | ||
| height: 100, | ||
| top: 0, | ||
| left: 0, | ||
| strokeWidth: 0, | ||
| }).toObject(), | ||
| ], | ||
| }; | ||
|
|
||
| const group = await Group.fromObject(objectData); | ||
| const rect = group.item(0); | ||
| expect( | ||
| Array.from(group.layoutManager['_subscriptions'].keys()) | ||
| ).toMatchObject([rect]); | ||
| }); |
There was a problem hiding this comment.
will fail on master
|
@zhe-he take a look |
|
Please remove the bug in its own PR, so that can go in in any moment. I don't want to discuss the api change before we know why this would be necessary at all. |
|
will do |
|
Wait, not good. |
|
shows that it is better to stick to the API |
|
Hmm... I'll explain why I customized this. Similar to Figma's Now I'm solving this issue by setting it again after initialization. I think this approach (LayoutStrategy#shouldPerformLayout) should also work. I'll look into it first. Thank you for your suggestion. |
|
@zhe-he but what is wrong with the suggestion of just having your fromObject method in your class? This proposal of fromObject is a step back in the world of 'initialized: true/false' that we always said we don't like. |
can you explain this comment here? |
|
the fromObject with the flag is a big no go for me. I rather prefer an extra type than a flag. |
|
or make a direct call to the object registration after assigning the correct layout manager |
I may not modify the properties of child objects on the canvas, but instead directly modify the properties of child objects in the exported serialized data. In this case, I have two options in front of me: one is to modify the parent object's properties in the exported serialized data through computation, and the other is to recompute the parent object's properties at initialization. If I choose the second approach, I cannot trust its serialized representation. So I would like to execute a custom performantLayout instead of an empty performantLayout during initialization. I see that the fromObject method has been added here, and by modifying the value of _fromObject, the performLayout is no longer executed when loading serialized data. If I don't trust the serialized data, I can simply modify the return value of shouldPerformLayout in the custom object to execute performLayout during initialization. I think this method is great.
Can we add an additional property 'dirty' to the LayoutManager, with a default value of false or undefined? If the user manually changes it to true, performLayout will be executed during initialization; otherwise, it won't be executed. |
|
I like both approaches |
|
The second solution I want to propose is probably like this. class Group {
// noop
fromObjectPerformLayout() {},
fromObject() {
// All layouts skip performLayout during initialization.
const group = new this(objects, ...);
// Custom layouts can override the performLayout2 method to recalculate.
group.fromObjectPerformLayout();
}
}class Frame extends Group {
fromObjectPerformLayout() {
this.layoutManager.performLayout({ type: "xxx" });
}
} |
|
The old pr can't be easily reverted. |
|
@zhe-he is still unclear why you can't write your own fromObject method in the Frame class, that just adds the layout manager, exactly as you did with your initial pr. |
Hmm... I can modify the fromObject method in Frame to solve my issue. |
|
If not modified, all custom layout objects will not be initialized during data import. If they need to be, they must override the fromObject method. |
|
@zhe-he i still think i have very little informations to give you suggestions. |
|
@asturur |
|
The registration bug needs to be fixed, and i don't want to do internal flags for fromObject. |
|
well what i wanted to do can't be really done as good as i wish because i don't like the NoopClassManager hack i have to add. It would be also ok to make subscribe public, and just call it on every object in the Group FromObject. I didn't even test it for now, i wanted to put on the table first. I have some notes on the context, one is that i can't see where we use 'stopPropagaton' at all, and i wonder why is there. |
|
I think after much thinking my preferred solution is this one: Just expose the subscription and call it when necessary. Please let me know, i see you are maybe all busy, but if in 1 or 2 weeks i don't get a feedback i ll just move on. |
|
What about adding a prop/flag/indication to the options passed by I think this is so much easier and straight forward and provides useful context. We can protect it by wrapping in with a private class and checking instanceof if you want to be protective but I am not sure that is needed class DeserializedOptions {
constructor(options) {
Object.assign(this, options)
}
}
class FabricObject {
static fromObject(options) {
return new FabricObject(new DeserializedOptions(options));
}
constructor(options) {
const isDeserialized = options instanceof DeserializedOptions
}
} |
|
I dislike both of the suggestions you provided, I commented in the PR |
|
@ShaMan123 'dislike' is a sensation that does not really matter. You pointed out the restriction in the api context and that has been restored, what else isn't working in your opinion? We both said in the past that internal flags to say 'initialized: true/false' or specific flags to initialize the object differently were something that long term didn't work out. That is why i don't wan't to add back a flag like fromObject() or '{ fromObject: true }'. |
Description
Motivation #9605
A different approach to the
NoopLayoutManagerThis approach uses the standard API to skip layout:
shouldPerformLayoutand makes it an edge case handled within the API and not externally, making it even easier to use because nowshouldPerformLayouthandles all casesDuring this PR I found a bug:
Since the
NoopLayoutManagerskippedperformLayoutchildren were not subscribed on initialization. This would not have happened if we noopcalcLayoutResultwhich is what this PR does using the APII also removed the
setCoordscall because it was introduced in #9522 (comment) wronglyIn Action