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

Jump to bottom

Fixing bugs 1260528 and 1260525 #366

Merged
merged 8 commits into from
May 10, 2021
Merged

Fixing bugs 1260528 and 1260525 #366

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f4984a1
fixing bug 1260528
May 3, 2021
f05b1d9
fixing bug 1260525
May 3, 2021
bbf0b7a
fixing bug where the total expense cost doesn't update when changing …
May 3, 2021
539e12c
adding requested changes
May 6, 2021
065cfa0
removing ensure code and overriding InsertItem method
May 7, 2021
0247015
Merge branch 'master' into fixing-expenseit-demo
guimafelipe May 7, 2021
72d7ecd
reverting last commit
May 7, 2021
bd2c8b0
changed to use OnListItemsChanged event
May 10, 2021
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
36 changes: 29 additions & 7 deletions Sample Applications/ExpenseIt/ExpenseItDemo/CreateExpenseReportDialogBox.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ private void addExpenseButton_Click(object sender, RoutedEventArgs e)
var app = Application.Current;
var expenseReport = (ExpenseReport) app.FindResource("ExpenseData");
expenseReport?.LineItems.Add(new LineItem());

DataGridRow row =null;

// Dispatching this at loaded priority so the new row has been added before our code runs
Expand Down Expand Up @@ -82,15 +82,37 @@ private void viewChartButton_Click(object sender, RoutedEventArgs e)
dlg.Show();
}

bool isValid(DependencyObject parent)
{
if (System.Windows.Controls.Validation.GetHasError(parent)) return false;

for(int i = 0; i != VisualTreeHelper.GetChildrenCount(parent); i++)
{
DependencyObject child = VisualTreeHelper.GetChild(parent, i);
if (!isValid(child)) return false;
}

return true;
}
Comment on lines +85 to +96
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need all that? I think DataGrid won't leave edit-mode if there are validation errors, so all you have to do is check whether the DataGrid is editing. (Caution: I remember arguing about exactly this issue - knowing whether all validation errors have been cleared before proceeding with a higher-level commit like 'save' or 'close dialog' - but I don't remember how it turned out.)


private void okButton_Click(object sender, RoutedEventArgs e)
{
MessageBox.Show(
"Expense Report Created!",
"ExpenseIt Standalone",
MessageBoxButton.OK,
MessageBoxImage.Information);
if(!isValid(expenseDataGrid1)){
MessageBox.Show(
"Please, fix the errors.",
"Error",
MessageBoxButton.OK,
MessageBoxImage.Error);
} else
{
MessageBox.Show(
Copy link
Contributor

Choose a reason for hiding this comment

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

Another horrible UI design - message boxes should be a last resort only for very serious problems. But not worth changing here.

"Expense Report Created!",
"ExpenseIt Standalone",
MessageBoxButton.OK,
MessageBoxImage.Information);

DialogResult = true;
DialogResult = true;
}
}

private void cancelButton_Click(object sender, RoutedEventArgs e)
Expand Down
9 changes: 9 additions & 0 deletions Sample Applications/ExpenseIt/ExpenseItDemo/LineItemCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@ public class LineItemCollection : ObservableCollection<LineItem>
{
public event EventHandler LineItemCostChanged;

public new void InsertItem(int index, LineItem item)
{
if (item != null)
{
item.PropertyChanged += LineItemPropertyChanged;
}
base.InsertItem(index, item);
}

public new void Add(LineItem item)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this method - otherwise you'll add the PropertyChanged handler twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to revert the last modification and remain with the EnsureInitialize solution because the behaviour was not correct at all. With this version, athough we use more lines of code, the result seems to be the safest and it solves the bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

What was wrong? Moving the "add PropertyChanged listener" logic from Add to InsertItem should work, and eliminate the need for both EnsureInitialized and the "new Add" method. That uses fewer lines, but more importantly it demonstrates the right way to do the job. (The presence of the 'new' keyword is often a sign that you're doing something fishy.)

All the IList and IList "add" methods - Add, Insert - end up calling InsertItem. Unless I'm missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I understand. I will try to change this one again to make it in the correct way.

Copy link
Contributor Author

@guimafelipe guimafelipe May 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What was wrong? Moving the "add PropertyChanged listener" logic from Add to InsertItem should work, and eliminate the need for both EnsureInitialized and the "new Add" method. That uses fewer lines, but more importantly it demonstrates the right way to do the job. (The presence of the 'new' keyword is often a sign that you're doing something fishy.)

All the IList and IList "add" methods - Add, Insert - end up calling InsertItem. Unless I'm missing something?

I was mistaken. The the "add PropertyChanged listener" in the InsertItem method didn't work. It seems that the list still doesn't subscribe to the events of the ones in the markup.

{
if (item != null)
Expand Down
1 change: 1 addition & 0 deletions Sample Applications/ExpenseIt/ExpenseItDemo/Styles.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,7 @@
<Setter Property="FontFamily" Value="Ariel" />
<Setter Property="TextAlignment" Value="Center" />
<Setter Property="TextTrimming" Value="WordEllipsis" />
<Setter Property="TextWrapping" Value="WrapWithOverflow" />
<Setter Property="Width" Value="100" />
<Style.Triggers>
<DataTrigger Binding="{Binding Path=(SystemParameters.HighContrast)}" Value="true">
Expand Down
1 change: 1 addition & 0 deletions Sample Applications/ExpenseIt/ExpenseItDemo/ViewChartWindow.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
xmlns:local="clr-namespace:ExpenseItDemo"
DataContext="{StaticResource ExpenseData}"
mc:Ignorable="d"
Title="ViewChartWindow" Height="300" Width="300">
<Grid Style="{StaticResource WindowContentGrid}">
Expand Down