Bug 140941 - [Viewers] JFace viewers cannot handle duplicate elements unless they have an element map
Summary: [Viewers] JFace viewers cannot handle duplicate elements unless they have an ...
Status: CLOSED WONTFIX
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Platform UI Triaged CLA
QA Contact:
URL:
Whiteboard: stalebug
Keywords: helpwanted
Depends on:
Blocks:
 
Reported: 2006-05-09 17:19 EDT by Stefan Xenos CLA
Modified: 2020-06-17 14:25 EDT (History)
3 users (show)

See Also:


Attachments
Partial fix (4.54 KB, patch)
2006-05-09 17:26 EDT, Stefan Xenos CLA
no flags Details | Diff
Subclass with proposed change to CheckboxTreeViewer (991 bytes, text/plain)
2014-01-29 14:55 EST, Zachary DeLuca CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Xenos CLA 2006-05-09 17:19:28 EDT
Callers of findItems assume that the method returns all items associated with the given element. This is true normally, but if the element map is disabled it only returns the FIRST element.

The impact for viewers without element maps is that duplicate items are not refreshed in response to label updates (only the first occurrance is).

By inspecting the code, it looks like it would also be impossible to remove duplicate items.
Comment 1 Stefan Xenos CLA 2006-05-09 17:26:32 EDT
Created attachment 40880 [details]
Partial fix

Here's the start of a fix. More intended to point out where the problem is than as a refined solution. It addresses the issue in TreeViewer, but not the other StructuredViewers.

Note: there is also a JavaDoc bug in findItems. It says: "If the element map is disabled, the widget is found via <code>doFindInputItem</code>.". This is untrue. The current implementation finds the element using doFindItem (not doFindInputItem)... however, that should not be part of the method contract either since neither doFindItem nor doFindInputItem would be capable of returning the correct answer.
Comment 2 Boris Bokowski CLA 2006-05-10 10:17:51 EDT
This needs to be documented in the Javadoc. (in the porting guide too?)
Comment 3 Boris Bokowski CLA 2006-05-11 00:18:29 EDT
Filed bug 141192 to track the Javadoc change. Moving this one to 3.3.
Comment 4 Boris Bokowski CLA 2007-04-27 21:04:22 EDT
Deferring
Comment 5 Boris Bokowski CLA 2009-11-26 09:55:49 EST
Hitesh is now responsible for watching bugs in the [Viewers] component area.
Comment 6 Zachary DeLuca CLA 2014-01-29 02:53:09 EST
Has this been addressed at all, or is there any known workaround? I have a use case where I'd like to have objects occurring multiple times in a CheckboxTreeViewer. I've set a CheckStateProvider, but when the state of an object that has duplicates is changed, only the check state of the first occurrence of the object is updated.

One thing I can think of is create a simple wrapper around the objects that causes them to look and behave the same within the tree while not actually being the same object. I don't really want to do that, though.

Thanks,
Zach
Comment 7 Paul Webster CLA 2014-01-29 09:43:00 EST
It says your element map is disabled.  Why is that?

But no, no one is looking at this area but we accept contributions.  http://wiki.eclipse.org/Platform_UI/How_to_Contribute

PW
Comment 8 Zachary DeLuca CLA 2014-01-29 12:08:52 EST
(In reply to Paul Webster from comment #7)
> It says your element map is disabled.  Why is that?

Where does it say that? I used viewer.setUseHashlookup(true); but that had no effect.

> But no, no one is looking at this area but we accept contributions. 
> http://wiki.eclipse.org/Platform_UI/How_to_Contribute

Okay, I'll see if there is anything I can do.
Comment 9 Paul Webster CLA 2014-01-29 12:19:09 EST
(In reply to Zachary DeLuca from comment #8)
> 
> Where does it say that? I used viewer.setUseHashlookup(true); but that had
> no effect.

(In reply to Stefan Xenos from comment #0)
> Callers of findItems assume that the method returns all items associated
> with the given element. This is true normally, but if the element map is
> disabled it only returns the FIRST element.

Sorry, that was based on the first comment, that findItems will return more than one widget if the system is using an element map - org.eclipse.jface.viewers.StructuredViewer.findItems(Object)

PW
Comment 10 Zachary DeLuca CLA 2014-01-29 14:53:34 EST
So I just saw in the javadoc for AbstractTreeViewer that in order to support multiple equal elements I need to use ITreePathContentProvider when setting the content provider, in addition to using setUseHashlookup(true). I did that, and properly implemented ITreePathContentProvider.getParents to include all possible parents of my duplicate entries. Still, this has no effect.

So I dug into CheckBoxTreeViewer and looked at specifically doUpdateItem and setChecked/setGrayed. I determined that what is happening (I think?) is although doUpdateItem is called correctly (i.e. called for every occurrence of the duplicate item), the subsequent calls to setChecked/setGrayed use internalExpand to locate the widget rather than just using the item provided to doUpdateItem. As far as I can tell, internelExpand will only locate the first occurrence of the item, and therefore the checked/grayed state of the others never get updated.

I subclassed CheckboxTreeViewer (not supposed to, I know, but much easier for testing than trying to change CheckboxTreeViewer directly) and I will attach it for discussion. Basically all it does is set the checked/grayed state directly in doUpdateItem rather than relying on the calls to setChecked/setGrayed that locate the wrong widget. This is not a fix or a patch of any kind, but it appears to solve my use case so I am going to use it for now.

Can anyone predict any pitfalls I might encounter by using this? Should I open a new bug report for this? Is it even a bug? I would like to contribute to make this standard functionality but the SWT framework is quite complicated to me and I have no idea what else a change like this might affect. I could use some guidance on that.

Lastly, I don't even know why I did this but after I got it working I got curious. I removed the call to setUseHashlookup(true) and went back to my original ITreeContentProvider. So by the information provided in the javadoc of AbstractTreeViewer, this situation cannot support multiple equal elements. And yet, everything works just as it did before making that change, I have duplicates at different paths in the tree and they update as desired. So can someone please explain to me, what exactly is meant by "multiple equal elements" and how does it differ to what I am doing?

Sorry for the long post, thank you for your help.
Comment 11 Zachary DeLuca CLA 2014-01-29 14:55:14 EST
Created attachment 239439 [details]
Subclass with proposed change to CheckboxTreeViewer
Comment 12 Paul Webster CLA 2014-02-04 09:49:36 EST
(In reply to Zachary DeLuca from comment #10)
> So I dug into CheckBoxTreeViewer and looked at specifically doUpdateItem and
> setChecked/setGrayed. I determined that what is happening (I think?) is
> although doUpdateItem is called correctly (i.e. called for every occurrence
> of the duplicate item), the subsequent calls to setChecked/setGrayed use
> internalExpand to locate the widget rather than just using the item provided
> to doUpdateItem. As far as I can tell, internelExpand will only locate the
> first occurrence of the item, and therefore the checked/grayed state of the
> others never get updated.

Yes, your analysis is correct.  doUpdateItem already has the item, but uses the public setChecked(*) method which only takes an element.  setChecked(*) supports expanding the tree to create items, but since we already have the item that's not needed.


> 
> Can anyone predict any pitfalls I might encounter by using this? Should I
> open a new bug report for this? Is it even a bug? I would like to contribute
> to make this standard functionality but the SWT framework is quite
> complicated to me and I have no idea what else a change like this might
> affect. I could use some guidance on that.

Your change looks like the right place.  As a fix on CheckboxTreeViewer, I would make a private internalsetChecked/internalSetGrayed methods that take a widget and the state.  Refactor the TreeItem check out into that method, so it can be called from setChecked(*) as well as doItemUpdate(*).

See https://wiki.eclipse.org/Platform_UI/How_to_Contribute#Setting_up_your_SDK for the steps on setting you the JFace code and pushing a change to Gerrit and http://wiki.eclipse.org/CLA for accepting the CLA.


> Lastly, I don't even know why I did this but after I got it working I got
> curious. I removed the call to setUseHashlookup(true) and went back to my
> original ITreeContentProvider. So by the information provided in the javadoc
> of AbstractTreeViewer, this situation cannot support multiple equal
> elements. And yet, everything works just as it did before making that
> change, I have duplicates at different paths in the tree and they update as
> desired. So can someone please explain to me, what exactly is meant by
> "multiple equal elements" and how does it differ to what I am doing?

I don't know this, I'm not that familiar with the more complex use of viewers.

Thanks for looking into this.

PW
Comment 13 Eclipse Genie CLA 2020-06-17 14:25:51 EDT
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. As such, we're closing this bug.

If you have further information on the current state of the bug, please add it and reopen this bug. 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.