Bugzilla – Full Text Bug Listing
| Summary: | [api] support deletion of multiple interaction elements | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Shawn Minto <shawn.minto> | ||||||||
| Component: | Mylyn | Assignee: | Shawn Minto <shawn.minto> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | enhancement | ||||||||||
| Priority: | P3 | CC: | steffen.pingel | ||||||||
| Version: | unspecified | ||||||||||
| Target Milestone: | 3.2 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Shawn Minto
Created attachment 134337 [details]
patch
Here is a patch that takes in a list of elements for deletion instead of deleting one-by-one. This improves the performance since it only triggers 1 refresh (of many objects) instead of many different refreshes. Steffen, let me know what you think.
Created attachment 134338 [details]
mylyn/context/zip
Created attachment 134340 [details]
additional improvments
This patch tries to be smart about the deletion events that are received to determine if it is cheaper to refresh all or just refresh the changes. This patch doesn't increase the performance as much as the first patch and it may not be worth applying. The only part that may be worth it is the TypeHistoryManager.
First patch looks good to me, minor nits: - rename taskscape to context :) - InteractionContextManager.deleteElements() should thrown an exception in case of a null elements and only check once if elements.size() ==0 Let's look over the second patch together. I can't say how much this will improve performance. It would be really great if we could come up with performance tests to verify this. (In reply to comment #4) > First patch looks good to me, minor nits: > - rename taskscape to context :) > - InteractionContextManager.deleteElements() should thrown an exception in case > of a null elements and only check once if elements.size() ==0 I will fix these nits and commit the patch. I forgot that I can change names now without patch size problems ;) > Let's look over the second patch together. I can't say how much this will > improve performance. It would be really great if we could come up with > performance tests to verify this. I agree that we need to review this and I don't know if there is enough benefit to maintain these cases or not since the real problem is related to refresh of the viewers and it would be better not to mask that problem until we determine how it can be fixed. I have applied the first patch after fixing the nits that you pointed out. We now throw an IllegalArgumentException if a null list of elements is passed in. When should we assert vs throw an exception? Lets decide if we want the second patch before we close this bug. Yes, please always use Assert from o.e.core.runtime. Just noticed that the patch removes //$NON-NLS-1$ comments. These should be left in the code. I updated to use Assert instead. As for the //$NON-NLS-1$ comments, I didn't remove any and I don't see any warnings for the classes that I patched. Does the patch wizard ignore these for some reason? I see what you mean, the second patch seems to have them removed. Interesting as I remember them being there in my local workspace. I will make sure to watch out for this. Thanks Shawn. I have moved the second patch to bug 275512. We can discuss addtional performance improvements on that bug. |