Opened 14 years ago
Closed 14 years ago
#1324 closed enhancement (done)
Improve performance of item collection views
Reported by: | abeham | Owned by: | swagner |
---|---|---|---|
Priority: | high | Milestone: | HeuristicLab 3.3.3 |
Component: | Core.Views | Version: | 3.3.3 |
Keywords: | Cc: |
Description (last modified by swagner)
The ItemCollectionView and others are quite slow when the collection is cleared (the index references to the SmallImageList are updated for every removed item).
Change History (34)
comment:1 Changed 14 years ago by abeham
- Status changed from new to accepted
comment:2 Changed 14 years ago by abeham
- Owner changed from abeham to mkommend
- Status changed from accepted to reviewing
comment:3 Changed 14 years ago by mkommend
- Owner changed from mkommend to abeham
- Status changed from reviewing to assigned
review comments:
- All events must be deregistered in the DeregisterContentEvents method. Escpecially ItemImageChanged and ToStringChanged are not deregistered and hence there are reference to the view as long as the content exists, regardless if the view is opened, closed or disposed.
- The collection item events should only be registered once.
comment:4 Changed 14 years ago by abeham
- Status changed from assigned to accepted
Thanks! It doesn't seem to be common knowledge that Content is not set to null when a view is closed. The old code probably expected that. I'll fix this for the other views also.
- Fixed event (de)registration in ItemCollectionView
comment:5 Changed 14 years ago by abeham
- Owner changed from abeham to mkommend
- Status changed from accepted to reviewing
- Fixed event deregistration in ItemArrayView and ItemListView
comment:6 Changed 14 years ago by mkommend
- Owner changed from mkommend to swagner
comment:7 Changed 14 years ago by swagner
- Status changed from reviewing to assigned
comment:8 Changed 14 years ago by swagner
- Status changed from assigned to accepted
comment:9 Changed 14 years ago by swagner
Deregistration of the ItemImageChanged and ToStringChanged events of each item was already implemented in the control's Dispose method which is located in the designer file and therefore easily overlooked. For sure the better solution is to implement event deregistration in DeregisterContentEvents. I will removed the deregistration code in the Dispose methods.
comment:10 Changed 14 years ago by swagner
- Description modified (diff)
- Summary changed from Improve performance of ItemCollectionView to Improve performance of item collection views
comment:11 Changed 14 years ago by abeham
We both overlooked that. Yes, it seems better to do as much as possible in the user file.
comment:12 Changed 14 years ago by swagner
Changes in r5237:
- Added performance optimization in all other item collection views
- Checked and refactored item event registration/deregistration
- Enabled null items in item collection views
- Moved non-default Dispose methods from designer files into user files
comment:13 Changed 14 years ago by swagner
- Owner changed from swagner to abeham
- Status changed from accepted to reviewing
Please review changes of r5237 and then forward to mkommend for review.
comment:14 Changed 14 years ago by swagner
- Owner changed from abeham to swagner
- Status changed from reviewing to assigned
comment:15 Changed 14 years ago by swagner
- Status changed from assigned to accepted
comment:16 Changed 14 years ago by swagner
Adapted method GetListViewItemsForItem of ItemListView and ItemArrayView to work with null items in r5238.
comment:17 Changed 14 years ago by swagner
Adapted method RebuildImageList to work with null items in r5239.
comment:18 Changed 14 years ago by swagner
- Owner changed from swagner to abeham
- Status changed from accepted to reviewing
comment:19 Changed 14 years ago by abeham
- Owner changed from abeham to mkommend
Thanks for the changes, I reviewed them and have some minor comments
- ItemArrayView
- Since (Add|Insert|Remove|Update)ListViewItem and UpdateListViewItem(Image|Text) are protected they should throw an ArgumentNullException when listViewItem is null
- This holds for the other views as well
- ItemSetView
- When moving an item in the ItemSetView the item is deleted from the set
I'll forward it to mkommend
comment:20 Changed 14 years ago by swagner
When item updates are still pending after the content of an ItemCollectionView has already been changed, the item is no longer held in the itemListViewItemMapping dictionary and consequently the update fails with a KeyNotFoundException. Fixed this in r5302.
comment:21 Changed 14 years ago by mkommend
- Status changed from reviewing to assigned
comment:22 Changed 14 years ago by mkommend
- Status changed from assigned to accepted
comment:23 Changed 14 years ago by mkommend
Added argument null checks in (Add|Insert|Remove|Update)ListViewItem and UpdateListViewItem(Image|Text) methods with r5371 as abeham suggested.
comment:24 follow-up: ↓ 28 Changed 14 years ago by mkommend
- Owner changed from mkommend to swagner
- Status changed from accepted to reviewing
Reviewed the changesets r5237, r5238 and r5239.
I do not fully understand from abeham regarding moving of items in an ItemSetView and what the problem is if the item is deleted.
One thing which is still pending is the unification of the code of the different collection views (lots of duplicate code exists in the different collection view classes), but this should be addressed in a separate ticket.
comment:25 Changed 14 years ago by mkommend
- Owner changed from swagner to mkommend
- Status changed from reviewing to assigned
comment:26 Changed 14 years ago by mkommend
- Status changed from assigned to accepted
comment:27 Changed 14 years ago by mkommend
Corrected ItemSetView to check for duplicates while dropping items with r5380.
comment:28 in reply to: ↑ 24 Changed 14 years ago by mkommend
Replying to mkommend:
I do not fully understand from abeham regarding moving of items in an ItemSetView and what the problem is if the item is deleted.
The problem is that the item is not readded when a move operations occurs. This can be tested in the OperatorGraphView which contains a OperatorSetView that is the only ItemSetView in the HL trunk.
comment:29 Changed 14 years ago by mkommend
- Owner changed from mkommend to abeham
- Status changed from accepted to reviewing
comment:30 Changed 14 years ago by abeham
- Status changed from reviewing to readytorelease
Change looks good. I guess it's ready to release now
comment:31 Changed 14 years ago by swagner
- Owner changed from abeham to swagner
- Status changed from readytorelease to reviewing
comment:32 Changed 14 years ago by swagner
Created follow-up ticket #1407 for unifying the code of the different collection views.
comment:33 Changed 14 years ago by swagner
- Status changed from reviewing to readytorelease
All additional changes look good. Thanks.
comment:34 Changed 14 years ago by mkommend
- Resolution set to done
- Status changed from readytorelease to closed
- Version changed from 3.3.2 to 3.3.3
r5057