Improvement Number Format by jesusalvino · Pull Request #13744 · DynamoDS/Dynamo

jesusalvino · 2023-02-11T06:45:21Z

Purpose

Implement the improvement https://jira.autodesk.com/browse/DYN-4469

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

Reviewers

@zeusongit

FYIs

@RobertGlobant20 @filipeotero

jesusalvino · 2023-02-11T06:46:48Z

src/DynamoCoreWpf/Views/Preview/PreviewControl.xaml.cs

QilongTang · 2023-02-13T20:08:25Z

src/DynamoCoreWpf/Views/Preview/PreviewControl.xaml.cs

+            }
+        }
+
+        private bool IsDoubleRealNumber(string valueToTest)


Do we really need this function? Since this is only a one line if check, maybe we can just fold it to the function above?

if (double.TryParse(valueToTest, out double d) && !Double.IsNaN(d) && !Double.IsInfinity(d)) { return Convert.ToDouble(numberValue).ToString(nodeViewModel.DynamoViewModel.PreferenceSettings.NumberFormat); } else { return numberValue; }

Do we really need this function? Since this is only a one line if check, maybe we can just fold it to the function above?

if (double.TryParse(valueToTest, out double d) && !Double.IsNaN(d) && !Double.IsInfinity(d)) { return Convert.ToDouble(numberValue).ToString(nodeViewModel.DynamoViewModel.PreferenceSettings.NumberFormat); } else { return numberValue; }

just wanted to keep separate it, probably it would be part of a helper class or similar so we can move latter, like ADSK.Dynamo.Helpers.IsDoubleRealNumber (maybe we already have a similar) or you approach to keep it simple is totally valid, please your thoughts.

OK, in that case, I am fine with keeping it, but we should then probably move this function to one of DynamoUtilites file, like https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/Utilities/PreferencesPanelUtilities.cs. Also maybe pick a name easier to understand, like IsValidDoubleNumber?

QilongTang

LGTM with two comments

aparajit-pratap · 2023-02-14T17:40:04Z

@jesusalvino I think there was some work done on this before and this issue is not isolated to only String from Object node. Here is the PR that tries to fix this issue more holistically. There was some work left on the UI side to add a default format preference that would not impose any digits: https://jira.autodesk.com/browse/DYN-5101. FYI @Amoursol .

I feel we should consider that previous work before merging this fix that is specific to just one node. ⚠️

jesusalvino · 2023-02-16T14:51:14Z

@aparajit-pratap I have picked up your previous work and refactored the mine based on your suggestion in my last commit 584cbcb , your thoughts please.

aparajit-pratap · 2023-02-16T20:56:45Z

src/Engine/ProtoCore/Reflection/MirrorData.cs

+                        if (Data is double)
+                        {
+                            return (Data as IFormattable).ToString(PrecisionFormat, CultureInfo.InvariantCulture);
+                        }
+                        else


I would comment out this code for the time being and leave a TODO comment here saying "uncomment this once https://jira.autodesk.com/browse/DYN-5101 is complete".

aparajit-pratap · 2023-02-16T20:57:56Z

src/DynamoCoreWpf/ViewModels/Preview/WatchViewModel.cs

                default:
+                    if (double.TryParse(obj.ToString(), out double d))
+                    {
+                        return Convert.ToDouble(obj).ToString(PrecisionFormat, CultureInfo.InvariantCulture);


This should also be ProtoCore.Mirror.MirrorData.PrecisionFormat.

aparajit-pratap · 2023-02-16T20:59:26Z

src/DynamoCoreWpf/ViewModels/Preview/WatchViewModel.cs

                    return ObjectToLabelString(obj);
                case TypeCode.Double:
-                    return ((double)obj).ToString(numberFormat, CultureInfo.InvariantCulture);
+                    return ((double)obj).ToString(ProtoCore.Mirror.MirrorData.PrecisionFormat, CultureInfo.InvariantCulture);


Again I would comment out this change for the time being until https://jira.autodesk.com/browse/DYN-5101 is done. Add a TODO.

aparajit-pratap · 2023-02-16T20:59:58Z

src/DynamoCoreWpf/ViewModels/Preview/WatchViewModel.cs

            IsCollection = label == WatchViewModel.LIST || label == WatchViewModel.DICTIONARY;
        }

+        internal static string PrecisionFormat { get; set; } = "f3";


I don't think we need to add this property.

aparajit-pratap · 2023-02-16T21:00:43Z

src/DynamoCoreWpf/Views/Preview/PreviewControl.xaml.cs

                            nodeViewModel.NodeModel.OutPorts.Select(p => p.Name).Where(n => !string.IsNullOrEmpty(n));
                    }

+                    WatchViewModel.PrecisionFormat = nodeViewModel.DynamoViewModel.PreferenceSettings.NumberFormat;


Changes to this file can be reverted entirely.

aparajit-pratap · 2023-02-16T21:02:23Z

src/DynamoCoreWpf/Utilities/PreferencesPanelUtilities.cs

@@ -1,4 +1,5 @@
-using System.Linq;
+using System;
+using System.Linq;


This file can be reverted entirely.

QilongTang · 2023-02-17T14:56:46Z

Reported regression is sporadic

Improvement Number Format

01f5a8b

mjkkirschner reviewed Feb 11, 2023

View reviewed changes

src/DynamoCoreWpf/Views/Preview/PreviewControl.xaml.cs Outdated Show resolved Hide resolved

updating scope

9a48930

QilongTang reviewed Feb 13, 2023

View reviewed changes

src/DynamoCoreWpf/Views/Preview/PreviewControl.xaml.cs Outdated Show resolved Hide resolved

QilongTang reviewed Feb 13, 2023

View reviewed changes

QilongTang reviewed Feb 14, 2023

View reviewed changes

jesusalvino added 2 commits February 14, 2023 15:00

minor refactoring

e0a46a8

Number Format to Double and String values based on the preference

584cbcb

aparajit-pratap reviewed Feb 16, 2023

View reviewed changes

comment changes and refactoring

29881fa

aparajit-pratap approved these changes Feb 17, 2023

View reviewed changes

QilongTang approved these changes Feb 17, 2023

View reviewed changes

QilongTang added this to the 2.18.0 milestone Feb 17, 2023

QilongTang merged commit e1649a0 into DynamoDS:master Feb 17, 2023

QilongTang mentioned this pull request Feb 17, 2023

[DNM] Align node previews with number format selected in preferences #13076

Open

8 tasks

mjkkirschner mentioned this pull request Sep 1, 2023

DYN-5965: introduce new string from nodes that accept numeric format specifiers #14369

Merged

20 tasks

Conversation

jesusalvino commented Feb 11, 2023

Purpose

Declarations

Reviewers

FYIs

Uh oh!

jesusalvino commented Feb 11, 2023

Uh oh!

Uh oh!

Uh oh!

QilongTang Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesusalvino Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

QilongTang Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

QilongTang left a comment

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jesusalvino commented Feb 16, 2023

Uh oh!

aparajit-pratap Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

QilongTang commented Feb 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

QilongTang Feb 13, 2023 •

edited

Loading

aparajit-pratap commented Feb 14, 2023 •

edited

Loading

aparajit-pratap Feb 16, 2023 •

edited

Loading

aparajit-pratap Feb 16, 2023 •

edited

Loading