Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 382428

Summary:	Changing navigator root fires 3 selection changed events.
Product:	[ECD] Orion	Reporter:	Susan McCourt <susan>
Component:	Client	Assignee:	Susan McCourt <susan>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	P3	CC:	libingw
Version:	0.5
Target Milestone:	2.0 RC1
Hardware:	PC
OS:	Windows 7
Whiteboard:
Bug Depends on:	380687
Bug Blocks:

Description Susan McCourt

2012-06-12 15:41:05 EDT

In debugging bug 382394, I saw 4 calls to render the commands associated with the selection when you change the navigator root (drill in or use the breadcrumb).

- the first is in "updateNavTools"...the method that updates navigator commands based on a change in the item.
- the next three happen because there are three selection events firing as a result of the selection being set to an empty selection.

Several things to consider:

1 - is it correct to update the selection tools in updateNavTools on an item change?  Or should we assume that selection change events will follow.
2 - why is the selection being reset three times...
3 - the selection service should be smarter about not firing a changed method if the selection did not actually change.  We should make this change but first we should look at #2 and rationalize that this is okay.  (Because fixing this will hide any problems with #2)
4 - if the user were to switch navigator root and in both cases the selection is empty, should a notification be sent?  technically the selection has not changed.  But that means we might still need to do the update in #1.

We should look at this for 1.0.

Comment 1 Susan McCourt

2012-06-12 15:42:08 EDT

Libing, I'm assigning to me but assuming we'll work on this together, from a widget point of view and from nav (client) point of view.

Comment 2 libing wang

2012-06-13 08:33:43 EDT

(In reply to comment #1)
> Libing, I'm assigning to me but assuming we'll work on this together, from a
> widget point of view and from nav (client) point of view.

Sure. The selection model still has room to improve its performance.
There is an existing bug 380687 talking about the life cycle in a generic way. I will address this in it.

Comment 3 Susan McCourt

2013-02-01 13:03:39 EST

Finally looked at this.

(In reply to comment #0)
> 1 - is it correct to update the selection tools in updateNavTools on an item
> change?  Or should we assume that selection change events will follow.

I think this is correct because of what I said in #4.  When the root of the tree is changed, we should update the selection tools to whatever the current selection is.  So this is working properly.

> 2 - why is the selection being reset three times...
The culprit seems to be this, around line 648 of explorer.js:
rowsChanged: function() {
	// notify the selection service of the change in state.
	if(this.explorer.selectionPolicy !== "cursorOnly"){ //$NON-NLS-0$
		this.explorer.refreshSelection();
		this.explorer.initNavHandler();			
	}
},

So basically, every time the rows in the tree change (due to expand/collapse, or a refresh, or whatever), the selection is reset.  This keeps all the selection traversal stuff from holding onto stale dom nodes (I think, Libing would know for sure) so rather than check whether we need to refresh the selection (which might be a complicated check), we just do it all the time.  I think it's a reasonable thing to do, which means we should indeed fix #3.


> 3 - the selection service should be smarter about not firing a changed
> method if the selection did not actually change.  We should make this change
> but first we should look at #2 and rationalize that this is okay.  (Because
> fixing this will hide any problems with #2)

Per above, this is what we should fix.

> 4 - if the user were to switch navigator root and in both cases the
> selection is empty, should a notification be sent?  technically the
> selection has not changed.  But that means we might still need to do the
> update in #1.

Yes, we still do the update in 1 so we are okay.

Fixed in http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=be06d304b3538e2a3055bd49439fdceb45da7684