Fixing bugs 1260528 and 1260525 #366
Conversation
| var app = Application.Current; | ||
| var expenseReport = (ExpenseReport) app.FindResource("ExpenseData"); | ||
|
|
||
| if (expenseReport != null && !expenseReport.Initialized) | ||
| { | ||
| expenseReport.InitializeItems(); | ||
| } |
There was a problem hiding this comment.
This app does a lot of things a real app wouldn't do:
- operate on a singleton ExpenseReport
- pre-load that report with real LineItems
- do the pre-loading in markup
This brought on the "change an original item" bug, because markup calls IList.Add and thus avoids the override of IList.Add that adds the PropertyChanged listener. I hate to see so much code devoted to a problem that wouldn't arise in a real app. But it should be solved, so that people following the sample aren't led astray.
Let's make the solution smaller:
- Don't expose an Initialized property
- Rename InitializeItems to EnsureInitialized, and have it check _initialized to do the work only once
- Replace 25-29 by expenseReport?.EnsureInitialized();
- Add a comment in EnsureInitialized explaining why it's needed, and why a real app wouldn't need it.
There was a problem hiding this comment.
On further thought, I think the app simply overrides the wrong method. Instead of overriding Add, try overriding InsertItem. This should be called even in the markup case, so you can get rid of all the Initialize code you added.
There was a problem hiding this comment.
It works! I overrided the InsertItem method and erased the EnsureInserted code from before.
| 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; | ||
| } |
There was a problem hiding this comment.
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.)
| MessageBoxImage.Error); | ||
| } else | ||
| { | ||
| MessageBox.Show( |
There was a problem hiding this comment.
Another horrible UI design - message boxes should be a last resort only for very serious problems. But not worth changing here.
| } | ||
| base.InsertItem(index, item); | ||
| } | ||
|
|
||
| public new void Add(LineItem item) |
There was a problem hiding this comment.
Remove this method - otherwise you'll add the PropertyChanged handler twice.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Okay, I understand. I will try to change this one again to make it in the correct way.
There was a problem hiding this comment.
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.
| private void OnListItemsChanged(object sender, NotifyCollectionChangedEventArgs e) | ||
| { | ||
| if (e.PropertyName == "Cost") | ||
| if(e.Action == NotifyCollectionChangedAction.Add) | ||
| { | ||
| OnLineItemCostChanged(this, new EventArgs()); | ||
| for(int i = 0; i < e.NewItems.Count; i++) | ||
| { | ||
| LineItem item = e.NewItems[i] as LineItem; | ||
| item.PropertyChanged += LineItemPropertyChanged; | ||
| } |
There was a problem hiding this comment.
Changed to use the Add event of the list. This version is working well.
Fixes issues #362 and #361.
Description
This fix changes two bugs related above. The first is the expense report being generated successful even when there are errors in the table. A method was addet to check if there exists any error in the expenses table and show an error message box if there is one. The second one is that the graph char window doesn't show the expense report total value and the text was being clipped. To fix the text, it was changed to wrap, and to fix the value not showing, the ExpenseData declared as the DataContext of the page.
Actually, there was another bug in the app. When changing the value of the items that are in the table since beggining, the total expense was not changing. This is because the parent list didn't subscribe to their property change events. So now there is an InitializeItms function that is called once and do these subscriptions.
Customer Impact
When using the app, the customer will now see that the table has an error when they try to generete the expense report. And now the total expense value is being updated correctly everywhere.
Regression
No. This sample is used for Accessibility testing and never fully supported Narrator.
Testing
The sample project was tested by its usage.