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.

Sign up for GitHub

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

DYN-6377: Restrict where OnNodeModified is called. #14656

Merged
merged 4 commits into from
Dec 4, 2023
Merged

DYN-6377: Restrict where OnNodeModified is called. #14656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2a3db5f
Restrict where OnNodeModified is called.
twastvedt Nov 29, 2023
9d828da
Clean up.
twastvedt Nov 29, 2023
e958170
Dispose
twastvedt Nov 29, 2023
bc7a742
No SuppressFinalize
twastvedt Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/Libraries/CoreNodeModels/DropDown.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public ObservableCollection<DynamoDropDownItem> Items
set
{
items = value;
RaisePropertyChanged("Items");
RaisePropertyChanged(nameof(Items));
}
}

Expand Down Expand Up @@ -106,7 +106,6 @@ public int SelectedIndex
selectedString = GetSelectedStringFromItem(Items.ElementAt(value));
}

OnNodeModified();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it fair to say that an api dev should not call this function and use updatemodelvaluecommand instead ? otherwise the node will not be marked as modified. Perhaps make the set internal at some point in the future ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we need to be able to set SelectedIndex from the Revit dropdown nodes. I guess it could be protected. But maybe there are other situations where you want to be able to set the value without triggering updates?

RaisePropertyChanged("SelectedIndex");
RaisePropertyChanged("SelectedString");
}
Expand Down Expand Up @@ -137,12 +136,12 @@ public string SelectedString
if (item != null)
{
selectedIndex = Items.IndexOf(item);
RaisePropertyChanged("SelectedIndex");
RaisePropertyChanged(nameof(SelectedIndex));
}
}

selectedString = value;
RaisePropertyChanged("SelectedString");
RaisePropertyChanged(nameof(SelectedString));
}
}

Expand Down Expand Up @@ -203,6 +202,8 @@ protected override bool UpdateValueCore(UpdateValueParams updateValueParams)
Warning(Dynamo.Properties.Resources.NothingIsSelectedWarning);
}

OnNodeModified();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this function called when SelectedIndex property is set to a different value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it's not

Copy link
Contributor

@aparajit-pratap aparajit-pratap Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I didn't get it then, how does moving OnNodeModified here work in that case?

Copy link
Contributor

@aparajit-pratap aparajit-pratap Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

When exactly is UpdateValueCore called in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see, by moving it out of SelectedIndex, you are avoiding chances of an infinite loop, since SelectedIndex is changed in PopulateItems, which is incidentally called here: https://github.com/DynamoDS/DynamoRevit/blob/a1c5384651133ebab113dd0abfef5bd256ef51f0/src/Libraries/RevitNodesUI/GenericClasses.cs#L194 inside the BuildOutputAst function, am I right?
I think I would agree that calling PopulateItems in BuildOutputAst is a bad idea in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.
Since it's been that way for a while (7 years!), perhaps we could fix it back to working as it did before #14122 and look at improving in a separate task?


return true;
}

Expand Down
54 changes: 31 additions & 23 deletions src/Libraries/CoreNodeModelsWpf/NodeViewCustomizations/DSDropDownBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,61 +15,69 @@ namespace CoreNodeModelsWpf.Nodes
public class DropDownNodeViewCustomization : INodeViewCustomization<DSDropDownBase>
{
private DSDropDownBase model;
private ComboBox comboBox;

public void CustomizeView(DSDropDownBase model, NodeView nodeView)
{
this.model = model;

//add a drop down list to the window
var combo = new ComboBox
// Add a drop down list to the window
comboBox = new ComboBox
{
Width = System.Double.NaN,
MinWidth= 150,
Width = double.NaN,
MinWidth = 150,
Height = Configurations.PortHeightInPixels,
HorizontalAlignment = HorizontalAlignment.Stretch,
VerticalAlignment = VerticalAlignment.Center
VerticalAlignment = VerticalAlignment.Center,
Style = (Style)SharedDictionaryManager.DynamoModernDictionary["RefreshComboBox"]
};

nodeView.inputGrid.Children.Add(comboBox);
Grid.SetColumn(comboBox, 0);
Grid.SetRow(comboBox, 0);

combo.Style = (Style)SharedDictionaryManager.DynamoModernDictionary["RefreshComboBox"];
comboBox.DropDownOpened += DropDownOpened;
comboBox.SelectionChanged += SelectionChanged;

nodeView.inputGrid.Children.Add(combo);
System.Windows.Controls.Grid.SetColumn(combo, 0);
System.Windows.Controls.Grid.SetRow(combo, 0);
comboBox.DataContext = model;

combo.DropDownOpened += combo_DropDownOpened;

combo.DataContext = model;

// bind this combo box to the selected item hash
var bindingVal = new System.Windows.Data.Binding("Items")
// Bind this combo box to the selected item hash.
var bindingVal = new Binding(nameof(DSDropDownBase.Items))
{
Mode = BindingMode.TwoWay,
Source = model
};
combo.SetBinding(ItemsControl.ItemsSourceProperty, bindingVal);
comboBox.SetBinding(ItemsControl.ItemsSourceProperty, bindingVal);

// bind the selected index to the model property SelectedIndex
var indexBinding = new Binding("SelectedIndex")
// Bind the selected index to the model property SelectedIndex.
var indexBinding = new Binding(nameof(DSDropDownBase.SelectedIndex))
{
Mode = BindingMode.TwoWay,
Source = model
};
combo.SetBinding(Selector.SelectedIndexProperty, indexBinding);
comboBox.SetBinding(Selector.SelectedIndexProperty, indexBinding);
}

private void SelectionChanged(object sender, SelectionChangedEventArgs e)
{
if (comboBox.SelectedIndex != -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't -1 actually a valid value ? Unless it is not allowed to clear the current selection here or in a subclass.

Copy link
Member

Choose a reason for hiding this comment

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

if you want to avoid introducing the field, I think the sender will be the combobox.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I need the field anyways for Dispose though.

{
model.OnNodeModified();
}
}

public void Dispose()
{
comboBox.DropDownOpened -= DropDownOpened;
comboBox.SelectionChanged -= SelectionChanged;
}

/// <summary>
/// When the dropdown is opened, the node's implementation of PopulateItems is called
/// </summary>
/// <param name="sender"></param>
/// <param name="e"></param>
void combo_DropDownOpened(object sender, EventArgs e)
void DropDownOpened(object sender, EventArgs e)
{
this.model.PopulateItems();
model.PopulateItems();
}

}
Expand Down
Loading