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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aparajit-pratap
Copy link
Contributor

Purpose

See slack thread.

image

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

Release Notes

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

FYIs

@Amoursol

@@ -208,6 +208,8 @@ internal static object GetData(StackValue sv, RuntimeCore runtimeCore)
return null;
}

internal static string PrecisionFormat { get; set; } = "f3";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as default value in preferencesettings.numberformat.

@@ -1574,7 +1574,7 @@ private IPreferences CreateOrLoadPreferences(IPreferences preferences)

private static void InitializePreferences(IPreferences preferences)
{
DynamoUnits.Display.PrecisionFormat = preferences.NumberFormat;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to DynamoUnits.Display.PrecisionFormat?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still there in the next line.

@mjkkirschner
Copy link
Member

It looks good @aparajit-pratap - maybe we can add a simple test though using one of the customer graphs that prompted this change?

@aparajit-pratap
Copy link
Contributor Author

aparajit-pratap commented Jul 1, 2022

Let's wait for a decision from @Amoursol, @Jingyi-Wen, and the rest of the team on the next steps for this. For this PR to be given the green light some of the questions that need answering/assumptions made are:

  1. Are we okay to base the node preview format on the number format chosen in preferences? I don't recall if the number format was targeted for number nodes only or for the overall UI. I leave this decision to UX and others.
  2. If we're good with point (1) do we want to add a "default" option to the existing number format options that doesn't limit the number of decimal places? This will require more changes to this PR that are simple to add.
  3. Are we okay with the idea that the number of decimal places (rounding) is only a visual thing and that the original numbers, for example, 0.0199999999999996, will continue to be used in actual computations?

@Amoursol
Copy link
Contributor

Amoursol commented Jul 5, 2022

Let's wait for a decision from @Amoursol, @Jingyi-Wen, and the rest of the team on the next steps for this. For this PR to be given the green light some of the questions that need answering/assumptions made are:

  1. Are we okay to base the node preview format on the number format chosen in preferences? I don't recall if the number format was targeted for number nodes only or for the overall UI. I leave this decision to UX and others.
  2. If we're good with point (1) do we want to add a "default" option to the existing number format options that doesn't limit the number of decimal places? This will require more changes to this PR that are simple to add.
  3. Are we okay with the idea that the number of decimal places (rounding) is only a visual thing and that the original numbers, for example, 0.0199999999999996, will continue to be used in actual computations?

@aparajit-pratap thank you for putting this PR in! My answers as below:

  1. This is a question for @Jingyi-Wen, but it will largely depend on what those settings do today and if they can be conflated.
  2. We 💯 want to add a default option if we use this, yes.
  3. Typically, we are trying to index into clarity for all things in Dynamo, so that every single user can understand what is going on, ideally at a glance. The tricky bit with the behavior before this PR was that it was hidden in the UI from the user, and only exposed via nodes, causing confusion. We may need another Preference setting here that enables rounding under a certain threshold for users, as users coming in from the Revit world may not care about infinitesimally small numbers and prefer rounding. Would love to discuss this one with @Jingyi-Wen too.

@Jingyi-Wen
Copy link

Let's wait for a decision from @Amoursol, @Jingyi-Wen, and the rest of the team on the next steps for this. For this PR to be given the green light some of the questions that need answering/assumptions made are:

  1. Are we okay to base the node preview format on the number format chosen in preferences? I don't recall if the number format was targeted for number nodes only or for the overall UI. I leave this decision to UX and others.
  2. If we're good with point (1) do we want to add a "default" option to the existing number format options that doesn't limit the number of decimal places? This will require more changes to this PR that are simple to add.
  3. Are we okay with the idea that the number of decimal places (rounding) is only a visual thing and that the original numbers, for example, 0.0199999999999996, will continue to be used in actual computations?

@aparajit-pratap thank you for putting this PR in! My answers as below:

  1. This is a question for @Jingyi-Wen, but it will largely depend on what those settings do today and if they can be conflated.
  2. We 💯 want to add a default option if we use this, yes.
  3. Typically, we are trying to index into clarity for all things in Dynamo, so that every single user can understand what is going on, ideally at a glance. The tricky bit with the behavior before this PR was that it was hidden in the UI from the user, and only exposed via nodes, causing confusion. We may need another Preference setting here that enables rounding under a certain threshold for users, as users coming in from the Revit world may not care about infinitesimally small numbers and prefer rounding. Would love to discuss this one with @Jingyi-Wen too.

My thoughts:

  1. User would expect the number format they chose in Preference to be universal, so it would be ideal if we can align them all
  2. I like @Amoursol's idea, we can add a rounding setting

@aparajit-pratap
Copy link
Contributor Author

aparajit-pratap commented Jul 7, 2022

Regarding "We may need another Preference setting here that enables rounding under a certain threshold for users", I believe this is a bigger task that would need addressing at a core level that would affect all numeric results, certainly not in this PR. Until then I suppose we can either wait until those changes happen or first roll out this part where we are only using preferences for display purposes.

@Amoursol @Jingyi-Wen any thoughts about the ☝️ suggestion?

@Amoursol
Copy link
Contributor

Regarding "We may need another Preference setting here that enables rounding under a certain threshold for users", I believe this is a bigger task that would need addressing at a core level that would affect all numeric results, certainly not in this PR. Until then I suppose we can either wait until those changes happen or first roll out this part where we are only using preferences for display purposes.

@Amoursol @Jingyi-Wen any thoughts about the ☝️ suggestion?

Looks good to me @aparajit-pratap - We can look to further rounding improvement later.

@aparajit-pratap aparajit-pratap mentioned this pull request Feb 14, 2023
8 tasks
@QilongTang
Copy link
Contributor

Closing since this is now duplicating #13744

@QilongTang QilongTang closed this Feb 17, 2023
@aparajit-pratap aparajit-pratap added the DNM Do not merge. For PRs. label Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DNM Do not merge. For PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants