Fix crash when opening second script in French C3D#9994
Closed
angelowang wants to merge 1 commit intoDynamoDS:RC2.4.1_masterfrom
angelowang:RC2.4.0_master
Closed
Fix crash when opening second script in French C3D#9994angelowang wants to merge 1 commit intoDynamoDS:RC2.4.1_masterfrom angelowang:RC2.4.0_master
angelowang wants to merge 1 commit intoDynamoDS:RC2.4.1_masterfrom
angelowang:RC2.4.0_master
Conversation
Steps to reproduce: - Launch Dynamo in French OS and French Civil 3D. - Open from `Help` menu: `Help>Samples>Core>Core_ListAtLevel.dyn` - Without close the dyn, open from `Help` menu: `Help>Samples>Core>Core_ListLacing.dyn`. - Exception dialog comes and C3D will crash. The exception arises when converting `1.0` to double under `fr-FR` culture, in `GroupFontSizeToEditorEnabledConverter.Convert()` on `values[0]`. Questions: - Where does this `1.0` come from? - It is from `AnnotationView.xaml`, where `FallbackValue` of the `Visibility` property is set to `1.0`. When we switch from first dyn to the second, the current workspace will be removed, triggered WPF binding updates. - Why it doesn't crash when we do the same operation in DynamoSandbox? - WPF actually doesn't follow `CurrentCulture` or `CurrentUICulture` when resolving the binding (https://weblog.west-wind.com/posts/2009/Jun/14/WPF-Bindings-and-CurrentCulture-Formattings. So by default, the `culture` passed to the `Convert` method is always `en-US`. So it is OK. - How to reproduce with DynamoSandbox? - You have to manually add several lines of code to DynamoCore entry fuction. ``` CultureInfo.DefaultThreadCurrentCulture = CultureInfo.CurrentCulture = new CultureInfo("fr-FR"); CultureInfo.DefaultThreadCurrentUICulture = CultureInfo.CurrentUICulture = new CultureInfo("fr-FR"); FrameworkElement.LanguageProperty.OverrideMetadata(typeof(FrameworkElement), new FrameworkPropertyMetadata(XmlLanguage.GetLanguage(CultureInfo.CurrentCulture.IetfLanguageTag))); ``` - Why does Civil 3D have `fr-FR` culture? - People buying Civil 3D will get Map 3D for free. The Map 3D dll, `Autodesk.Map.IM.Loader.dll`, will override the Culture to be used by all WPF bindings, with the very similar code as above. When I rename it, the crash goes away. - So how to fix? - At this stage, the safest fix would be simply changing `1.0` to `1`. Later we should evaluate whether we really need to simply use the InvariantCulture in all our binding coverters. We have unit tests for those converters, so also need to remove/change them if decided to go that way.
Member
|
Thanks for the write up @angelowang - I think I understand whats happening now. This will need to end up getting merged to a different branch, and like you mentioned above I think we should add some try/catch logic, and a test for this situation. (we can try to set the localization to FR just for the test, and then set it back after). We'll make a PR. |
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please Note:
DynamoRevitrepo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after aLGTMlabel is added to the PR.Purpose
Fix crash when opening second script in French C3D
Steps to reproduce:
Helpmenu:Help>Samples>Core>Core_ListAtLevel.dynHelpmenu:Help>Samples>Core>Core_ListLacing.dyn.The exception arises when converting
1.0to double underfr-FRculture,in
GroupFontSizeToEditorEnabledConverter.Convert()onvalues[0].Questions:
Where does this
1.0come from?It is from
AnnotationView.xaml, whereFallbackValueof theVisibilityproperty is set to
1.0. When we switch from first dyn to the second, thecurrent workspace will be removed, triggered WPF binding updates.
Why it doesn't crash when we do the same operation in DynamoSandbox?
WPF actually doesn't follow
CurrentCultureorCurrentUICulturewhen resolving the binding (https://weblog.west-wind.com/posts/2009/Jun/14/WPF-Bindings-and-CurrentCulture-Formattings.
So by default, the
culturepassed to theConvertmethod is alwaysen-US.So it is OK.
How to reproduce with DynamoSandbox?
You have to manually add several lines of code to
DynamoSandboxentry fuction, it will crashalthough no exception dialog (interestingly, it pops up IE on my side).
Why does Civil 3D have
fr-FRculture?People buying Civil 3D will get Map 3D for free. The Map 3D dll,
Autodesk.Map.IM.Loader.dll,will override the Culture to be used by all WPF bindings, with the very similar code as above.
When I rename it, the crash goes away.
So how to fix?
At this stage, the safest fix would be simply changing
1.0to1.Later we should evaluate whether we really need to simply use the InvariantCulture
in all our binding coverters. We have unit tests for those converters, so also
need to remove/change them if decided to go that way.
Another way, we might should catch all the exception and
Converter.ToDouble()and use invariant culture if failed..Declarations
Check these if you believe they are true
*.resxfilesReviewers
Michael Kirschner
Aaran Tang
FYIs