Bug 210448 - [Viewers] [JFace] AbstractTreeViewer inputChanged setRedraw should be in try/finally
Summary: [Viewers] [JFace] AbstractTreeViewer inputChanged setRedraw should be in try/...
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.3   Edit
Hardware: PC All
: P3 minor (vote)
Target Milestone: 3.4 M4   Edit
Assignee: Boris Bokowski CLA
QA Contact:
URL:
Whiteboard: hasPatch
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2007-11-20 16:06 EST by Micah Hainline CLA
Modified: 2007-11-21 16:11 EST (History)
0 users

See Also:


Attachments
Patch for org.eclipse.jface HEAD (1.32 KB, patch)
2007-11-20 16:19 EST, Micah Hainline CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Micah Hainline CLA 2007-11-20 16:06:37 EST
The inputChanged method on the AbstractTreeViewer makes use of setRedraw, which is good, but it doesn't use a try/finally block as per recommendations.

If an exception is thrown from within one of the methods called, redraw will remain false.

Additionally, the commented out portion is fine, but the fact that we have conditionals in the code that always evaluate to true is less fine.  When we're ready to actually determine when to use redraw we could put them back in.

Current code:
protected void inputChanged(Object input, Object oldInput) {
    preservingSelection(new Runnable() {
        public void run() {
            Control tree = getControl();
            boolean useRedraw = true;
            // (size > REDRAW_THRESHOLD) || (table.getItemCount() >
            // REDRAW_THRESHOLD);
            if (useRedraw) {
                tree.setRedraw(false);
            }
            removeAll(tree);
            tree.setData(getRoot());
            internalInitializeTree(tree);
            if (useRedraw) {
                tree.setRedraw(true);
            }
        }
    });
}

Suggestion:
protected void inputChanged(Object input, Object oldInput) {
    preservingSelection(new Runnable() {
        public void run() {
            Control tree = getControl();
            try {
                tree.setRedraw(false);
                removeAll(tree);
                tree.setData(getRoot());
                internalInitializeTree(tree);
            } finally {
                tree.setRedraw(true);
            }
        }
    });
}
Comment 1 Micah Hainline CLA 2007-11-20 16:19:06 EST
Created attachment 83360 [details]
Patch for org.eclipse.jface HEAD

This patch brings HEAD to the code snippet from comment 0.
Comment 2 Boris Bokowski CLA 2007-11-21 10:13:11 EST
Thanks for the patch. Released to HEAD with minor changes (contribution comment, moved setRedraw(false) out of the try block).

	            tree.setRedraw(false);
	            try {
	                removeAll(tree);
	                tree.setData(getRoot());
	                internalInitializeTree(tree);
	            } finally {
	                tree.setRedraw(true);
	            }
Comment 3 Micah Hainline CLA 2007-11-21 16:11:34 EST
(In reply to comment #2)
> moved setRedraw(false) out of the try block

Thanks for getting that in Boris.  I saw that setRedraw(false) thing right after I put in the patch and thought "Theoretically speaking that would be better off... eh, they'll catch that."  Good to see my faith was not misplaced. ;)