Bugzilla – Full Text Bug Listing
| Summary: | [Viewers] Adding new tree items to an expanded Tree node scrolls away the current selection | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Tod Creasey <Tod_Creasey> |
| Component: | SWT | Assignee: | Platform-SWT-Inbox <platform-swt-inbox> |
| Status: | CLOSED WONTFIX | QA Contact: | Hitesh <hsoliwal> |
| Severity: | normal | ||
| Priority: | P5 | CC: | birsan, carolynmacleod4, grant_gayed, markus.kell.r, Michael.Valenta, Mike_Wilson, mistria, n.a.edgar, snorthov |
| Version: | 3.0 | Keywords: | helpwanted |
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | stalebug | ||
| Bug Depends on: | |||
| Bug Blocks: | 42158 | ||
Happens on motif and photon also. This definately impacts the usability of the concurrency work that is going into R3.0. Has any progress been made? I'm looking at motif/photon. Car can you look at win32? The problem now is with deletion. When you delete an item the visible area is being set to the top. What is happening with adding items is that when you insert an item 'above' your current position in the tree, the tree has to push you down to make space for the item. It's easier to see this if you modify the snippet code to just add one item. Of course, if you add 10 items, the tree has to make space for 10 items, so it pushes down more stuff. Depending on what you are looking at you might lose context. If you insert items 'below' your current position in the tree, it won't appear to scroll at all, because it pushes down stuff that is below you. Not sure how clear this explanation is, but I think it is reasonable behavior, because certainly if you were watching the items being added, it is what you would expect to see. The only thing you could do is getTopItem before inserting and setTopItem when you are done (but this will flash). I will investigate the problem with deletion. Looked at delete, and the behavior seems reasonable here, too. Try this code.
It only adds or deletes one item, but this just lets you see better what's
going on - feel free to add/delete more at once. Expand item2 and any others
you like, scroll anywhere, then add, add, add, delete, delete, delete (as many
times as you like). Scroll somewhere else, and repeat. Please tell me if there
is anything that you think should be done here, or if we should close this bug
report.
Thanks.
import org.eclipse.swt.SWT;
import org.eclipse.swt.events.SelectionEvent;
import org.eclipse.swt.events.SelectionListener;
import org.eclipse.swt.widgets.Button;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Shell;
import org.eclipse.swt.widgets.Tree;
import org.eclipse.swt.widgets.TreeItem;
public class AddToTreeTest {
private static int depth = 10;
public static void main(String[] args) {
Display display = new Display();
Shell shell = new Shell(display);
final Tree tree =
new Tree(
shell,
SWT.MULTI | SWT.H_SCROLL | SWT.V_SCROLL |
SWT.BORDER);
for (int i = 0; i < depth; i++) {
TreeItem item1 = new TreeItem(tree, SWT.NONE);
String name = "Item" + String.valueOf(i);
item1.setText(name);
createItem(item1, name, 1);
}
tree.setBounds(0, 0, 200, 200);
Button addButton = new Button(shell, SWT.PUSH);
addButton.setText("Add entry");
addButton.addSelectionListener(new SelectionListener() {
public void widgetSelected(SelectionEvent e) {
System.out.println("BEGIN ADD: Selection=" +
tree.getSelection()[0]);
System.out.println("BEGIN ADD: TopItem=" +
tree.getTopItem());
TreeItem[] items = tree.getItems();
if (items.length > 0) {
for (int i = 0; i < 1; i++) {
TreeItem item1 = new TreeItem
(items[2], SWT.NONE);
String name = "Item" +
String.valueOf(String.valueOf(System.currentTimeMillis()));
item1.setText(name);
System.out.println("AFTER new
TreeItem " + i + ", Selection=" + tree.getSelection()[0]);
System.out.println("AFTER new
TreeItem " + i + ", TopItem=" + tree.getTopItem());
}
}
System.out.println("END ADD: Selection=" +
tree.getSelection()[0]);
System.out.println("END ADD: TopItem=" +
tree.getTopItem());
}
public void widgetDefaultSelected(SelectionEvent e) {
}
});
Button deleteButton = new Button(shell, SWT.PUSH);
deleteButton.setText("Delete entry");
deleteButton.addSelectionListener(new SelectionListener() {
public void widgetSelected(SelectionEvent e) {
System.out.println("BEGIN DELETE: Selection="
+ tree.getSelection()[0]);
System.out.println("BEGIN DELETE: TopItem=" +
tree.getTopItem());
TreeItem[] items = tree.getItems();
items = items[2].getItems();
if (items.length > 1) items[1].dispose();
System.out.println("END DELETE: Selection=" +
tree.getSelection()[0]);
System.out.println("END DELETE: TopItem=" +
tree.getTopItem());
}
public void widgetDefaultSelected(SelectionEvent e) {
}
});
addButton.setBounds(300, 0, 100, 50);
deleteButton.setBounds(300, 100, 100, 50);
shell.setSize(400, 400);
shell.open();
while (!shell.isDisposed()) {
if (!display.readAndDispatch())
display.sleep();
}
display.dispose();
}
private static void createItem(TreeItem parent, String name, int
count) {
TreeItem item = new TreeItem(parent, SWT.NONE);
count++;
String newName = name + String.valueOf(count);
item.setText(newName);
if (count < depth)
createItem(item, name, count);
}
}
Please understand: The reason this PR was entered is that, we believe the behavior is *not* reasonable. We should talk about this. I will investigate further. I'll see what list and table do, and I'll see what tree does on the other platforms, and we'll come up with a plan. Stand by... :) My test case does not indicate the problem which was with selection maintenance. I will take this PR and commit a fix that Carolyn and I came up with for the JFace viewer. Here is Caraolyns fix
protected void setSelectionToWidget(List v, boolean reveal) {
if (v == null) {
setSelection(new ArrayList(0));
return;
}
int size = v.size();
List newSelection = new ArrayList(size);
for (int i = 0; i < size; ++i) {
// Use internalExpand since item may not yet be
created. See 1G6B1AR.
Widget w = internalExpand(v.get(i), true);
if (w instanceof Item) {
newSelection.add(w);
}
}
Tree tree = null;
if(getControl() instanceof Tree)
tree = (Tree) getControl();
getControl().setRedraw(false);
TreeItem topItem = null;
if(tree != null)
topItem = tree.getTopItem();
setSelection(newSelection);
if(tree != null && topItem != null)
tree.setTopItem(topItem);
getControl().setRedraw(true);
if (reveal && newSelection.size() > 0) {
showItem((Item) newSelection.get(0));
}
}
A cleaner fix (one in TreeViewer.setSelection()) has been released for build
>20031016
*** Bug 42158 has been marked as a duplicate of this bug. *** The new change uses turns redraw off then back on. This always forces a redraw, which we don't want to have happen whenever the selection is programmatically changed. I haven't verified that this is actually a problem, but I'm betting it is. I assumed that the "cleaner fix" would not turn redraw off/on. If it is doing that, then we need to find some other answer. Further testing without it indicates that it is not required so I have just removed the turning off of redraw(). Sorry, Tod - I should have suggested trying it without the redraw off/on, however note that you will want to try your exact use case, the one that you know causes the scrolling, because when the topItem is forced to scroll by setSelection and then the topItem is reset afterwards, what you may see is a very disconcerting double-scroll (scroll away, scroll back) that might be undesirable. You may find that the best possible solution avoids calling setSelection entirely. So if you can do that, you are much better off... The change to restore the top item causes the seizure-inducing flashiness described in bug 59449. Steve and Grant have been looking at this and are willing to help come up with a better solution. It's unclear that the change should go in setSelection anyway. This is called a lot more often than just for adds and removes. We're probably getting a free call from preservingSelection, which most viewer modification methods call. There's no need to call preservingSelection for adds, since this shouldn't mess up any existing selection. And for removes, if selected items are removed then we should just let that happen, as long as notification of the change is sent afterwards. *** Bug 59449 has been marked as a duplicate of this bug. *** Have you guys changed something since this bug was opened?
I am running 200404270800, and I tried the snippet and I tried the "switch
editors in the editor tab folder while watching the Package Explorer tree" use
case, and I just don't see anything I would call "seizure-inducing
flashiness". I see a little blip when I switch editors, but it's not that bad.
I'm really sorry. Maybe my machine is super-fast? Or maybe there's something
in the way I am clicking or something? Tod, McQ, Nick, can anyone write down a
very specific use case, maybe switching between some very specific editors? I
currently have most of the ui, workbench, and core projects loaded, plus swt
projects, so if you can show the fail case using any 2 files in those
projects, that would be best - thanks!
For info for this bug, the setSelection code that caches/restores the topItem
in the tree is:
protected void setSelection(List items) {
Item[] current = getSelection(getTree());
//Don't bother resetting the same selection
if(haveSameData(items,current))
return;
//Cache the current position first
Tree cachedTree = getTree();
TreeItem topItem = cachedTree.getTopItem();
TreeItem[] newItems = new TreeItem[items.size()];
items.toArray(newItems);
getTree().setSelection(newItems);
//Restore the current position
if(topItem != null)
cachedTree.setTopItem(topItem);
}
I suppose "jumpiness" is more accurate than "flashiness". Try: - load the platform-ui module from CVS - open new window in Java perspective - ensure Link with Editor is on in the Package Explorer - select org.eclipse.jface - open type (Ctrl+Shift+T) on WWinPluginAction - open type on ViewerSorter - switch back and forth between these, with the occasional Collapse All interspersed Note that both of these file are at the end of a package with many types, so scrolling is typically required after expanding. It does this in multiple steps, and the scroll bar's handle jumps around a lot. No matter what I try, I can't get this to happen on my machine. I can see the problem on Steve's machine, so I'll have to debug it there. We compared: - comctl32 versions (same - 6.0.2800.1106), - vm (same - 1.4.2_01), - vm args (same, after I deleted my extra "-Xmx400m" vm args), - and screen resolution (same, after I made mine 1400 x 1050), I also tried running with his workspace (it's on the net). Nothing. I just don't have a problem with flash in the tree. The only other thing I can think of is that I have JAWS & Window-Eyes installed, and they do tend to do strange things to one's computer. :) Tod, since you have JAWS & W-E installed too, can you please follow Nick's use case, and tell me what you see? Thanks. Tod doesn't have the wild flashing, either. Don't know why my machine and his machine don't demonstrate the problem - maybe faster graphics cards? Is that ridiculous? Anyhow, guess I have to try to work around it for all those flashy machines out there. :( Tod, I commented out the caching of the top item, and things seem to work ok,
except for the very first time (after the workbench is started).
Does this work for you? Why did we add the cache? The bug report doesn't
really give an eclipse use case for the original problem.
protected void setSelection(List items) {
Item[] current = getSelection(getTree());
//Don't bother resetting the same selection
if(haveSameData(items,current))
return;
//Cache the current position first
//Tree cachedTree = getTree();
//TreeItem topItem = cachedTree.getTopItem();
TreeItem[] newItems = new TreeItem[items.size()];
items.toArray(newItems);
getTree().setSelection(newItems);
//Restore the current position
//if(topItem != null)
// cachedTree.setTopItem(topItem);
}
Re comment 23, I'm on Win2K. Maybe there's a platform difference. I considered that possibility, but the problem occurs on Steve's XP machine. I'm just waiting now for Tod's answer to what the original problem was, because this new problem seems to go away (on Steve's machine) if I take out the original workaround. The original problem was loss of focus - you would add an item and your selection would be scrolled away I am guessing that you don't mean "loss of keyboard focus", but something more along the lines of "loss of context" - right? Would you be able to come up with an eclipse example where this is bad behaviour? I tried "Add class" and it behaves as I would expect - it selects the new item, and scrolls to it if necessary. I am running eclipse with the commented-out caching in setSelection that I gave you above (in comment #24). I am very sorry that I just can't figure out what the problem is. Please drop by, and we'll go use Steve's machine and you can show me what needs to be fixed. Comment 0 and comment 2 explain the original problem: You DON'T want to scroll to the position where the new item was inserted. The item might be put there by a background task (e.g. one which replaces a "Pending..." label with the real items), and you don't want the background task to do anything but inserting the items, in particular no horizontal nor vertical scrolling. An eclipse-example is bug 42158, which was blocked by this bug. I guess Nick's comment 17 only meant to criticize the implementation of this bug's fix, and not the validity of this PR. IMO bug 59449 is not a dup, but only a consequence of the solution taken for this PR. It that is correct, why was the setSelection() method patched? How does this method have anything to do with adding items? I dunno, maybe because of this call hierarchy: ?
setSelection(List) - TreeViewer
- setSelectionToWidget(List, boolean) - AbstractTreeViewer
- setSelectionToWidget(ISelection, boolean) - StructuredViewer
- preservingSelection(Runnable) - StructuredViewer
- refresh(...)
The call path you show is accurate. But I agree that setSelection is too far down to be trying to keep the selection visible. Even then, the workaround might not work if the added items appear between the top item and the selected item. It also doesn't catch the case where viewer.add is called, since it doesn't call preservingSelection (since adding elements can't affect the selection). Perhaps internalRefresh should just call showSelection. But that may still cause an unwanted scroll, particularly in the case of multiple selection (not sure how showSelection behaves in that case). It would be nice if there was an isSelectionVisible method -- if true, we wouldn't do anything. Just one more data point... I had to reboot my machine into Win2K to check on something else, and I decided to see if I have a jumpy tree over here. Note that my Windows 2000 install is really really basic, and I do not have any accessibility tools installed. Note that I do NOT have the jumpy tree here either. So we have ruled out OS, VM, and pretty much anything else software-related. It pretty much has to be the hardware of my machine that is doing something right. I must have some really fast graphics card or something. (Or maybe everyone else has a slow one :) Anyhow, I realize that this does not make the problem go away in the general case - it is just interesting. Removed re-setting of setTopItem from TreeViewer.setSelection. This fixes the flashiness problem, at the expense of the selection possibly getting hidden when concurrent changes are made to the tree. If this is problematic for the original concurrent use case, then we can investigate other options, such as using showSelection after a refresh. > If this is problematic for the original concurrent use case, then we can
> investigate other options, such as using showSelection after a refresh.
Yes, please. The original problem has come back with the removal of the topItem
re-setting code from setSelection().
I guess we need something like preservingTopItem(Runnable updateCode), which is
modeled after StructuredViewer#preservingSelection(..) and which is used by the
add(..), remove(..), and refresh(..) methods.
Sigh, Nick or someone please hold my hand and show me the original case. Reading this PR, the title says "Adding new ...", TC claims "The problem is now with deletion." and the (bogus) fix involved selection. I have no idea what is actually going on. Markus, can you please provide reproduceable steps showing the problem described in your last comment? I20040518-0816 - synchronize your workspace and make sure you have (lots of) incoming changes - ensure Preferences > Workbench > "Always run in background" is checked - expand projects in Synchronize view (e.g. press Numpad_Multiply on each project) - choose "Update" from context menu of first project - scroll down without selecting anything in the tree -> when the update is finished, the first item of the tree is revealed (it got the focus - but not the selection - because the previous focus has been removed) From bug 100565 comment 16: The check for same selection in TreeViewer.setSelection was added in v1.11 of TreeViewer (by Tod), as part of the fix for bug 42175. With the removal of get/setTopItem from this code in v1.15 (by me), it's unclear whether this check is still needed. Should review this change when this bug is revisited. Hitesh is now responsible for watching bugs in the [Viewers] component area. This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie. |
20030826 M3 candidate When you add items somewhere in a tree if the node you are adding too is expanded the tree will scroll to the new point. STEPS 1) Run the example below 2) Expand Item 2 3) Scroll down to Item 9 and expand downa few levels until Item 2 is not visible 4) Hit the Add button which adds to Items 2. Tree is scrolled to the added item so you lose your current context /* * (c) Copyright 2001 MyCorporation. * All Rights Reserved. */ import org.eclipse.swt.SWT; import org.eclipse.swt.events.SelectionEvent; import org.eclipse.swt.events.SelectionListener; import org.eclipse.swt.widgets.Button; import org.eclipse.swt.widgets.Display; import org.eclipse.swt.widgets.Shell; import org.eclipse.swt.widgets.Tree; import org.eclipse.swt.widgets.TreeItem; public class AddToTreeTest { private static int depth = 10; public static void main(String[] args) { Display display = new Display(); Shell shell = new Shell(display); final Tree tree = new Tree( shell, SWT.MULTI | SWT.H_SCROLL | SWT.V_SCROLL | SWT.BORDER); for (int i = 0; i < depth; i++) { TreeItem item1 = new TreeItem(tree, SWT.NONE); String name = "Item" + String.valueOf(i); item1.setText(name); createItem(item1, name, 1); } tree.setBounds(0, 0, 200, 200); Button addButton = new Button(shell, SWT.PUSH); addButton.setText("Add entry"); addButton.addSelectionListener(new SelectionListener() { /* (non-Javadoc) * @see org.eclipse.swt.events.SelectionListener#widgetSelected (org.eclipse.swt.events.SelectionEvent) */ public void widgetSelected(SelectionEvent e) { TreeItem[] items = tree.getItems(); if (items.length > 0) { for (int i = 0; i < 10; i++) { TreeItem item1 = new TreeItem (items[2], SWT.NONE); String name = "Item" + String.valueOf( String.valueOf(System.currentTimeMillis())); item1.setText(name); } } } /* (non-Javadoc) * @see org.eclipse.swt.events.SelectionListener#widgetDefaultSelected (org.eclipse.swt.events.SelectionEvent) */ public void widgetDefaultSelected(SelectionEvent e) { } }); addButton.setBounds(300, 0, 100, 50); shell.setSize(400, 400); shell.open(); while (!shell.isDisposed()) { if (!display.readAndDispatch()) display.sleep(); } display.dispose(); } private static void createItem(TreeItem parent, String name, int count) { TreeItem item = new TreeItem(parent, SWT.NONE); count++; String newName = name + String.valueOf(count); item.setText(newName); if (count < depth) createItem(item, name, count); } } context. new TreeItem(parent, SWT.NONE);