Bug 174355

Comment 1 Eric Moffatt 2007-02-15 15:00:59 EST

Tom, is this related to your work ??

Comment 2 Thomas Schindl 2007-02-15 15:07:52 EST

Eric I'm working on a fix with Boris I expect a solution within the next hour ;-)

Comment 3 Thomas Schindl 2007-02-15 16:55:15 EST

Created attachment 59101 [details]
patch

internals have changed a bit, standard providers now use ViewerLabel to pass information may we can make the new CellLabelProvider#update(ViewerCell,ViewerLabel) protected? A strange thing I noticed in our test suite, I think there's a failure in tests, I need to recheck that. When I fix the test code all test are OK.

Comment 4 Thomas Schindl 2007-02-15 17:02:52 EST

Ok now it's clear the new implementation in TableColumnViewerLabelProvider unrevealed a bug in our test suite

Comment 5 Thomas Schindl 2007-02-15 17:08:19 EST

Created attachment 59103 [details]
one more time

same than before but a test for trees is also included now

Comment 6 Frank Lyner 2007-02-16 09:21:51 EST

There is still at least one interface that isn't supported anymore: ITreePathLabelProvider

Comment 7 Thomas Schindl 2007-02-16 10:06:57 EST

you are right Frank, I'll workout a fix. Does the IViewerLabelProvider stuff now work for you?

Comment 8 Frank Lyner 2007-02-16 10:10:50 EST

Yes, the IViewerLabelProvider is working now.

Comment 9 Thomas Schindl 2007-02-16 11:19:04 EST

Created attachment 59158 [details]
fix the treepath problem

fix the problem and add test case

Comment 10 Thomas Schindl 2007-02-16 11:58:32 EST

Created attachment 59165 [details]
small fix after talking to Boris

the two mentionned interfaces are not support when using the new label support because they can be worked around easily by providing ones own CellLabelProvider e.g. derived from ColumnLabelProvider and overloading buildLabel and using ViewerRow#getTreePath()

Comment 11 Dani Megert 2007-02-20 09:46:06 EST

Which of the 4 attached patches is blessed by the UI team to make M5 work? Bug 174701 is marked as blocking this one - does it mean I also need the patches that are attached there. Please advice what someone who wants to patch M5 has to do - or even better: provide a patched M5 JFace plug-in.

Comment 12 Thomas Schindl 2007-02-20 09:50:46 EST

sorry but Mylar didn't let me to mark patches obsolet. You only need the patch from here to make it work. To get this patch in to CVS we need to PMC approval. I also removed the dependency because #174701 is not a must before we fix this.

Comment 13 Dani Megert 2007-02-20 09:57:17 EST

Thanks Tom.

Comment 14 Andre Weinand 2007-02-21 18:18:44 EST

Please consider to include this fix in a M5 rebuild.
Thanks

Comment 15 Boris Bokowski 2007-02-21 18:42:44 EST

The fix that Tom has come up with changes a lot of files.  I am uncomfortable with slipping that into an M5a without a milestone test pass.  I was under the impression that Dani had created a replacement jar file for you as a temporary solution until M6.  Is this not good enough?

Comment 16 Boris Bokowski 2007-02-21 22:11:52 EST

Created attachment 59535 [details]
patch for M5a

Single-file patch for 3.3M5a.

Comment 17 Boris Bokowski 2007-02-21 22:14:07 EST

(In reply to comment #16)
> Created an attachment (id=59535) [details]
> patch for M5a
> 
> Single-file patch for 3.3M5a.

Tom, Andre, Tod, could you please review the patch?

Comment 18 Thomas Schindl 2007-02-22 03:14:29 EST

There a big difference between what you are doing in update and what my code was doing. I use 

----------------8<----------------
if() {
//ITreePathLabelProvider
}

if() {
// IViewerLabelProvider
}

super.update()
----------------8<----------------

Your code is:
----------------8<----------------
if() {
// IViewerLabelProvider
} else if() {
// ITreePathLabelProvider
} else {
super.update();
}
----------------8<----------------

If you now look at buildLabel() the order of calls is:

TreeViewer#buildLabel(): 
1. Checks for ITreePathLabelProvider
2. calls super

StructuredViewer#buildLabel():
1. Checks for IViewerLabelProvider
2. Checks for ILabelProvider

Comment 19 Thomas Schindl 2007-02-22 03:47:14 EST

By the way Boris you only fix the providers using WrappedLabelProvider but not the TableColumnViewerLabelProvider. Although it might be unlikely that one implements the IViewerLabelProvider interface together with ITable*-Interfaces this can happen.

Comment 20 Thomas Schindl 2007-02-22 04:40:11 EST

Created attachment 59551 [details]
patch for M5a

corrects bugs from Boris patch (e.g. foreground tests didn't pass) and provides implementation for TableColumnViewerLabelProvider

Comment 21 Boris Bokowski 2007-02-22 05:26:00 EST

(In reply to comment #18)
> There a big difference between what you are doing in update and what my code
> was doing. 

Yes.

I did it this way to bring back the 3.2 behaviour: if a label provider implemented IViewerLabelProvider, *only* its #updateLabel method was called, not its #get(Text|Image|...) methods.

Still waiting for a response from Andre.

Comment 22 Thomas Schindl 2007-02-22 06:12:25 EST

After discussing with Boris and browsing 3.2 code we came to the conclusion: That Boris' patch is the right one and I'm the one to blame for making noise for nothing. The problem in Boris' patch was that he had a c&p error when setting the foreground the rest is OK.

Comment 23 Boris Bokowski 2007-02-22 06:18:13 EST

Created attachment 59557 [details]
patch without the background/foreground error

I would recommend using this patch going forward, or for anyone needing to run a modified M5.

Comment 24 Thomas Schindl 2007-02-22 11:00:28 EST

Created attachment 59578 [details]
Patch incooperating ViewerRow#getTreePath() into Boris M5a solution

Comment 25 Mike Wilson 2007-03-05 14:15:39 EST

A couple of questions about patch 59578:

1) I'm confused by this...
+	public TreePath getTreePath() {
+		ArrayList path = new ArrayList();
+		path.add(item.getData());
+		
+		TreeItem tItem = item;
+		LinkedList segments = new LinkedList();
+		while (tItem != null) {
+			Object segment = tItem.getData();
+			Assert.isNotNull(segment);
+			segments.addFirst(segment);
+			tItem = tItem.getParentItem();
+		}
+		
+		return new TreePath(segments.toArray());
+	}
... Specifically, the variable "path" doesn't appear to do anything useful.

2) "getTreePath" for tables is set to return null because they don't support paths. Would it be better to think of tables as "trees with nothing but roots"?

Comment 26 Thomas Schindl 2007-03-05 15:09:25 EST

Created attachment 60281 [details]
Revised patch

ad 1. you are right this is a left over from former days
ad 2. I've never thought about it this way but I like the idea of not return null
      and updated the patch in this way

Comment 27 Boris Bokowski 2007-03-12 16:25:44 EDT

The updated patch looks good to me.

Comment 28 Mike Wilson 2007-03-13 09:03:49 EDT

+1

Comment 29 Thomas Schindl 2007-03-13 10:01:39 EDT

Released > 20070313

Comment 30 Thomas Schindl 2007-03-23 03:50:27 EDT

Verified by code inspection in I20070321-1800

Comment 31 Nicolas Bros 2010-10-22 04:30:06 EDT

When I try to give a TreeViewer an IViewerLabelProvider as label provider, I get an AssertionFailedException.

setLabelProvider doesn't seem to accept IViewerLabelProvider :

public void setLabelProvider(IBaseLabelProvider labelProvider) {
	Assert.isTrue(labelProvider instanceof ITableLabelProvider
			|| labelProvider instanceof ILabelProvider
			|| labelProvider instanceof CellLabelProvider);
   ...
}

Is this a regression?
I'm using Helios (I20100608-0911)

Comment 32 Dani Megert 2010-10-22 04:39:21 EDT

Nicolas, this bug got fixed in 3.3. If you experience a problem in Helios then please open a new bug report.