Pm violation fix #14506
Pm violation fix #14506
Conversation
- located the issue causing a crash when input text into the search box - LuceneUtility.DisposeAll(); was called in the Close() method of the PackageManagerSearchViewModel. The Close() method was used to 'reset' values used by the PackageManagerView between sessions. Since the PackageManagerSearchViewModel itself is not distroyed, but persists between PM sessions (old design pattern, cached search results one reason to keep the VM) LuceneUtility.DisposeAll() should not have been disposed of - the 'Close()' method was removed in favor of PackageManagerViewClose() to avoid confusion - multiple binding issues resolved
- keep resolving binding issues
- hiding the base class 'Model' peropert, by using the `public new PackageManagerSearchElement Model` inside the PackageManagerSearchElementViewModel was the cause of the bug. This is a legacy code and the issue must have been present all along - renaming the Model to SearchElementModel fixes the issue - also created ViewModel properties corresponding to the Model properties for View binding
| @@ -569,7 +569,7 @@ internal static void ExecuteViewDetailsSideBar(Step stepInfo, StepUIAutomation u | |||
| if (packageManagerSearchElementViewModel == null) return; | |||
|
|
|||
| if (packageDetailsWindow == null) | |||
| packageManagerViewModel.ViewPackageDetailsCommand.Execute(packageManagerSearchElementViewModel.Model); | |||
| packageManagerViewModel.ViewPackageDetailsCommand.Execute(packageManagerSearchElementViewModel.SearchElementModel); | |||
There was a problem hiding this comment.
whats the difference between the Model and SearchElementModel - I would definitely also have thought the model was the appropriate parameter for this command?
Does the type checking or documentation for the command need to be improved?
There was a problem hiding this comment.
There is no difference between Model and SearchElementModel. The name Model was causing a AmbiguousMatchException error, as it was hiding the Model of the base class, thus causing the ambiguity. Renaming the property solved that issue.
| @@ -21,7 +21,46 @@ public class PackageManagerSearchElementViewModel : BrowserItemViewModel, IEquat | |||
| public ICommand VisitRepositoryCommand { get; set; } | |||
| public ICommand DownloadLatestToCustomPathCommand { get; set; } | |||
|
|
|||
| public new PackageManagerSearchElement Model { get; internal set; } | |||
There was a problem hiding this comment.
oh I see, you removed it and the new keyword...what was it hiding?
There was a problem hiding this comment.
Yup, exactly! It was hiding it.
| /// <summary> | ||
| /// The PackageManagerSearchElement Model this VM links to | ||
| /// </summary> | ||
| public PackageManagerSearchElement SearchElementModel { get; internal set; } |
There was a problem hiding this comment.
so this is really the model?
There was a problem hiding this comment.
This is the Model model .. yes :/ I came up with this name to avoid the Ambiguity.
| /// <summary> | ||
| /// VM IsDeprecated property | ||
| /// </summary> | ||
| public bool IsDeprecated { get; set; } |
There was a problem hiding this comment.
so why do we need all these now? Were we binding to the model directly before?
There was a problem hiding this comment.
We were binding directly to the Model. We had a discussion with @QilongTang a while back to move the binding to the VM, but left it for another day. I took the opportunity to do it with this PR.
| /// <summary> | ||
| /// Sets the VM properties to be bound to | ||
| /// </summary> | ||
| private void SetModelProperties() |
There was a problem hiding this comment.
this method name seems backwards to me, isn't this setting ViewModel properties from the model?
There was a problem hiding this comment.
SetViewModelProperties sounds more suitable, I will change that!
| /// </summary> | ||
| private void SetModelProperties() | ||
| { | ||
| IsDeprecated = this.SearchElementModel.IsDeprecated; |
There was a problem hiding this comment.
maybe these properties should just be getters to the model directly?
There was a problem hiding this comment.
Got it! Will give it a try and rework those.
There was a problem hiding this comment.
Done, @mjkkirschner , got them as getters to the model now! Thank you for the prompt.
| /// </summary> | ||
| internal void Close() | ||
| internal void PackageManagerViewClose() |
There was a problem hiding this comment.
is this a handler? Maybe it should be called OnPackageManagerViewClose?
There was a problem hiding this comment.
It is actively called from inside the View when the View is about to close. I don't know if the VM has handle on the View to subscribe to an event, if so I can flip it around, as in subscribe to the View Closing or Closed event.
There was a problem hiding this comment.
I checked, and the view model does not reference the view. Would that be an acceptable solution?
| @@ -43,24 +43,25 @@ public string Watermark | |||
| new FrameworkPropertyMetadata("0")); | |||
|
|
|||
| // The Value of the numerical up/down control. Bind to this property | |||
| public int Value | |||
| public string Value | |||
There was a problem hiding this comment.
This is a Number up/down slider custom control (wpf does not have one out of the box that does what we need. We used a library one, but discarded that to avoid reliance on that library, if you remember? that's the same control)
Having the property as an int was causing a conversion error that was difficult to navigate out of, for me at least. Having the property as a string simplifies this and serves the need, which is to display the number as a text, better.
There was a problem hiding this comment.
@dnenov when publishing a new version where is this string converted to a number? At least I am assuming that a number is needed in the request for the version information?
There was a problem hiding this comment.
The properties of the PublishPackageViewModel, and they are also strings. https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/ViewModels/PackageManager/PublishPackageViewModel.cs#L294 It gets used in the BuildPackage https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/ViewModels/PackageManager/PublishPackageViewModel.cs#L1646, so it's strings all the way really. But there is an int check built inside those properties.
- removed an old conversion that seems to no longer be necessary - added converter test for the DateToPackageLabelConverter
- reworked the view model properties
|
I think was able to address all the comments @mjkkirschner . The PR is working a bit harder than addressing the crash only, I hope that's OK. |
|
@dnenov can you please resolve the conflict here |
| { | ||
| TimedOut = false; // reset the timedout screen | ||
| InitialResultsLoaded = false; // reset the loading screen settings | ||
| RequestShowFileDialog -= OnRequestShowFileDialog; | ||
| nonHostFilter.ForEach(f => f.PropertyChanged -= filter_PropertyChanged); | ||
| LuceneUtility.DisposeAll(); |
There was a problem hiding this comment.
@QilongTang @reddyashish I think you should take a look at this. Should this be called from somewhere else, in this case @dnenov found that calling DisposeAll caused an access violation the next time you try to start the PackageManager View.
Instead of just being removed, should this be moved to shutdown/dispose of the PackageManagerViewExtension or DynamoView? IE somewhere higher up than the PackageManagerSearchView.
I also want to make sure we're not now leaking indexing data if we open and close the package manager search view over and over.
There was a problem hiding this comment.
This method was not meant to be a Dispose method for the PackageManagerSearchViewModel, and I am sorry that I might have mislead someone with the naming. The PackageManagerSearchViewModel is not being destroyed once created inside the DynamoView .. I suppose it is garbage collected once Dynamo closes? If LuceneUtility needs to be disposed of .. I don't know, I couldn't figure out what exactly it is disposing of, I only know that if it was disposed then, on subsequent PM view creation, it would crash.
There was a problem hiding this comment.
yes, I understand that this is not the dispose method, but it seems someone thought it was, I'm not sure who added this lucene dispose call, so I want to make sure that we don't need to find another place to relocate it to.
|
Closing this PR. All the original intended changes were disseminated over a number of smaller PRs. The main issue has been resolved in a separate PR (everything in the description above). |
Purpose
The main issue was resolved inside PR .This PR will be closed in favor of the following separate PRs:
SearchBoxControlThis is an urgent PR to address a crash when inputting text in the search box of the new Package Manager.
The issue was found to be the inappropriate placement of the
LuceneUtility.DisposeAll()clean-up code inside theClosemethod of thePackageManagerSearchViewModelclass. The View Model persists in between sessions, or uses of the Package Manager. Only the View is closed. The methodClosewas used to reset the state of the View (cleaning loading progress, search bar text, etc). When using the LuceneUtility.DisposeAll() without disposing of the View Model, we would create the conditions for the crash on subsequent restarts of the View.LuceneUtility.DisposeAll()was removedClosewas renamed toPackageManagerViewCloseto avoid further confusionOther binding issues:
public new PackageManagerSearchElement **Model**inside the PackageManagerSearchElementViewModel was the cause of the bug. Model was hiding the base classModelproperty, during reflection WPF was finding both and reporting AmbiguousMatchException.No issues when interacting with the new PM
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
public new PackageManagerSearchElement Modelinside the PackageManagerSearchElementViewModel was the cause of the bug. This is a legacy code and the issue must have been present all alongReviewers
@mjkkirschner
@QilongTang
FYIs
@Amoursol