update forge units package

[pinzart90]

Purpose

Update the ForgeUnits nuge package to 4.1.1

Declarations

Check these if you believe they are true

• The codebase is in a better state after this PR
• Is documented according to the standards
• The level of testing this PR includes is appropriate
• User facing strings, if any, are extracted into *.resx files
• All tests pass using the self-service CI.
• Snapshot of UI changes, if any.
• Changes to the API follow Semantic Versioning and are documented in the API Changes document.
• This PR modifies some build requirements and the readme is updated
• This PR contains no files larger than 50 MB

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

[github-actions]

⚠️ [run-bin-diff-net60-windows] - Files Added/Deleted::19 new file(s) have been added and 1 file(s) have been deleted!
(Updated: 2023-10-11-19:34:37)

[mjkkirschner]

did this namespace change? If so why is this package not version 5?

[pinzart90]

answered in the general chat

[mjkkirschner]

@pinzart90 I'm a bit concerned, maybe without reason about the namespace change? If others have referenced these types to use the unit types in their nodes won't they need to recompile their nodes on the new version?

Oh I guess they will really only reference the Dynamo wrapper types, so it should be fine. Though my other question stands about ForgeUnits versioning strategy.

Is ForgeUnits not semantically versioned?

[pinzart90]

@pinzart90 I'm a bit concerned, maybe without reason about the namespace change? If others have referenced these types to use the unit types in their nodes won't they need to recompile their nodes on the new version?

Oh I guess they will really only reference the Dynamo wrapper types, so it should be fine. Though my other question stands about ForgeUnits versioning strategy.

Is ForgeUnits not semantically versioned?

ForgeUnits is semantically versioned.
The move from 4.0.3 to 4.1.1 should not impact Dynamo API users that much, since (as you said) they only consume the Dynamo wrappers.
About the namespace change: I had to change the namespace because the ForgeUnits package has 2 assemblies the old CLR dll and the new swig dll (I guess the FOrgeUnits devs want to support both for a while). If both dlls are referenced at compile time(which happens out of the box when referencing a nuget package) then we will get a namespace collision with the already defined alias in the dynamo code. There are other ways to fix it (I think), maybe to explicitely skip referencing the old CLR dll....but I chose to rename it.
Also the old CLR dll should not be copied in the output folder (since we are not using any of its types) - I hope. I will check this

[mjkkirschner]

....

good info, thanks for sharing it.

[pinzart90]

Looks like updating to the new forgeUnits will break some existing units nodes (specifically the dropdown UI nodes - ex. that list all units, all symbols or all quantities maybe some others too)
Looks like we are serializing the index in dropdown nodes which will break if the underlying list of items changes (which happened for this upgrade)
Do you guys think this is serious enough to try to migrate in some way ?
FYI @mjkkirschner @sm6srw

[pinzart90]

Maybe this only happens when the dropDown's SelectedString property does not find a match in the existing itemList.
So an edge case...

[pinzart90]

Ugh...it is weird. Looks like DropDown nodes serialize both the SelectedString and the SelectedIndex. No sure about the order of deserialization. When one is deserialized it tries to override the other....
https://github.com/DynamoDS/Dynamo/blob/master/src/Libraries/CoreNodeModels/DropDown.cs#L91-L147

[pinzart90]

@QilongTang Looks like you implemented the serialization of DropDown SelectedString and SelectedIndex.
#9367
Did you expect that the SelectedIndex might become out of sync with SelectedString ?

[QilongTang]

SelectedString is deserialized after SelectedIndex, so when the index is out of sync because of item sources updated, changed, we fall back to SelectedString later as a second chance. Let me know if that is clear

[pinzart90]

Ok..so this is only an issue when the SelectedString is no longer found. That means it was bad from the start .... or it is missing from the underlying item list, which would be a breaking change (so it might acceptable for it not to work properly)