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

Part: Rename AttachExtension::Support property - fixes #7052 #12714

Merged

Conversation

jcoalson
Copy link
Contributor

@jcoalson jcoalson commented Mar 3, 2024

⚠️ Supersedes #12579

Summary

Part: Rename AttachExtension::Support property to AttachmentSupport, to avoid name conflict with base features, e.g. ShapeBinder.

Fixes #7052

The above bug is triggered when using the attach extension on a binder.

Details

When projects are saved, the scoping of object properties is lost, which means a class must be careful not to use the same name for a property as another class if the two classes can be composed via inheritance or extension into a single feature.

AttachExtension and ShapeBinder currently conflict on Support. #12579 renames ShapeBinder's but in discussion it was noted that since AttachExtension is an App::Extension, it should be the one to avoid conflicts. This PR is longer due to the ubiquity of attachments, and trickier due to the lack of compile-type checking on the python side, but I think it's worth it.

The approach taken here is to do a full migration instead of keeping a deprecated Support property around, which could get very confusing since attachments are so widely used.

While I was there, I moved the migration for an old type change out of Part2DObject into AttachExtension where it belongs, which cleaned up some cruft.

@github-actions github-actions bot added Mod: FEM Related to the FEM Workbench Mod: BIM Related to the BIM/Arch Workbench Mod: Draft Related to the Draft Workbench Mod: TechDraw Related to the TechDraw Workbench Mod: Part Design Related to the Part Design Workbench Mod: Part Related to the Part Workbench Mod: Sketcher Related to the Sketcher Workbench labels Mar 3, 2024
@jcoalson
Copy link
Contributor Author

jcoalson commented Mar 3, 2024

PS I didn't update any of the numerous docstrings, I assume that happens on crowdin?

@adrianinsaval
Copy link
Member

PS I didn't update any of the numerous docstrings, I assume that happens on crowdin?

are you talking about documentation in cpp/py files or in .ts files? The later is managed with crowdin but code documentation should be updated here.

@jcoalson
Copy link
Contributor Author

jcoalson commented Mar 3, 2024

I meant the .ts files. I updated everywhere else.

@chennes chennes requested a review from wwmayer March 4, 2024 16:47
@adrianinsaval adrianinsaval merged commit a8ae56e into FreeCAD:main Mar 4, 2024
10 checks passed
@adrianinsaval
Copy link
Member

Let's ship it and keep an eye out for complaints to fix

@Zolko-123
Copy link
Contributor

Rename AttachExtension::Support property to AttachmentSupport,

Are you MAD ? Do you know how many things depend on this ?

This bug is triggered when using the attach extension on a binder

Who uses an attachment with a ShapeBinder ???

@jcoalson
Copy link
Contributor Author

jcoalson commented Mar 8, 2024

Rename AttachExtension::Support property to AttachmentSupport,

Are you MAD ? Do you know how many things depend on this ?

Nothing breaks, old projects are automatically migrated when loaded. edit: I guess you mean 3rd party code.

This bug is triggered when using the attach extension on a binder

Who uses an attachment with a ShapeBinder ???

How would you solve this problem without it?

@Zolko-123
Copy link
Contributor

YOUR solution to YOUR problem is to destroy every-body elses work ?

@adrianinsaval
Copy link
Member

THE solution to A problem... and it's fairly easy to adapt your code. I think something like this should work:
0001-Port-to-FreeCAD-0.22.0.36274.patch.txt

@Zolko-123
Copy link
Contributor

How many people did have this "problem" that you talk about ? Where was it ever reported ?

@jcoalson
Copy link
Contributor Author

There are multiple forum threads and at least one github issue, linked right here in the PR comments.

This is your third comment about things discussed for over a year and accessible through this very same page. I'm not sure why you don't want to read about it first before commenting.

@snelweg
Copy link
Contributor

snelweg commented May 8, 2024

Wow, this is so stupid and will make FC even more unloved.
How can you ruin someone's work like a dictator? Fix the unbelievable still existing topo error if you want to fix a REAL problem. Did the guys at Ondsel make you do this?
F*ck FC, moving on to OnShape.

@jcoalson
Copy link
Contributor Author

i still love you freecad

@3x380V
Copy link
Contributor

3x380V commented Jun 3, 2024

@jcoalson, ordinary love needs some proof occasionally to not perish, so let me introduce a challenge in file. In was created using 0.21.2:
Screenshot from 2024-06-03 09-44-22
As you can see, all datums have Support assigned. However opening the very same file in current version leaves AttachmentSupport empty:
Screenshot from 2024-06-03 09-47-16
which breaks all the models using datums and local coordinate systems and hopefully explains all that angry comments you read above. Also note @FlachyJoe's statement in #12894 (comment)

@FlachyJoe
Copy link
Contributor

in current version

Which one? See #14429 too.

@3x380V
Copy link
Contributor

3x380V commented Jun 3, 2024

Top most commit is 26714820cdfe ("Fix compiler warnings"), #14429 is not related to this problem although it is included in tested version.

@3x380V
Copy link
Contributor

3x380V commented Jun 4, 2024

Which one? See #14429 too.

It is #13964, which brings code unaware of property rename. Also extHandleChangedPropertyName was renamed meantime. I will create separate issue for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mod: BIM Related to the BIM/Arch Workbench Mod: Draft Related to the Draft Workbench Mod: FEM Related to the FEM Workbench Mod: Part Design Related to the Part Design Workbench Mod: Part Related to the Part Workbench Mod: Sketcher Related to the Sketcher Workbench Mod: TechDraw Related to the TechDraw Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Problem] PartDesign: Attachment of (sub-object) shapebinder co-modifies wrong field
6 participants