Community
Participate
Working Groups
I'd like to be able to call TableViewer.setSelection() and not have the underlying TableItems created. I've hacked AbstractTableViewer a bit and I think I have it working. The concept at least appears feasible, but my hack needs more work and testing. The basis of the hack is to eliminate calls to doGetItem() in virtualSetSelectionToWidget(), and to dynamically set the real selection inside the VirtualManager.handleEvent(). I have run into one problem with this hack: bug #251290
This enhancement is also related to bug #246417
I have a similar but distinct requirement. In my application, I'd like to pre-select a particular element as the default selection and scroll the Table to make this visible. In this case, the TableViewer needs an efficient way to lookup the index of the element without loading all of the elements in the Table. Then the TableViewer can use the same basic codepath as Table.setSelection(int[]), which we know is efficient. I propose that we add new API to handle this case. IIndexableLazyContentProvider (or perhaps ILookupLazyContentProvider) would extend ILazyContentProvider and add this method: /** * Find the row index of the parameter element in the set of contents provided * by this object. Under normal usage, this method will only be used to * implement <code>StructuredViewer#setSelection(ISelection)</code> more * efficiently. * * @param element the element to find within the contents served here * @return the zero-based index of the element, or -1 if the element is not found */ public int findElement(Object element); TableViewer should test for this interface on content providers in a similar way that it tests for ITableColorProvider on label providers. If the method is available, AbstractTableViewer.doFindItem(Object) should use it to find and populate the specific Item requested. Otherwise, the implementation should fall back to the current behavior. What say the platform-ui API gurus?
Peter's suggestion of a new mixin interface makes sense to me. Bryan, will you be posting a patch at some point?
Boris, I'd be happy to post a patch. My big concern is that requires OSSC approval, and the last time I went through that, it took about 6 months.
There is a block of code in AbstractTableViewer.virtualSetSelectionToWidget() that I don't understand completely. There is a test for ILazyContentProvider and then what looks like initialization logic, with provider.updateElement() etc. I think that my suggested change to doFindItem() would have the side-effect that this logic is not executed, because of the way "virtualElements" is used. Boris, can you take a look at this and see if there's a simple way to restructure this so all of the invariants are still maintained, even if findItem() returns a TableItem?
BTW, I can post a patch, assuming that Boris can help me understand the code I just mentioned.
Peter, if you are talking about this section: for (int i = 0; virtualElements.size() > 0 && i < doGetItemCount(); i++) { provider.updateElement(i); Item item = doGetItem(i); if (virtualElements.contains(item.getData())) { indices[count++] = i; virtualElements.remove(item.getData()); if (firstItem == null) { firstItem = item; } } } This loops through the items, asking the content provider to update each item, followed by a check if the i'th item's data matches one of the elements we are asked to select. This will materialize items eagerly, starting at item index 0, until all elements to be selected have been found, or until we have materialized all items. Does this make sense?
Created attachment 115846 [details] Proposed mixin interface
Created attachment 115847 [details] TableViewer with more efficient setSelection() implementation I have run this through a few trivial tests, and it appears to do the job. Please review, and if it passes muster, change TableViewer directly instead of using this class as-is. As far as I know, this is enough to close the issue.
I attached the code in this form so that it can be dropped into an existing RCP app running on 3.4. I've been testing this way with my own app. Bryan, this has the side-effect of creating the TableItems for exactly the items that are selected. Is this still okay for your use case? It's not clear from your initial request whether you need absolutely no TableItems to be created, or if a minimal, non-zero number is acceptable.
(In reply to comment #10) > I attached the code in this form so that it can be dropped into an existing RCP > app running on 3.4. I've been testing this way with my own app. Peter, could you please also upload this in patch form? We need that format so that the automated IP tracking tools can know how much you contributed etc. Thanks!
Peter, for my case, I need exactly zero TableItems created when setSelection() is called when the VIRTUAL flag is set.
Created attachment 115870 [details] Previous attachments as patch FYI, this patch expects a root directory inside the org.eclipse.jface.viewers package. (I have the classes in my local SVN repo, not the JFace CVS.) Bryan, I think you can use the same basic idea, but you'll have to rework the AbstractTableViewer.virtualSetSelectionToWidget() method a bit. (I didn't want to mess with any private methods myself.) Instead of this block: Object o = list.get(i); Widget w = findItem(o); if (w instanceof Item) { Item item = (Item) w; indices[count++] = doIndexOf(item); if (firstItem == null) { firstItem = item; } } else { virtualElements.add(o); } ... and the whole block Boris pasted above, just do: indices[count++] = provider.findElement(list.get(i)); It appears that there is no way to call Table.showItem() without a real TableItem. This means that if (reveal == true), we can't get to zero. However, you can delay the doGetItem() call until inside the final block, so you'll have a max of 1 TableItem created and only in that specific case.
Peter & Boris, do either of you have time to modify the selection code? If not I can start the OSSC approval process for attaching a patch and hopefully it won't take 6 months this time. The changes should be fairly simple. The TableViewer needs to keep the selection independent of the Table. The VirtualManager might be a good place to store this. getSelection() simply needs to return this stored selection. The last piece is that the actual table selection needs to be updated when TableItems are created. The VirtualManager.handleEvent() seems like the place to do this.
(In reply to comment #14) Does Peter's idea from comment 13 not work? I am reluctant to do what you (Bryan) suggest, because then the selection as seen through the TableViewer would be different from what the underlying Table widget thinks is selected.
Boris, I believe the answer is no. The fundamental problem is that virtualSetSelectionToWidget() calls doGetItem() which causes TableItems to be created. The whole point of "virtual selection" is that the table viewer selection is different from the table selection. Please have a look at the bug referenced in comment #1 for the motivation behind this request.
Yes, that bug shows that we'd be fighting the native behavior if we tried to maintain a "selection" that is different from the operating system's idea of what is selected. Btw, Peter's idea from comment 13 was to ask the content provider (using the new proposed API) for the indices, which means you would be able to issue the calls: doDeselectAll(); doSelect(indices); without any calls to doGetItem(). My question was if this does not work (I haven't tried it, so I don't know if doSelect() will materialize items).
(In reply to comment #17) Boris, I was basing my "no" answer on comment #10. Peter, can you verify that I'm understanding your comment correctly.
My proposal in comment 13 would avoid doGetItem() entirely, as long as (reveal == false). As Boris says, doDeselectAll() and doSelect() would still get called. Whether or not this results in TableItems being created is not specified in the SWT Table implementation and is not specified in the API. Bryan, I don't think you're going to get a rock-solid cross-platform guarantee using this approach. If that's okay, I'd suggest that you setup a trivial test case to see whether these two methods cause TableItems to be created in your environment. If they don't, we can proceed with the approach in comment 13 and satisfy both of us.
(In reply to comment #19) Peter, thanks for the clarification. I'll try to test your patch tomorrow, or this weekend, and post my results.
Peter, your proposed patch does not work for me. When I select all, all of the rows are materialized as TableItems.
I just realized that reveal was set to true, but when I changed it to false, the rows are still materialized. In any event, the limitation of requiring reveal to be false to avoid row materialization probably won't work for our use case.
I have received permission to work on this on my own time. I'll try to put a patch together in the next few days.
Created attachment 116722 [details] virtual selection patch Here's a first attempt at my proposed patch. Using my testcase from bug #246417, the time to select all on OS X went from 1.449s to 0.006s, and on Linux went from 6.278s to 0.007s The selection drawing problem described in bug #251290 does not occur on Linux so it may be OS X specific.
Boris, could you please comment on my patch? I have time to work on it over the next three weeks.
I agree with Peter's comment 19: > Bryan, I don't think you're going to get a rock-solid cross-platform guarantee > using this approach. I'd be fine with adding API as suggested by Peter, and otherwise enable people like Bryan to subclass TableViewer with additional behavior. Bryan, what would prevent you from moving your code into a subclass? Do you need additional "protected" method hooks, or should we make some existing private methods protected?
(In reply to comment #26) Boris, Please see comment #21 and comment #22. Peter's proposed patch does not work for me. I'd also appreciate it if someone could explain why my patch would not work cross-platform. So far, it seems to work fine on OS X, and Linux. I'll see if I can test Windows and AIX this week. I'll also see if I can put my changes into a subclass. IMHO, when you set the "VIRTUAL" style, the selection should also be virtual, and I personally consider the current behavior a bug. I filed this as an enhancement because the current behavior has not been challenged by others.
(In reply to comment #27) > I'll also see if I can put my changes into a subclass. Any progress on this?
(In reply to comment #28) > (In reply to comment #27) > > I'll also see if I can put my changes into a subclass. > > Any progress on this? > I started working on this when my hard drive died. I got my backup restored yesterday so I can try to finish the work this week. One thing I noticed was that I was duplicating a lot of internal code. I hope to have more info later this week.
(In reply to comment #29) > ... I hope to have more info later > this week. ping?
Sorry Boris, I haven't had time to look at this. Thanks for pinging me. I'll try to look at it soon.
Boris, I've started looking into this again, and I've run into a problem. When StructuredViewer.widgetSelected() is called in response to clicking on a row in the table, the event.stateMask is always 0 no matter which modifier keys were pressed. Is this just the way the event works, or a bug? Is there any other way to tell that I need to start with a new selection vs adding or removing from the selection? I'm currently blocked by this problem.
(In reply to comment #32) > Boris, I've started looking into this again, and I've run into a problem. When > StructuredViewer.widgetSelected() is called in response to clicking on a row in > the table, the event.stateMask is always 0 no matter which modifier keys were > pressed. Is this just the way the event works, or a bug? Is there any other > way to tell that I need to start with a new selection vs adding or removing > from the selection? I'm currently blocked by this problem. Could you hook a key listener to track which modifier keys are pressed?
Boris, the key listener works except the first time the table is clicked. When I bring up my test view, the table does not have focus, and does not receive any key events until it is selected. Any ideas on how to work around this problem?
Released Peter's contribution. Bryan, we are at the API freeze for 3.5 now, so if you need additional hooks as API, we won't be able to do anything about it until 3.6.
