ALSO-5603 Add parameter hiding autocomplete options from the UI #13834

spodo99 · 2023-03-17T15:40:04Z

Please Note:

Before submitting the PR, please review How to Contribute to Dynamo
Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
PRs will be reviewed from oldest to newest
If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
PRs made to the DynamoRevit repo 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 a LGTM label is added to the PR.

Purpose

The new flag is added to the setting to hide new autocomplete method options, if it will be requested by the users. Per conversation at https://autodesk.slack.com/archives/C1866AFQB/p1679067914031959

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

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

QilongTang · 2023-03-17T18:08:30Z

src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs

+            /// <summary>
+            /// If true, autocomplete method options are hidden from UI 
+            /// </summary>
+            public bool HideAutocompleteMethodOptions { get; set; }


@pinzart @mjkkirschner Is this allowed to add new params in this class?

https://learn.microsoft.com/en-us/dotnet/core/compatibility/library-change-rules
looks at DISALLOWED: Adding an instance field to a struct that has no nonpublic fields

If a struct has only public fields or has no fields at all... Adding any new fields - public or nonpublic - to such a struct is a source breaking change for these callers, as the compiler will now require the additional fields to be initialized.

mjkkirschner · 2023-03-18T02:57:47Z

But doesn’t a bool have a default value ?On Mar 17, 2023, at 3:08 PM, pinzart90 ***@***.***> wrote: @pinzart90 commented on this pull request. In src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs:

@@ -655,6 +655,11 @@ public struct StartConfiguration

/// If true, Analytics and Usage options are hidden from UI /// </summary> public bool HideReportOptions { get; set; } + + /// <summary> + /// If true, autocomplete method options are hidden from UI + /// </summary> + public bool HideAutocompleteMethodOptions { get; set; } https://learn.microsoft.com/en-us/dotnet/core/compatibility/library-change-rules looks at DISALLOWED: Adding an instance field to a struct that has no nonpublic fields Adding any new fields - public or nonpublic - to such a struct is a source breaking change for these callers, as the compiler will now require the additional fields to be initialized. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

pinzart90 · 2023-03-19T01:51:29Z

But doesn’t a bool have a default value ?

It does, but that is not the issue. Try this code in c#:

using System;
				
public struct Test {
	
	public int a; // Was added in version 1.0.0
	public double b; // Was added version 1.0.0
	public bool c;// Was added in version 1.0.1 (breakin change)
}

public class Program
{
	public static void Main()
	{
		Test m;
		
		m.a = 3;
		m.b = 0.4;
		Console.WriteLine(m); // Compilation error (line 18, col 21): Use of unassigned local variable 'm'
	}
}

If you remove the breaking change, you will see that the code compiles

spodo99 · 2023-03-20T12:12:02Z

@pinzart90 Got it, sorry, in C++ it would fail only if you use initializer list...
How would be better to pass this parameter then? Using new argument to DynamoViewModel::Start() function and DynamoViewModel constructor?

pinzart90 · 2023-03-20T13:49:15Z

@pinzart90 Got it, sorry, in C++ it would fail only if you use initializer list... How would be better to pass this parameter then? Using new argument to DynamoViewModel::Start() function and DynamoViewModel constructor?

I don't know strict we've been in the past about this case. I think we've done this before without knowing it is a breaking change.
The way we've extended startup params is to add a new Interface to the Preferences class. There are quite a few now, looke at the interfaces here https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCore/Configuration/IPreferences.cs

This needs to avoid breaking compatibility

pinzart90 · 2023-03-22T01:21:48Z

src/DynamoCore/Configuration/IPreferences.cs

+    /// Temporary interface to avoid breaking changes.
+    /// TODO: Merge with StartupConfiguration for 3.0 (DYN-1699)
+    /// </summary>
+    internal interface IHideAutocompleteMethodOptions


@QilongTang if there are plans to introduce new options like this one, maybe we can put it in the same place before the next release.

@pinzart90 I don't think we do currently, we intend to use Launchdarkly to control beta features and UI aspects in general.

pinzart90 · 2023-03-22T01:23:23Z

@mjkkirschner any other ideas besides yet another interface ?

mjkkirschner · 2023-03-22T14:06:36Z

@mjkkirschner any other ideas besides yet another interface ?

no better ideas, except perhaps just modifying the concrete type.

mjkkirschner · 2023-03-22T15:50:29Z

@spodo99 there is a test failure:

Dynamo.Tests.Configuration.PreferenceSettingsTests.TestImportCopySettings

QilongTang · 2023-03-22T15:57:25Z

@mjkkirschner any other ideas besides yet another interface ?

@pinzart @mjkkirschner I think startup params should also work for them right? See https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoSandbox/DynamoCoreSetup.cs#L40, I guess when Alias launching DynamoSandbox, they can pass in param to modify certain feature sets. Essentially, this might just be similar to the existing approach anyway

mjkkirschner · 2023-03-22T16:05:09Z

Do you mean a startup configuration param? (which is a struct - and had the issue Tibi identified) ? or did you mean a new CLI argument? Or both?

I think that like Tibi said, it's an API break, but it's unclear to me who would be affected.

IMO The new preference is safer, the startup config option seems more idiomatic in our repo at the cost of integrators potentially having runtime or compile time failures. (unlikely I guess).

Perhaps we should add a constructor for these structs - @pinzart90 would that avoid this issue in the future?

spodo99 · 2023-03-22T16:30:34Z

@mjkkirschner Added missing option to the new setting file

mjkkirschner · 2023-03-29T15:03:25Z

Hi @spodo99 it looks good to me. I guess ideally it would be good to add a test that asserts the UI is actually hidden or that the preferences file actually modifies the view model ? I know UI tests are sometimes painful to write in wpf.

Perhaps because of the time constraints here you could merge this and send a followup for a UI test if it's straightforward enough to write - maybe something like this test:
https://github.com/DynamoDS/Dynamo/blob/master/test/DynamoCoreWpfTests/CoreUITests.cs#L760

mjkkirschner

the test failure is a flaky ui test.

QilongTang · 2023-03-29T16:42:07Z

As communicated internally, we will make a 2.17.2 release for this, so this needs to be cherry-picked to https://github.com/DynamoDS/Dynamo/tree/RC2.17.2_master

spodo99 · 2023-03-30T16:36:28Z

@mjkkirschner , @avidit found that I missed several option controls in the Preferences menu. I created a PR #13859 fixing this. I control visibility of the entire StackPanel containing all ML controls now. Could you please merge it with 2.17.2 as well, if it's ok?
I will create unit tests later as it was proposed...

…moDS#13834) * ALSO-5603 Add parameter hiding autocomplete options from the UI * ALSO-5603 Use IPreferences for hide autocomplete option This needs to avoid breaking compatibility * ALSO-5603 Add hide autocomplete option to the settings * ALSO-5603 Fix unit test

* ALSO-5603 Add parameter hiding autocomplete options from the UI (#13834) * ALSO-5603 Add parameter hiding autocomplete options from the UI * ALSO-5603 Use IPreferences for hide autocomplete option This needs to avoid breaking compatibility * ALSO-5603 Add hide autocomplete option to the settings * ALSO-5603 Fix unit test * ALSO-5603 Hide missing ML complete controls if hiding option is set (#13859) --------- Co-authored-by: spodo99 <[email protected]>

* ALSO-5603 Add parameter hiding autocomplete options from the UI * ALSO-5603 Use IPreferences for hide autocomplete option This needs to avoid breaking compatibility * ALSO-5603 Add hide autocomplete option to the settings * ALSO-5603 Fix unit test

ALSO-5603 Add parameter hiding autocomplete options from the UI

4def4e9

QilongTang reviewed Mar 17, 2023

View reviewed changes

spodo99 and others added 2 commits March 20, 2023 17:11

ALSO-5603 Use IPreferences for hide autocomplete option

1b6e768

This needs to avoid breaking compatibility

Merge branch 'DynamoDS:master' into spodo/ALSO-5603

26df60a

spodo99 requested review from pinzart90 and QilongTang and removed request for pinzart90 and QilongTang March 20, 2023 15:15

pinzart90 reviewed Mar 22, 2023

View reviewed changes

pinzart90 requested a review from mjkkirschner March 22, 2023 01:22

ALSO-5603 Add hide autocomplete option to the settings

3a41fb0

ALSO-5603 Fix unit test

ba2a95d

mjkkirschner approved these changes Mar 29, 2023

View reviewed changes

QilongTang added this to the 2.18.0 milestone Mar 29, 2023

QilongTang merged commit 1167f83 into DynamoDS:master Mar 29, 2023

spodo99 mentioned this pull request Mar 30, 2023

ALSO-5603 Hide missing ML complete controls if hiding option is set #13859

Merged

8 tasks

sm6srw mentioned this pull request Mar 31, 2023

Cp 13834 13859 #13862

Merged

8 tasks

spodo99 deleted the spodo/ALSO-5603 branch April 25, 2023 10:31

spodo99 mentioned this pull request Apr 25, 2023

ALSO-5603 Hide missing ML complete controls if hiding option is set #13934

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ALSO-5603 Add parameter hiding autocomplete options from the UI #13834

ALSO-5603 Add parameter hiding autocomplete options from the UI #13834

spodo99 commented Mar 17, 2023 •

edited by QilongTang

Loading

QilongTang Mar 17, 2023

pinzart90 Mar 17, 2023 •

edited

Loading

mjkkirschner commented Mar 18, 2023 via email

pinzart90 commented Mar 19, 2023 •

edited

Loading

spodo99 commented Mar 20, 2023

pinzart90 commented Mar 20, 2023 •

edited

Loading

pinzart90 Mar 22, 2023

QilongTang Mar 22, 2023

pinzart90 commented Mar 22, 2023

mjkkirschner commented Mar 22, 2023

mjkkirschner commented Mar 22, 2023

QilongTang commented Mar 22, 2023 •

edited

Loading

mjkkirschner commented Mar 22, 2023 •

edited

Loading

spodo99 commented Mar 22, 2023

mjkkirschner commented Mar 29, 2023 •

edited

Loading

mjkkirschner left a comment

QilongTang commented Mar 29, 2023 •

edited

Loading

spodo99 commented Mar 30, 2023

ALSO-5603 Add parameter hiding autocomplete options from the UI #13834

ALSO-5603 Add parameter hiding autocomplete options from the UI #13834

Conversation

spodo99 commented Mar 17, 2023 • edited by QilongTang Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

QilongTang Mar 17, 2023

Choose a reason for hiding this comment

pinzart90 Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

mjkkirschner commented Mar 18, 2023 via email

pinzart90 commented Mar 19, 2023 • edited Loading

spodo99 commented Mar 20, 2023

pinzart90 commented Mar 20, 2023 • edited Loading

pinzart90 Mar 22, 2023

Choose a reason for hiding this comment

QilongTang Mar 22, 2023

Choose a reason for hiding this comment

pinzart90 commented Mar 22, 2023

mjkkirschner commented Mar 22, 2023

mjkkirschner commented Mar 22, 2023

QilongTang commented Mar 22, 2023 • edited Loading

mjkkirschner commented Mar 22, 2023 • edited Loading

spodo99 commented Mar 22, 2023

mjkkirschner commented Mar 29, 2023 • edited Loading

mjkkirschner left a comment

Choose a reason for hiding this comment

QilongTang commented Mar 29, 2023 • edited Loading

spodo99 commented Mar 30, 2023

spodo99 commented Mar 17, 2023 •

edited by QilongTang

Loading

pinzart90 Mar 17, 2023 •

edited

Loading

pinzart90 commented Mar 19, 2023 •

edited

Loading

pinzart90 commented Mar 20, 2023 •

edited

Loading

QilongTang commented Mar 22, 2023 •

edited

Loading

mjkkirschner commented Mar 22, 2023 •

edited

Loading

mjkkirschner commented Mar 29, 2023 •

edited

Loading

QilongTang commented Mar 29, 2023 •

edited

Loading