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.

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

revert(): rm active selection ref #9561

Merged
merged 6 commits into from
Jan 9, 2024
Merged

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Dec 29, 2023

Description

This PR reverts the active selection ref on the canvas.
It was a bad decision that opened odd bugs and bad design
Voids the need for #9525

In Action

Copy link
Contributor

github-actions bot commented Dec 29, 2023

Build Stats

file / KB (diff) bundled minified
fabric 907.592 (-0.183) 304.760 (-0.350)

@ShaMan123 ShaMan123 marked this pull request as draft December 29, 2023 08:59
@ShaMan123 ShaMan123 requested a review from asturur January 6, 2024 10:31
@ShaMan123
Copy link
Contributor Author

@asturur please review this so we can decide on the right way forward

src/canvas/Canvas.ts Outdated Show resolved Hide resolved
@asturur
Copy link
Member

asturur commented Jan 7, 2024

i am ok reverting this feature, i was neutral about it, if the code in this way is simpler i prefer to roll it back

@asturur
Copy link
Member

asturur commented Jan 7, 2024

I think we should use the classRegistry to load the ActiveSelection class and we should avoid to use instanceOf for it.
I can do those changes if you are in rush of merging and you don't have time today, otherwise you can do them

@ShaMan123
Copy link
Contributor Author

I think we should use the classRegistry to load the ActiveSelection class and we should avoid to use instanceOf for it. I can do those changes if you are in rush of merging and you don't have time today, otherwise you can do them

I will do it now.
What else apart from instanceof? Back to isActiveSelection?

@asturur
Copy link
Member

asturur commented Jan 8, 2024

I think we should use the classRegistry to load the ActiveSelection class and we should avoid to use instanceOf for it. I can do those changes if you are in rush of merging and you don't have time today, otherwise you can do them

I will do it now. What else apart from instanceof? Back to isActiveSelection?

yes something that can detect withou using the class.

if we didn't have selections of objects inside a group we could just say that the activeObject is not in canvas, that would grant is an active selection, but now we have to flag it maybe recognizing the stac property we added recently or something else

@ShaMan123 ShaMan123 force-pushed the revert-active-selection-ref branch 2 times, most recently from c421e98 to d72ab68 Compare January 8, 2024 10:40
Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

All tests should pass now
Remaining is to decide about how to handle instanceof ActiveSelection
Since there is no issue with import cycles I don't mind leaving it as is.
I could re-expose isActiveSelection using classRegistry

const isActiveSelection = (object: FabricObject) =>
  object instanceof
  classRegistry.getClass<typeof ActiveSelection>('ActiveSelection');

Is that better?

@ShaMan123 ShaMan123 marked this pull request as ready for review January 8, 2024 10:46
@ShaMan123 ShaMan123 requested a review from asturur January 8, 2024 10:47
@ShaMan123 ShaMan123 changed the title patch(): rm active selection ref revert(): rm active selection ref Jan 8, 2024
@asturur
Copy link
Member

asturur commented Jan 8, 2024

All tests should pass now Remaining is to decide about how to handle instanceof ActiveSelection Since there is no issue with import cycles I don't mind leaving it as is. I could re-expose isActiveSelection using classRegistry

const isActiveSelection = (object: FabricObject) =>
  object instanceof
  classRegistry.getClass<typeof ActiveSelection>('ActiveSelection');

Is that better?

That would be fine imho, but is slightly different from the others, but does the job.
Wouldn't be applicable to the other functions we have there, but for the specific case of activeSelection i think is fine.

@asturur
Copy link
Member

asturur commented Jan 8, 2024

not sure typing getClass in that way is a good move, but we can still default to any as it was before and patch where it creates issues

@ShaMan123
Copy link
Contributor Author

not sure typing getClass in that way is a good move, but we can still default to any as it was before and patch where it creates issues

Can you elaborate?
I don't really like passing typeof Class and prefer passing Class but TS can't easily support that
Also I thought of adding a default any

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Jan 8, 2024

My thoughts about isActiveSelection still stand, please consider.
It is used only by Canvas/SelectableCanvas. These classes depend on ActiveSelection. So there is no reason not to import ActiveSelection and use instanceof, de facto no import cycles were created.
I think isActiveSelection is messy and weird and opens up strange issues with the class registry and it means the ActiveSelection was imported anyways so why not just do it without this workaround?
Anyways I will expose the util, it is easy to expel it later on

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

Looks ready

@ShaMan123
Copy link
Contributor Author

jest just proved that.
waiting for your review on this

My thoughts about isActiveSelection still stand, please consider. It is used only by Canvas/SelectableCanvas. These classes depend on ActiveSelection. So there is no reason not to import ActiveSelection and use instanceof, de facto no import cycles were created. I think isActiveSelection is messy and weird and opens up strange issues with the class registry and it means the ActiveSelection was imported anyways so why not just do it without this workaround? Anyways I will expose the util, it is easy to expel it later on

@asturur
Copy link
Member

asturur commented Jan 9, 2024

not sure typing getClass in that way is a good move, but we can still default to any as it was before and patch where it creates issues

Can you elaborate? I don't really like passing typeof Class and prefer passing Class but TS can't easily support that Also I thought of adding a default any

My thoughts about isActiveSelection still stand, please consider. It is used only by Canvas/SelectableCanvas. These classes depend on ActiveSelection. So there is no reason not to import ActiveSelection and use instanceof, de facto no import cycles were created. I think isActiveSelection is messy and weird and opens up strange issues with the class registry and it means the ActiveSelection was imported anyways so why not just do it without this workaround? Anyways I will expose the util, it is easy to expel it later on

Doing isActiveSelection or doing instanceOf ActiveSelection is the same exact thing, at the net of the forced import.
Canvas do not depend on activeSelection if you don't plan to use it and disable the multi selection entirely and anyway being splittable is a target goal.
So if we have to check for a switch in a piece of code is better we do it in a way it doesn't bind the code together.

.getClass(obj.type)
.fromObject(obj, {
signal,
reviver,
Copy link
Member

Choose a reason for hiding this comment

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

ok i wonder how reviver ended up here in the refactors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just because of the type change

Copy link
Member

Choose a reason for hiding this comment

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

yes but it never was part of fromObject, why did we put it there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a good question for the blame and spare time to answer

instance.group === this._activeSelection &&
this._activeObject === instance.group
) {
if (instance.group && isActiveSelection(instance.group)) {
Copy link
Member

Choose a reason for hiding this comment

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

this check is different now, we still have to check that instance.group is activeObject
Being an activeSelection, doesn't mean you are the active one.

@asturur
Copy link
Member

asturur commented Jan 9, 2024

using the classRegistry check with Jest is broken because jest imports only what it needs, so i had to revert it to checking a random exclusive property

Copy link
Contributor

github-actions bot commented Jan 9, 2024

Coverage after merging revert-active-selection-ref into master will be

82.82%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 35, 38, 41
src
   ClassRegistry.ts100%100%100%100%
   Collection.ts94.85%94.83%87.10%97.14%109, 112, 215–216, 241–242
   CommonMethods.ts96.55%87.50%100%100%9
   Intersection.ts100%100%100%100%
   Observable.ts87.76%85.29%87.50%89.58%134–135, 160–161, 32–33, 41, 50, 78, 89
   Point.ts100%100%100%100%
   Shadow.ts98.36%95.65%100%100%178
   cache.ts97.06%90%100%100%57
   config.ts74.14%65%66.67%82.76%131, 139, 139, 139, 139, 139–141, 152–154, 30
   constants.ts100%100%100%100%
src/LayoutManager
   LayoutManager.ts88.05%89.83%76.92%90.54%136–137, 139–140, 140, 140, 140, 140, 234, 297–298, 309, 47
   constants.ts100%100%100%100%
src/LayoutManager/LayoutStrategies
   ClipPathLayout.ts82.05%72.22%100%88.89%30–31, 42, 54, 57–58, 65
   FitContentLayout.ts100%100%100%100%
   FixedLayout.ts20%0%0%40%20–21, 23, 23, 23, 23, 23
   LayoutStrategy.ts86.54%64.71%100%96.30%36, 43, 43, 43, 51, 78–79
   utils.ts91.67%85.71%100%100%28, 34
src/Pattern
   Pattern.ts90.54%93.94%80%90.32%119, 130, 139, 32, 36
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%0%0%0%108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 13, 136, 138, 23–24, 32–36, 40–44, 51–54, 62–66, 68, 76, 76, 76, 76, 76–77, 79, 79, 79–82, 84, 92–93, 95, 97–99
   PatternBrush.ts97.06%87.50%100%100%21
   PencilBrush.ts91.06%82.35%100%93.81%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%0%0%0%107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 148, 148, 148–151, 153–156, 160–161, 163, 165–168, 17, 171, 178–179, 18, 181, 183–184, 186, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 211, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
   Canvas.ts79.02%76.99%83.05%79.88%1005–1006, 1006, 1006–1007, 1013, 1015, 1043–1045, 1048–1049, 1053–1054, 1177–1179, 1182–1183, 1256, 1371, 1492, 158, 183, 289, 289–294, 299, 322–323, 328–333, 353, 353, 353–354, 354, 354–355, 363, 368–369, 369, 369–370, 372, 381, 387–388, 388, 388, 42, 431, 439, 443, 443, 443–444, 446, 46, 528–529, 529, 529–530, 536, 536, 536–538, 558, 560, 560, 560–561, 561, 561, 564, 564, 564–565, 568, 577–578, 580–581, 583, 583–584, 586–587, 599–600, 600, 600–601, 603–608, 614, 621, 658, 658, 658, 660, 662–667, 673, 679, 679, 679–680, 682, 685, 690, 703, 731, 783–784, 784, 784, 784, 784, 784, 787–788, 791, 791–793, 796–797, 879, 886, 886, 886, 899, 932, 953–954, 970–971, 971, 971–973, 976–977, 977, 977, 979, 987, 987, 987–989, 989, 989, 996–997
   CanvasOptions.ts100%100%100%100%
   SelectableCanvas.ts93.17%91.84%94.34%94.01%1001, 1009, 1120, 1122, 1124–1125, 460–461, 463–464, 464, 464, 513–514, 575–576, 589, 629–631, 663, 668–669, 696–697, 757–758, 763–767, 769, 928, 928–929, 932, 952, 952
   StaticCanvas.ts96.53%93.09%98.92%98.29%1019, 1029, 1080–1082, 1085, 1120–1121, 1197, 1206, 1206, 1210, 1210, 1257–1258, 178–179, 195, 559, 571–572, 902–903, 903, 903–904
   StaticCanvasOptions.ts100%100%100%100%
   TextEditingManager.ts86%71.43%91.67%91.67%17, 17, 17–18, 18, 18
src/canvas/DOMManagers
   CanvasDOMManager.ts95.52%70%100%100%21–22, 29
   StaticCanvasDOMManager.ts97.50%88.89%100%100%33
   util.ts86.67%80.56%83.33%93.94%14, 26, 63–64, 67, 67, 74, 93–94
src/color
   Color.ts94.96%91.67%96.30%96.05%233, 258–259, 267–268, 48
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts85.71%76.92%100%89.74%55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
   Control.ts94.44%93.10%91.67%96.77%183, 249, 354
   changeWidth.ts100%100%100%100%
   commonControls.ts100%100%100%100%
   controlRendering.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   drag.ts100%100%100%100%
   fireEvent.ts88.89%75%100%100%13
   polyControl.ts10.87%0%0%16.13%114, 114, 114, 114, 114, 116–119, 119, 122, 129, 24–26, 50–52, 58–59, 61, 71, 77–79, 79, 81, 84, 86, 90–92, 94, 99
   rotate.ts19.57%12.50%50%21.43%41, 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

@asturur
Copy link
Member

asturur commented Jan 9, 2024

i m good to go

@ShaMan123
Copy link
Contributor Author

we should follow up with removing the code in the layout manager shouldResetTransform

@ShaMan123 ShaMan123 merged commit 4979eec into master Jan 9, 2024
22 checks passed
@ShaMan123 ShaMan123 deleted the revert-active-selection-ref branch January 9, 2024 09:49
// clear active selection
if (obj === this._activeSelection) {
this._activeSelection.removeAll();
resetObjectTransform(this._activeSelection);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I removed this line but still the layout manager is present and does the same.
Let's see what will happen when I remove it

@asturur
Copy link
Member

asturur commented Jan 9, 2024

be mindful that this is not a revert.
Revert are safe by default they remove a PR as it was, usually from git revert command.
This should have been called refactor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants