Fix autocomplete popup - #3 by aparajit-pratap · Pull Request #11224 · DynamoDS/Dynamo

aparajit-pratap · 2020-11-02T04:49:43Z

Purpose

Attempt at fixing autocomplete popup placement one final time. Based on discussion with @zeusongit we decided that the CustomPopupPlacementCallback approach seemed most promising that would work on most machines. This also fixes an existing issue where the autocomplete popup doesn't show up on opening a new workspace.

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

src/DynamoCoreWpf/Controls/NodeAutoCompleteSearchControl.xaml.cs

aparajit-pratap · 2020-11-02T16:40:11Z

I will also take a look at the test failure.

QilongTang · 2020-11-02T21:40:58Z

src/DynamoCoreWpf/Views/Core/WorkspaceView.xaml.cs

+                        ViewModel.NodeAutoCompleteSearchViewModel.PortViewModel.SetupNodeAutocompleteWindowPlacement(popup);
+                    }
+                    popup.IsOpen = displayPopup;
+                    popup.CustomPopupPlacementCallback = null;


Is this a must have or just sanity cleanup?

Yes, otherwise the CustomPopupPlacementCallback continues pointing to PlaceAutocompletePopup, which can be invoked unexpectedly, say when in-canvas search popup is opened.

QilongTang · 2020-11-02T21:48:25Z

Changes here looks good to me, but we should fix the analytics somehow and make sure we unify the implementation between incanvas search and node autocomplete. @aparajit-pratap @zeusongit

aparajit-pratap · 2020-11-02T22:28:49Z

Can we leave this on the control level? I think it may live better there which in canvas control may just follow same example.

Where exactly would you suggest moving this to?

QilongTang · 2020-11-02T22:32:09Z

Can we leave this on the control level? I think it may live better there which in canvas control may just follow same example.

Where exactly would you suggest moving this to?

https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/Controls/NodeAutoCompleteSearchControl.xaml.cs#L136

* fix autocomplete popup - part 3 * revert unchanged file * fix failing test, fix analytics for node autocomplete * add null check * review comments

QilongTang · 2020-11-03T03:21:14Z

@aparajit-pratap Didn't you say the loaded function is not always called? Then why bring that implementation back?

aparajit-pratap · 2020-11-03T03:25:06Z

@aparajit-pratap Didn't you say the loaded function is not always called? Then why bring that implementation back?

I'm not sure - it seems like it's now being invoked because the popup does appear every time after moving the placement from the loaded function to explicitly calling it here. If however, I move the call to place the popup back into the loaded function, it will not be called every time.

QilongTang · 2020-11-03T03:31:55Z

@aparajit-pratap Regardless of that, the current implementation for analytics looks wrong in this PR because the if check below can't be removed for the analytics call, otherwise initialization of the control will also trigger analytics which gives us an extra use of node autocomplete everytime user launches Dynamo. Please add it back and to the cherry-pick PR. Thanks

if (ViewModel != null && ViewModel.PortViewModel != null)

aparajit-pratap · 2020-11-03T03:34:13Z

@QilongTang the above condition will always be true in the new case.

QilongTang · 2020-11-03T03:37:47Z

@QilongTang the above condition will always be true in the new case.

Must be I was debugging the old version, but anyway this brings a bug that extra use is reported from analytics..

aparajit-pratap · 2020-11-03T03:48:50Z

My debugging indicates that the loaded callback is being called just once for every port click and at all times both ViewModel and ViewModel.PortViewModel are non-null so if you're still seeing extra uses of analytics being reported, then even if I add the null-check, it might not fix the extra reporting. This could mean that I need to try adding the analytics tracking at some other place.
I think you're right. There is an extra call to this method; at least one when Dynamo is launched. I will add back the checks.

QilongTang · 2020-11-03T03:55:55Z

My debugging indicates that the loaded callback is being called just once for every port click and at all times both ViewModel and ViewModel.PortViewModel are non-null so if you're still seeing extra uses of analytics being reported, then even if I add the null-check, it might not fix the extra reporting. This could mean that I need to try adding the analytics tracking at some other place.

The loaded function is always called for me when Dynamo is started, see below gif

I think that is why I was say earlier that let's move the tracking to https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/Controls/NodeAutoCompleteSearchControl.xaml.cs#L136 because visibility change is a safer place IMHO. Let me know, if you are not comfortable doing it, I can make a PR because the change need to be done for in canvas search as well.

aparajit-pratap · 2020-11-03T03:58:45Z

@QilongTang I'm not confident of how many times and when OnNodeAutoCompleteSearchControlVisibilityChanged is invoked. As a correction to my above statement, I'm adding back the null checks to the loaded method. Please let me know if that is acceptable.

QilongTang · 2020-11-03T04:00:49Z

@QilongTang I'm not confident of how many times and when OnNodeAutoCompleteSearchControlVisibilityChanged is invoked. As a correction to my above statement, I'm adding back the null checks to the loaded method. Please let me know if that is acceptable.

From my debugging, that is acceptable

mjkkirschner · 2020-11-03T15:39:10Z

src/DynamoCoreWpf/ViewModels/Core/NodeViewModel.cs

            }
        }

+        internal double ActualHeight { get; set; }


Just curious, can you explain why these were needed?

These were needed to get the exact height and width from the node view and exposed to the portviewmodel via the nodeviewmodel. The width and height are needed to compute the popup placement positioning.

* Fix autocomplete popup - #3 (#11224) * fix autocomplete popup - part 3 * revert unchanged file * fix failing test, fix analytics for node autocomplete * add null check * review comments * add null checks

fix autocomplete popup - part 3

edd7bac

aparajit-pratap requested review from QilongTang and zeusongit November 2, 2020 04:49

revert unchanged file

80e8bcd

aparajit-pratap changed the title ~~Fix autocomplete popup - part 3~~ Fix autocomplete popup - #3 Nov 2, 2020

resolve merge conflicts with master

7e61f8e

QilongTang reviewed Nov 2, 2020

View reviewed changes

src/DynamoCoreWpf/Controls/NodeAutoCompleteSearchControl.xaml.cs Show resolved Hide resolved

zeusongit approved these changes Nov 2, 2020

View reviewed changes

QilongTang reviewed Nov 2, 2020

View reviewed changes

QilongTang approved these changes Nov 2, 2020

View reviewed changes

aparajit-pratap added 2 commits November 2, 2020 17:18

fix failing test, fix analytics for node autocomplete

257e9d7

add null check

6250818

review comments

b8afb43

aparajit-pratap merged commit 905d976 into DynamoDS:master Nov 3, 2020

aparajit-pratap deleted the fixAutocompletePopup branch November 3, 2020 02:53

mjkkirschner reviewed Nov 3, 2020

View reviewed changes

Conversation

aparajit-pratap commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Uh oh!

Uh oh!

aparajit-pratap commented Nov 2, 2020

Uh oh!

QilongTang Nov 2, 2020

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Nov 2, 2020

Choose a reason for hiding this comment

Uh oh!

QilongTang commented Nov 2, 2020

Uh oh!

aparajit-pratap commented Nov 2, 2020

Uh oh!

QilongTang commented Nov 2, 2020

Uh oh!

QilongTang commented Nov 3, 2020

Uh oh!

aparajit-pratap commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QilongTang commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aparajit-pratap commented Nov 3, 2020

Uh oh!

QilongTang commented Nov 3, 2020

Uh oh!

aparajit-pratap commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QilongTang commented Nov 3, 2020

Uh oh!

aparajit-pratap commented Nov 3, 2020

Uh oh!

QilongTang commented Nov 3, 2020

Uh oh!

mjkkirschner Nov 3, 2020

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Nov 3, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aparajit-pratap commented Nov 2, 2020 •

edited

Loading

aparajit-pratap commented Nov 3, 2020 •

edited

Loading

QilongTang commented Nov 3, 2020 •

edited

Loading

aparajit-pratap commented Nov 3, 2020 •

edited

Loading