Bugzilla – Full Text Bug Listing
| Summary: | [flex-hierarchy] NPE when column presentation changes its supported list of column IDs | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Winnie Lai <wlai> | ||||||||
| Component: | Debug | Assignee: | Platform-Debug-Inbox <platform-debug-inbox> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | cdtdoug, daniel_megert, Michael_Rennie, pawel.1.piech | ||||||||
| Version: | 3.7.1 | Flags: | Michael_Rennie:
review+
|
||||||||
| Target Milestone: | 3.8 M4 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Winnie Lai
Created attachment 205848 [details]
Patch for 3.7.x (R3_7_maintenance branch)
Created attachment 205862 [details]
Patch for master branch
I submit two patches here, one for 3.7x and the other is for master. Please review and commit them to the streams. Thanks. Created attachment 205871 [details]
Patch with alternative fix (against master).
I thought that the supplied patch was a little bit risky because effectively setVisibleColumns() would be called recursively. With a badly behaved column presentation this could lead to a stack overflow.
I came up with the patch attached. Winnie, please let me know if it still addresses the NPE.
(In reply to comment #4) Yes, your patch addresses the NPE. Please also merge your fix for 3.7.2. Thanks. I committed the fix with the alternative patch. Mike please take a look at the change. -1 for this fix. The code in InternalTreeModelViewer line 1427: if (header == null) header = id; could end up setting the name (visible to the user) of the column to be the internal id for the column (i.e. org.eclipse.myproject.column1) It would be better if we either ignored the bogus column completely (more work required to fix) or any exception in presenting columns simply causes a reset to the initial columns from the presentation. I am leaning more towards opinion 2, that way if the currently cached column infos have any other inconsistencies they are also replaced with the new infos from the column presentation. Either way the user will have to re-configure the columns, so I think we are safe there. (In reply to comment #8) > -1 for this fix. > The code in InternalTreeModelViewer line 1427: > if (header == null) header = id; > could end up setting the name (visible to the user) of the column to be the > internal id for the column (i.e. org.eclipse.myproject.column1) > It would be better if we either ignored the bogus column completely (more work > required to fix) or any exception in presenting columns simply causes a reset > to the initial columns from the presentation. I am leaning more towards opinion > 2, that way if the currently cached column infos have any other inconsistencies > they are also replaced with the new infos from the column presentation. Either > way the user will have to re-configure the columns, so I think we are safe > there. Michael, I am not sure which fix(es) will finally be accepted and get delivered in 3.8 and 3.7.2. What I want to iterate is, 1) We want this issue to be fixed in 3.7.2 and 3.8.2. It is ok that they employ two different fixes or the same fix. 2) We want a fix such that it does not require the user manually re-configure the columns of the view. This is the reason the initial proposal/idea is to have the viewer automatically flips to the intital columns, rather than keeping the 'broken' persistent columns. If the final fix is to require the user to manually re-configure the columns, it defeats the purpose. The reason I say that is, a few customers already hit this NPE in 3.7.1. We expect more and more customers will hit the same problem once they update to 3.7.2 from 3.7.0. Their feedback is they want a solution better than 'asking customers to do something extra'. Honestly, not many people know they can configure the columns from the view tool bar's pull-down menu. That also means 'ignoring the bad column' is not acceptable. Since 3.7.2 will not accept new APIs or so, I submitted a patch for 3.7.x that purely takes the approach of 'flipping to initial columns.' And, Pawel rewrote the patch to avoid any potential trap of infinite call-stack. Pawel's patch at least addresses our main concern. (In reply to comment #8) > -1 for this fix. I updated the getVislbleColumns() call to check for a missing header text, I also added the unit test. The only possible down side of this change that I can think of is that a programming error in an IColumnPresentation.getHeader() could cause the view to reset its columns for no apparent user from the end user's POV. An NPE in the log would actually be healthy in that respect, but it's a minor point. Thanks for the review and let me know what you think of the update when you get a chance. http://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?id=183d75523e8723fe50fae8e9a3301aef668dcce8 (In reply to comment #9) > > 2) We want a fix such that it does not require the user manually re-configure > the columns of the view. This is the reason the initial proposal/idea is to > have the viewer automatically flips to the intital columns, rather than keeping > the 'broken' persistent columns. I agree, and stated that I preferred to default to the initial columns from the presentation in comment #8. Either way though, if a user has customized the set of columns and an exception causes their set of columns to be changed, they will be required to reconfigure them back to the way they had them. +1 for Pawel's update in comment #10 |