FreeCAD
Rename Label2 to Description
This PR is meant to address issue https://github.com/FreeCAD/FreeCAD/issues/14344
Now the property field matches the name in the Tree View field. This is my first contribution. Please let me know if I've missed any steps.
The CI failures appear to be because CAM creates its own "Description" field, which then conflicts with this one. @sliptonic can you confirm?
This is a welcome change!
The change in ifc_tools need to be changed too. Basically there is no more need for that piece of code, it was there to automatically bind the Description property to Label2.
For CAM, if it adds a "Description" property, a simple fix is to add something like this before:
if "Description" not in obj.PropertiesList:
I am removing the 1.0 milestone. Let's not get carried away. We should not do this in the current dev cycle.
Where is the code that transfers the old Label2 property? Or am I missing something?
I am removing the 1.0 milestone. Let's not get carried away. We should not do this in the current dev cycle.
Agree. This needs proper testing.
Where is the code that transfers the old Label2 property? Or am I missing something?
That's right. Although Label2 is AFAICS rarely used, it might be important to some, and indeed we should have some code that detects the Label2 property on file load and transfers its value to Description.
Hi, sorry, I'm not familiar with how to resolve the conflicts since it compiled fine on my system. Could someone please provide additional details? Thanks!
On Tue, Nov 26, 2024 at 7:28 AM Max Wilfinger @.***> wrote:
@matt-taylor-git https://github.com/matt-taylor-git ping
— Reply to this email directly, view it on GitHub https://github.com/FreeCAD/FreeCAD/pull/16990#issuecomment-2500668316, or unsubscribe https://github.com/notifications/unsubscribe-auth/APOLI3Q2X2GQKX6MOEQDK4L2CRSQFAVCNFSM6AAAAABPKQMVISVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMBQGY3DQMZRGY . You are receiving this because you were mentioned.Message ID: @.***>
This change looks potentially dangerous as it breaks backwards compatibility with no clear gains. Why not just adding the new Description property while making the old Label2 hidden by default?
This change looks potentially dangerous as it breaks backwards compatibility with no clear gains. Why not just adding the new Description property while making the old Label2 hidden by default?
I don't fully agree there.. Label2 was wrongly named from the start. I don't think it's used anywhere (outside LinStage3 maybe, but that is anyway not in sync with main FreeCAD anymore), this is a good opportunity to set things right.
IMHO if something breaks anyway, the only way to find out and fix is by having this merged in main and let people test...
Hi, sorry, I'm not familiar with how to resolve the conflicts since it compiled fine on my system. Could someone please provide additional details? Thanks!
You basically need to rebase your branch onto main:
git checkout main
git pull
git checkout geit-rename-Label2
git rebase main
Then you will likely get an error and a src/App/DocumentObject.cpp file with >>>>>, ====== and <<<<< characters. Edit the file, and fix what needs to be fixed), then:
git add src/App/DocumentObject.cpp
git rebase --continue
when everything is fixed, force-push the updated version (use the name of your own github repo below):
git push -f your-own-github-remote
@yorikvanhavre I agree that Label2 was a mistake, but it has been there for ages and it is public API, usually removing old public APIs should follow a deprecation path, making them explicitly deprecated and hidden from new devs with a clrear note on when it will be removed, so people can do the changes in advance. Probably the specific case of Label2 is not too important as it is of very low impact. But removing public APIS overnight doesn't looks good. My concern is not much about Label2 specifically but about the deprecation process.
Okay, agree with your comments! We should define a procedure there I guess...
@matt-taylor-git I cannot reopen, can you reopen? Last time it said that you already have a PR with the same name so I changed the name and closed the other one but it still won't let me reopen this one. Maybe rebase and push again?
It says there are no new commits:
@matt-taylor-git I cannot reopen, can you reopen? Last time it said that you already have a PR with the same name so I changed the name and closed the other one but it still won't let me reopen this one. Maybe rebase and push again?
Looks like it's finally working. It originally wouldn't let me reopen it either
You've still got some extraneous changes in there, and we are at an impasse: we need to decide how to update the public API for this entry. @yorikvanhavre we have added this discussion to the next developer's meeting.
@yorikvanhavre what would happen in this case with existing files that have a "Description" property added by BIM and have LAbel2 pointing to it with an expression?
Normally, everywhere in BIM when a "Description" property is added, it checks first if that property already exists before adding a new one. If it exists, then the existing property is used. As a general rule, when dealing with dynamic properties, one is never sure if that name is available and one always needs to be cautious
In any case, if a "Description" property already exists, and you do add a "Description" dynamic property, it will work. Only, your new property will be called Description1. So the risk to break something here is pretty low.
The major problem here is if something, somewhere, is expecting a Label2 property. But I don't have the impression that any important workbench (both builtin and addon) uses Label2. Maybe the LinkStage branch, @realthunder ?
Normally, everywhere in BIM when a "Description" property is added, it checks first if that property already exists before adding a new one. If it exists, then the existing property is used. As a general rule, when dealing with dynamic properties, one is never sure if that name is available and one always needs to be cautious
In any case, if a "Description" property already exists, and you do add a "Description" dynamic property, it will work. Only, your new property will be called Description1. So the risk to break something here is pretty low.
my concern is not about what BIM will do with new files after the change but rather how will existing files where the Description was added and an expression was set for Label2=Description will migrate.
my concern is not about what BIM will do with new files after the change but rather how will existing files where the Description was added and an expression was set for Label2=Description will migrate.
Hm you're right... That expression will probably yield an error when opening. But I suppose we can fix that by putting something in the code to wipe the expression on load. But yeah, it would need some testing to be sure
I chose the name Lable2 instead of a more common name such as Description to avoid potential conflict with external workbench. My original intention is to hide Lable2 from end user and expose it as extra tree view column.
Thanks @realthunder ! Then I guess it's basically safe to rename. About the BIM expression, I can give this a test and see how that turns out, but this PR seems to include several changes that are not related to this... Could you have a look @matt-taylor-git ?
I would advise to put this on hold: I'm going to work on renaming properties in the near future and one suggestion in the discussion was to create an alias system exactly for the problem of Label2 and Description, see this forum post. I'm not sure whether I can actually incorporate that, but I could certainly try. A proper alias system would allow us to maintain backward compatibility while changing the property name from Label2 to Description.
Thanks for helping improve FreeCAD! This pull request hasn’t seen activity in a while. We automatically check each PR after 120 days without activity to keep the repository tidy.
If the PR is still relevant, let us know by adding a comment. Otherwise, we’ll close this PR automatically in 60 days.
If you would like to keep working on this pull request, we advice to rebase it on current main branch, ask feedback from users or maintainers and engage with the community to get it forward.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}