Bugzilla – Full Text Bug Listing
| Summary: | [Viewers] JFace Table and Tree Viewers need to handle SWT events in an extensible way | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Tod Creasey <Tod_Creasey> | ||||||||||||||||||||||||||||||
| Component: | UI | Assignee: | Tod Creasey <Tod_Creasey> | ||||||||||||||||||||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||||||||||||||||||||
| Severity: | normal | ||||||||||||||||||||||||||||||||
| Priority: | P3 | CC: | bokowski, bpasero, bradleyjames, cgross, daniel_megert, eclipse, elias, gunnar, markus.kell.r, mlists, n.a.edgar, rohit_1004, thatnitind, tom.schindl, villane | ||||||||||||||||||||||||||||||
| Version: | 3.2 | ||||||||||||||||||||||||||||||||
| Target Milestone: | 3.3 M2 | ||||||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||||||
| OS: | Windows XP | ||||||||||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||||
| Bug Blocks: | 70800, 83200, 113713, 142655, 150618, 151377, 154123 | ||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||
|
Description
Tod Creasey
That's a good idea because when reading mailling-list people simply have no idea about all those optional interfaces ;-) I think it's time to clean up more JFace code there are so many duplications between TreeViewer/TableViewer most of them because we need to cast to TableItem/TreeItem in those classes but we use the same methods getBounds(), ... . I tried in my last patches (#83200,#142655) to provide classes which can be used by both controls. I think we also need a concept how to deal with this. JFace code would get much less complicated if there would exists ITableTreeItem holding the interface definition both widgets share but because I think we SWT will never provide this interface we should think about an intelligent wrapper. Another thing we need to correct is to make features configurable from the outside look at #142655 (configure from outside how tabbing works) and #87733 (configure activation for CellEditors) Tom we might be able to talk the SWT guys into a common superclass if we can come up with a good name. Any suggestions? Let me have a start on this and I'll post some code soon - I just want to be sure my ideas aren't half baked <grin>... (In reply to comment #2) > Tom we might be able to talk the SWT guys into a common superclass if we can > come up with a good name. Any suggestions? > Well not at the moment but I'll think about a good name and let you know. > Let me have a start on this and I'll post some code soon - I just want to be > sure my ideas aren't half baked <grin>... > Tell me if you need any input from me you besides this bug #142655 addresses JFace-Viewers also in a global context maybe we should sync those efforts. (In reply to comment #2) > Tom we might be able to talk the SWT guys into a common superclass if we can > come up with a good name. Any suggestions? > > Let me have a start on this and I'll post some code soon - I just want to be > sure my ideas aren't half baked <grin>... > What's with one of these: - IColumnItem - ITableItem (because used in TreeTables) The reasons why this would be needed: a) Reduce amount of code duplication in JFace (see TableEditorImpl,...) b) When implementing Viewers for Components outside of JFace e.g. Nebula's grid-control (see #146945) JFace does have to have a dependency to this project without that the Viewer needs to live in the nebula project which is in my opion not the right place for it. Is it TableTree you are talking about? We are likely not going to do anything interesting as it a deprecated Viewer. But Boris was also keen on the concept of a column. I assume you are suggesting that we have a abstract class like JFaceItem with a subclass of ColumnItem and JFaceItem? (In reply to comment #5) > Is it TableTree you are talking about? We are likely not going to do anything > interesting as it a deprecated Viewer. > Well not exactly I think about having TableItem and TreeItem, GridItem (nebula-grid), ... implementing an interface holding the methods both have in common. In those cases it's always the same story that the item is split into "Columns" hence methods like item.getBounds(i) that's why I named the interface IColumnItem. This would make our/your job easier when dealing with Column-Widget like Table is and Tree is too. What I have already thought about is wrapping SWT-Columns like shown in bug #142655 (https://bugs.eclipse.org/bugs/attachment.cgi?id=42561) see - AbstractEditingColumn - AbstractEditableTableColumn - AbstractEditableTreeColumn Which addresses many many things we are talking about and people requested in the past. > But Boris was also keen on the concept of a column. > > I assume you are suggesting that we have a abstract class like > > JFaceItem > > with a subclass of ColumnItem and JFaceItem? > I not sure I can follow you here or rather I think you got me wrong. How do you think this would work technically the items are provided by the Tree/Table/Grid/.... Do you want to wrap them when request by any Viewer-method instead of returning the original Items? That's not what I had in mind. OK I think I might see where you are going with this - let me elaborate on what I was thinking about. Basically we have two types of interfaces for viewers - those for a whole row and those for inidividual entries (i.e. IColorProvider vs. ITableColorProvider). These are effectively both IColorProviders - the one for the table just has the column as a parameter. If instead ITableColorProvider was an IColorProvider applied to a column (i.e. we would have a method setColorProvider(IColorProvider provider, int columnIndex) that would reduce the need for parallel interfaces all of the time. We could then express everything in terms of the base interfaces without a need for table and tree versions of them that take an index. When we add a new one (like the tooltip support)we create one provider and apply it to either the whole table or tree or just one column. Internally we might create the idea of StructuredViewerColumn with subclasses of TableViewerColumn and TreeViewerColumn to implement this but I will have to play around to see what I think we need. (In reply to comment #7) > Internally we might create the idea of StructuredViewerColumn with subclasses > of TableViewerColumn and TreeViewerColumn to implement this but I will have to > play around to see what I think we need. I would like to see an abstraction like this in the API so that clients can call viewerColumn.setLabelProvider(...) and similar methods for cell editors, cell modifier etc. instead of the index-based calls. (In reply to comment #8) > (In reply to comment #7) > > Internally we might create the idea of StructuredViewerColumn with subclasses > > of TableViewerColumn and TreeViewerColumn to implement this but I will have to > > play around to see what I think we need. > > I would like to see an abstraction like this in the API so that clients can > call viewerColumn.setLabelProvider(...) and similar methods for cell editors, > cell modifier etc. instead of the index-based calls. > Well I'd go one step further we let the user create the ViewerColumn instead of create the TableColumn/TreeColumn at the moment. This would give us the possibility to remove all those ITableFontProvider, ITableColorProvider and replace them through methods in our ViewerColumn (getForegroundColor(),getFont(),getTooltip(),...). By default it would return null and users could overwrite it to get custom behaviour. As already said I've started this for CellEditors in my above mentionned concept for Viewers. In short at the moment we add the concept of Columns we don't need any LabelProvider, ICellModifier, ... stuff any more. My point moreover is that we only need ViewerColumn if we could convince SWT folks to provide interfaces for TableColumn/TreeColumn and TableItem/TreeItem. This is the direction that I am thinking as well - I would like to deprecate ITableXProvider at the end of this work. ColumnViewer should just be another viewer. ColumnViewer would have a TreeColumn or TableColumn as a widget just like a Viewer has a Table or a Tree. It is a bit of a departure as we have nowhere else where a Viewer has other Viewers in it but I think this fits our ViewerModel nicely. By making the column a first class viewer like the Table is we can implement all of the functionality you are after. Thanks for your input Tom - I think we are going in a good direction here. (Just noticed this bug from the planning notes)
As the owner of the Nebula Grid, if theres some possibility to allow Grid to
reuse the standard JFace table/tree viewers that would make me extremely happy.
I (and others) have taken to copying/modify the viewer code to support Grid
and its been difficult because of the access levels for the classes.
Rather than requiring SWT to create a new interface shared between Tables/Trees
and potentially Grid, a proxy interface could be created in JFace. The viewers would be refactored to act against the interface, but also
JFace would include implementations of the interface that wrapped existing
Table and Tree. This would allow the existing API constructor to remain the
same. The constructor code would look something like:
public TableViewer(Table table)
{
this(new TableProxy(table));
}
public TableViewer(ITableProxy table)
{
//...
}
But thats only if the SWT guys decide against a common interface.
I realize the focus of this bug report isn't this kind of change, but it would
be much appreciated.
I have been playing around with a ColumnViewer and it is not as simple as I thought at first. First of all let me ask you what you expect in a grid. The current implementation of tables and trees assumes that the same item is used for all of the columns in a particualr row. Are you looking ofr a viewer where every element could be different (i.e. two sequential cells would have different elements behind them)? If so then I am not sure if we are looking to do quite so radical a change to how a row of a tree or table is represented. Secondly my original idea of ColumnViewers does not make sense with the current implementation. If each row represents an element the Column can't be a viewer as we would be hacking it pretty hard to make it fit the viewer API. Created attachment 46241 [details]
First pass of the new API
Just so that you can see where I am going with this here is a patch that shows what I am suggesting.
TableColumnViewerPart (I am not attached to the name so suggestions are welcome) is the JFace representation of the TableColumn. The whole provider mechanism is simplified into the ViewerLabelProvider which can be subclassed for anyone who want to do anything custom.
TableColumnViewerPart (which can also be subclassed) is responsible for supplying the TableItem and the element when refresh is required.It asks ViewerLabelProvider to build the label and then updates the TableItem when the label is built.
I've taken a look at it and form my point of view it looks good but as already said I'd more like to see that instead of creating TableColumn/TreeColumn from the outside I'd like to have a see another constructor TableColumnViewerPart(TableViewer,ViewerLabelProvider), I would not give the user the possibility any more to create a TableColumn simply by deprecating getTable()/getTree(). What do you for example do if the user access the getTable().setColumnOrder()? I don't think that you could recognize this, can you? My dream would be do hide the underlying SWT-Control completely from the user. This way the user wouldn't event recognize if we would replace the underlying Table/Tree through e.g. the Grid-Control because of what ever reason (although we won't do that ever). Another great thing would be if we could merge my replacement ICellModifier,... from bug 142655 into it. (In reply to comment #13) > Created an attachment (id=46241) [edit] > First pass of the new API > > Just so that you can see where I am going with this here is a patch that shows > what I am suggesting. > > TableColumnViewerPart (I am not attached to the name so suggestions are > welcome) is the JFace representation of the TableColumn. The whole provider > mechanism is simplified into the ViewerLabelProvider which can be subclassed > for anyone who want to do anything custom. > > TableColumnViewerPart (which can also be subclassed) is responsible for > supplying the TableItem and the element when refresh is required.It asks > ViewerLabelProvider to build the label and then updates the TableItem when the > label is built. > Created attachment 46254 [details]
New patch
Here is a patch based on what I have done since the first one today (with a lot of tidy up).
I like your idea of the constructor - I did a similar thing in TableViewer - setColumnPart(ColumnViewerPart viewerPart, int columnIndex). ColumnViewerPart has no controls associated with it.
I am reluctant to hide the SWT widget as I would like this to be extensible and so if there something you want to do that JFace does not permit you would be out of luck if we didn't give it access. Many of the people interested in this have expressed that having the widget is a requirement.
We of course want to have your work as part of the final solution for this Tom - I just want to have the groundwork done before the feature work.
Thanks again for all of your input
Created attachment 46263 [details]
Patch that passes the test suites
Here is a version of the previous patch that does not break the JFace suites
Maybe we should also add the concept of Cells into Tables/Trees not provided my SWT. This came to my mind creating the patch for bug 151377 (In reply to comment #17) > Maybe we should also add the concept of Cells into Tables/Trees not provided my > SWT. This came to my mind creating the patch for bug 151377 > The new listener added by this bug-report can be used to refactor TableEditorImpl/TreeEditorImpl who extract the column themselfs at the moment. Created attachment 46677 [details]
Corrects problems with column-index
Today I had the chance to take a closer look at your implementation and found some major problems with your approach of fixing the column-index. I think this has to stay be dynamic because if you add/remove columns you may have problems with multiple columns having the same index.
I also added a new class named TableColumnViewerPart which can be used to created new TableColumns, ...
(In reply to comment #19) > Created an attachment (id=46677) [edit] > Corrects problems with column-index > > Today I had the chance to take a closer look at your implementation and found > some major problems with your approach of fixing the column-index. I think this > has to stay be dynamic because if you add/remove columns you may have problems > with multiple columns having the same index. > > I also added a new class named TableColumnViewerPart which can be used to > created new TableColumns, ... > Another problem just came to my mind is the way you call hasNewBackground(), ... you can not reset a background to the default color if you have set it once or am I wrong here? Created attachment 46686 [details]
Revised patch
This patch does the following changes to the one Tod:
- Add new WrapperClasses for TreeColumn and TableColumn
- Renames TreeViewerPart to TreeColumnViewerPart
- Moves ColumViewerPart <=> ColumnOwner relationship to ColumnViewerPart's contructor
- Add editing support to ColumViewPart and therefore refactors TableEditor/TreeEditorImpl
FYI - I've submitted bug 151644 seperately to investigate the possibility of a shared interface between Table and Tree. To whom it may interest today I had time and thought I it give a try and started a TableViewer from scratch. My interest was not to provide anything that is backward compatible but rather try out some ideas I had in mind. The ideas I used came from Tod's code and some of the existing feature requests. Interesting ideas: - Tod's ViewerPart - Wrapper for TableItem (should give possiblity to exchange Table) named RowPart maybe can be addressed from the jface side bug 151644 whether or not this request is fullfilled or not doesn't harm this implementation and could be extented by every Grid/TableProvider - Concept of Cells(CellPart) in conjunction with TableCursor At the moment the following bugs can be addressed with this viewer: - bug 151377: [Viewers] Listeners for CellSelections - bug 151205: [Viewers] More efficient addFilter-Method - bug 83200: [Viewers] Support to define custom tooltips for elements in viewers - bug 143789: [EditorMgmt] Add possibility to customize table-editor activation - bug 151295: [CellEditors] table viewer needs to provide a key combination to activate the celleditors - bug 87733: [CellEditors] Double click behavior for tableviewer - bug 142655: [Viewers] Rethink Viewer Concept of ICellModifier, columnProperties, CellEditor - bug 149193: [Viewers] JFace Table and Tree Viewers need to handle SWT events in an extensible way - bug 151644: to some extend that maybe the interfaces are not needed The concept of Custom-Tooltips also got many new features: - useNativeTooltip - getTooltipTimeDisplayed - getTooltipDisplayDelayTime - getTooltipImage - getTooltipFont The most outstandings things are at the moment: - Tab-Editing support to jump from CellEditor to CellEditor - Support for DecorationLabelProvider Ideas, suggestions, ... are welcome. I wanted to post this code to show you my way of thinking some parts of them can easily backported e.g. Tooltip things, ... . (In reply to comment #22) > FYI - I've submitted bug 151644 seperately to investigate the possibility of a > shared interface between Table and Tree. > Created attachment 47008 [details]
Test implementation
Tod/Boris is there anything new about what you guys think about the current implementation provided by Tod. I'm going to have time this week and if this is the way to go for future of Viewers I'd incooperate all my work into this. What I'd like to see merged in: - handling of editing by ColumnViewerPart - intergration of Tooltips like in my Test-Impl - KeyBoard-Navigation (we need to discuss how this can be done, I'm not sure the impl I've tried to use in my Test-Impl) - possiblity to Track-Clicks on cell "addCellSelectionListener" Sorry Tom - both of us were on vacation for the last two weeks so I haven't had a chance to look over anything yet. I'll try to get to it this week. I can see the sense in dumping the column indices - we don't keep them in item mappings either so this is a consistent implementation. I think Toms patch is on the right track so I am happy to keep working from here. I ran our suites and it doesn't appear to break anything. I of course can't speak for Boris - he will be back on Tuesday IIRC. The current patch has this exception when you import a project from a plug-in !ENTRY org.eclipse.jface 4 2 2006-08-03 12:05:39.671 !MESSAGE Problems occurred when invoking code from plug-in: "org.eclipse.jface". !STACK 0 java.lang.NullPointerException at org.eclipse.jface.viewers.ViewerLabelProvider.getFont(ViewerLabelProvider.java:143) at org.eclipse.jface.viewers.ViewerLabelProvider.updateLabel(ViewerLabelProvider.java:78) at org.eclipse.jface.viewers.ViewerLabelProvider.updateLabel(ViewerLabelProvider.java:90) at org.eclipse.jface.viewers.ColumnViewerPart.refresh(ColumnViewerPart.java:87) at org.eclipse.jface.viewers.TreeViewer.doUpdateItem(TreeViewer.java:156) at org.eclipse.jface.viewers.AbstractTreeViewer$UpdateItemSafeRunnable.run(AbstractTreeViewer.java:95) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:37) at org.eclipse.core.runtime.Platform.run(Platform.java:843) at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:44) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:149) at org.eclipse.jface.viewers.AbstractTreeViewer.doUpdateItem(AbstractTreeViewer.java:849) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.doUpdateItem(ProblemTreeViewer.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.updateItem(ResourceToItemsMapper.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.resourceChanged(ResourceToItemsMapper.java:63) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.handleLabelProviderChanged(ProblemTreeViewer.java:127) at org.eclipse.jface.viewers.ContentViewer$1.labelProviderChanged(ContentViewer.java:74) at org.eclipse.jdt.ui.ProblemsLabelDecorator.fireProblemsChanged(ProblemsLabelDecorator.java:380) at org.eclipse.jdt.ui.ProblemsLabelDecorator.access$0(ProblemsLabelDecorator.java:375) at org.eclipse.jdt.ui.ProblemsLabelDecorator$1.problemsChanged(ProblemsLabelDecorator.java:355) at org.eclipse.jdt.internal.ui.viewsupport.ProblemMarkerManager$1.run(ProblemMarkerManager.java:177) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:123) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3354) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3000) at org.eclipse.jface.window.Window.runEventLoop(Window.java:820) at org.eclipse.jface.window.Window.open(Window.java:796) at org.eclipse.jface.dialogs.MessageDialog.openError(MessageDialog.java:322) at org.eclipse.jface.util.SafeRunnable.handleException(SafeRunnable.java:60) at org.eclipse.core.runtime.SafeRunner.handleException(SafeRunner.java:68) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:39) at org.eclipse.core.runtime.Platform.run(Platform.java:843) at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:44) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:149) at org.eclipse.jface.viewers.AbstractTreeViewer.doUpdateItem(AbstractTreeViewer.java:849) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.doUpdateItem(ProblemTreeViewer.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.updateItem(ResourceToItemsMapper.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.resourceChanged(ResourceToItemsMapper.java:63) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.handleLabelProviderChanged(ProblemTreeViewer.java:127) at org.eclipse.jface.viewers.ContentViewer$1.labelProviderChanged(ContentViewer.java:74) at org.eclipse.jdt.ui.ProblemsLabelDecorator.fireProblemsChanged(ProblemsLabelDecorator.java:380) at org.eclipse.jdt.ui.ProblemsLabelDecorator.access$0(ProblemsLabelDecorator.java:375) at org.eclipse.jdt.ui.ProblemsLabelDecorator$1.problemsChanged(ProblemsLabelDecorator.java:355) at org.eclipse.jdt.internal.ui.viewsupport.ProblemMarkerManager$1.run(ProblemMarkerManager.java:177) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:123) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3354) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3000) at org.eclipse.jface.window.Window.runEventLoop(Window.java:820) at org.eclipse.jface.window.Window.open(Window.java:796) at org.eclipse.jface.dialogs.MessageDialog.openError(MessageDialog.java:322) at org.eclipse.jface.util.SafeRunnable.handleException(SafeRunnable.java:60) at org.eclipse.core.runtime.SafeRunner.handleException(SafeRunner.java:68) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:39) at org.eclipse.core.runtime.Platform.run(Platform.java:843) at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:44) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:149) at org.eclipse.jface.viewers.AbstractTreeViewer.doUpdateItem(AbstractTreeViewer.java:849) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.doUpdateItem(ProblemTreeViewer.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.updateItem(ResourceToItemsMapper.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.resourceChanged(ResourceToItemsMapper.java:63) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.handleLabelProviderChanged(ProblemTreeViewer.java:127) at org.eclipse.jface.viewers.ContentViewer$1.labelProviderChanged(ContentViewer.java:74) at org.eclipse.jdt.ui.ProblemsLabelDecorator.fireProblemsChanged(ProblemsLabelDecorator.java:380) at org.eclipse.jdt.ui.ProblemsLabelDecorator.access$0(ProblemsLabelDecorator.java:375) at org.eclipse.jdt.ui.ProblemsLabelDecorator$1.problemsChanged(ProblemsLabelDecorator.java:355) at org.eclipse.jdt.internal.ui.viewsupport.ProblemMarkerManager$1.run(ProblemMarkerManager.java:177) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:123) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3354) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3000) at org.eclipse.jface.window.Window.runEventLoop(Window.java:820) at org.eclipse.jface.window.Window.open(Window.java:796) at org.eclipse.jface.dialogs.MessageDialog.openError(MessageDialog.java:322) at org.eclipse.jface.util.SafeRunnable.handleException(SafeRunnable.java:60) at org.eclipse.core.runtime.SafeRunner.handleException(SafeRunner.java:68) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:39) at org.eclipse.core.runtime.Platform.run(Platform.java:843) at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:44) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:149) at org.eclipse.jface.viewers.AbstractTreeViewer.doUpdateItem(AbstractTreeViewer.java:849) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.doUpdateItem(ProblemTreeViewer.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.updateItem(ResourceToItemsMapper.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.resourceChanged(ResourceToItemsMapper.java:63) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.handleLabelProviderChanged(ProblemTreeViewer.java:127) at org.eclipse.jface.viewers.ContentViewer$1.labelProviderChanged(ContentViewer.java:74) at org.eclipse.jdt.ui.ProblemsLabelDecorator.fireProblemsChanged(ProblemsLabelDecorator.java:380) at org.eclipse.jdt.ui.ProblemsLabelDecorator.access$0(ProblemsLabelDecorator.java:375) at org.eclipse.jdt.ui.ProblemsLabelDecorator$1.problemsChanged(ProblemsLabelDecorator.java:355) at org.eclipse.jdt.internal.ui.viewsupport.ProblemMarkerManager$1.run(ProblemMarkerManager.java:177) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:123) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3354) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3000) at org.eclipse.jface.window.Window.runEventLoop(Window.java:820) at org.eclipse.jface.window.Window.open(Window.java:796) at org.eclipse.jface.dialogs.MessageDialog.openError(MessageDialog.java:322) at org.eclipse.jface.util.SafeRunnable.handleException(SafeRunnable.java:60) at org.eclipse.core.runtime.SafeRunner.handleException(SafeRunner.java:68) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:39) at org.eclipse.core.runtime.Platform.run(Platform.java:843) at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:44) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:149) at org.eclipse.jface.viewers.AbstractTreeViewer.doUpdateItem(AbstractTreeViewer.java:849) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.doUpdateItem(ProblemTreeViewer.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.updateItem(ResourceToItemsMapper.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.resourceChanged(ResourceToItemsMapper.java:63) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.handleLabelProviderChanged(ProblemTreeViewer.java:127) at org.eclipse.jface.viewers.ContentViewer$1.labelProviderChanged(ContentViewer.java:74) at org.eclipse.jdt.ui.ProblemsLabelDecorator.fireProblemsChanged(ProblemsLabelDecorator.java:380) at org.eclipse.jdt.ui.ProblemsLabelDecorator.access$0(ProblemsLabelDecorator.java:375) at org.eclipse.jdt.ui.ProblemsLabelDecorator$1.problemsChanged(ProblemsLabelDecorator.java:355) at org.eclipse.jdt.internal.ui.viewsupport.ProblemMarkerManager$1.run(ProblemMarkerManager.java:177) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:123) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3354) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3000) at org.eclipse.jface.window.Window.runEventLoop(Window.java:820) at org.eclipse.jface.window.Window.open(Window.java:796) at org.eclipse.jface.dialogs.MessageDialog.openError(MessageDialog.java:322) at org.eclipse.jface.util.SafeRunnable.handleException(SafeRunnable.java:60) at org.eclipse.core.runtime.SafeRunner.handleException(SafeRunner.java:68) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:39) at org.eclipse.core.runtime.Platform.run(Platform.java:843) at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:44) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:149) at org.eclipse.jface.viewers.AbstractTreeViewer.doUpdateItem(AbstractTreeViewer.java:849) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.doUpdateItem(ProblemTreeViewer.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.updateItem(ResourceToItemsMapper.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.resourceChanged(ResourceToItemsMapper.java:63) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.handleLabelProviderChanged(ProblemTreeViewer.java:127) at org.eclipse.jface.viewers.ContentViewer$1.labelProviderChanged(ContentViewer.java:74) at org.eclipse.jdt.ui.ProblemsLabelDecorator.fireProblemsChanged(ProblemsLabelDecorator.java:380) at org.eclipse.jdt.ui.ProblemsLabelDecorator.access$0(ProblemsLabelDecorator.java:375) at org.eclipse.jdt.ui.ProblemsLabelDecorator$1.problemsChanged(ProblemsLabelDecorator.java:355) at org.eclipse.jdt.internal.ui.viewsupport.ProblemMarkerManager$1.run(ProblemMarkerManager.java:177) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:123) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3354) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3000) at org.eclipse.jface.window.Window.runEventLoop(Window.java:820) at org.eclipse.jface.window.Window.open(Window.java:796) at org.eclipse.jface.dialogs.MessageDialog.openError(MessageDialog.java:322) at org.eclipse.jface.util.SafeRunnable.handleException(SafeRunnable.java:60) at org.eclipse.core.runtime.SafeRunner.handleException(SafeRunner.java:68) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:39) at org.eclipse.core.runtime.Platform.run(Platform.java:843) at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:44) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:149) at org.eclipse.jface.viewers.AbstractTreeViewer.doUpdateItem(AbstractTreeViewer.java:849) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.doUpdateItem(ProblemTreeViewer.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.updateItem(ResourceToItemsMapper.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.resourceChanged(ResourceToItemsMapper.java:63) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.handleLabelProviderChanged(ProblemTreeViewer.java:127) at org.eclipse.jface.viewers.ContentViewer$1.labelProviderChanged(ContentViewer.java:74) at org.eclipse.jdt.ui.ProblemsLabelDecorator.fireProblemsChanged(ProblemsLabelDecorator.java:380) at org.eclipse.jdt.ui.ProblemsLabelDecorator.access$0(ProblemsLabelDecorator.java:375) at org.eclipse.jdt.ui.ProblemsLabelDecorator$1.problemsChanged(ProblemsLabelDecorator.java:355) at org.eclipse.jdt.internal.ui.viewsupport.ProblemMarkerManager$1.run(ProblemMarkerManager.java:177) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:123) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3354) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3000) at org.eclipse.jface.window.Window.runEventLoop(Window.java:820) at org.eclipse.jface.window.Window.open(Window.java:796) at org.eclipse.jface.dialogs.MessageDialog.openError(MessageDialog.java:322) at org.eclipse.jface.util.SafeRunnable.handleException(SafeRunnable.java:60) at org.eclipse.core.runtime.SafeRunner.handleException(SafeRunner.java:68) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:39) at org.eclipse.core.runtime.Platform.run(Platform.java:843) at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:44) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:149) at org.eclipse.jface.viewers.AbstractTreeViewer.doUpdateItem(AbstractTreeViewer.java:849) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.doUpdateItem(ProblemTreeViewer.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.updateItem(ResourceToItemsMapper.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.resourceChanged(ResourceToItemsMapper.java:63) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.handleLabelProviderChanged(ProblemTreeViewer.java:127) at org.eclipse.jface.viewers.ContentViewer$1.labelProviderChanged(ContentViewer.java:74) at org.eclipse.jdt.ui.ProblemsLabelDecorator.fireProblemsChanged(ProblemsLabelDecorator.java:380) at org.eclipse.jdt.ui.ProblemsLabelDecorator.access$0(ProblemsLabelDecorator.java:375) at org.eclipse.jdt.ui.ProblemsLabelDecorator$1.problemsChanged(ProblemsLabelDecorator.java:355) at org.eclipse.jdt.internal.ui.viewsupport.ProblemMarkerManager$1.run(ProblemMarkerManager.java:177) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:123) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3354) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3000) at org.eclipse.jface.window.Window.runEventLoop(Window.java:820) at org.eclipse.jface.window.Window.open(Window.java:796) at org.eclipse.jface.dialogs.MessageDialog.openError(MessageDialog.java:322) at org.eclipse.jface.util.SafeRunnable.handleException(SafeRunnable.java:60) at org.eclipse.core.runtime.SafeRunner.handleException(SafeRunner.java:68) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:39) at org.eclipse.core.runtime.Platform.run(Platform.java:843) at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:44) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:149) at org.eclipse.jface.viewers.AbstractTreeViewer.doUpdateItem(AbstractTreeViewer.java:849) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.doUpdateItem(ProblemTreeViewer.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.updateItem(ResourceToItemsMapper.java:74) at org.eclipse.jdt.internal.ui.viewsupport.ResourceToItemsMapper.resourceChanged(ResourceToItemsMapper.java:63) at org.eclipse.jdt.internal.ui.viewsupport.ProblemTreeViewer.handleLabelProviderChanged(ProblemTreeViewer.java:127) at org.eclipse.jface.viewers.ContentViewer$1.labelProviderChanged(ContentViewer.java:74) at org.eclipse.jdt.ui.ProblemsLabelDecorator.fireProblemsChanged(ProblemsLabelDecorator.java:380) at org.eclipse.jdt.ui.ProblemsLabelDecorator.access$0(ProblemsLabelDecorator.java:375) at org.eclipse.jdt.ui.ProblemsLabelDecorator$1.problemsChanged(ProblemsLabelDecorator.java:355) at org.eclipse.jdt.internal.ui.viewsupport.ProblemMarkerManager$1.run(ProblemMarkerManager.java:177) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:123) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3354) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3000) at org.eclipse.jface.window.Window.runEventLoop(Window.java:820) at org.eclipse.jface.window.Window.open(Window.java:796) at org.eclipse.jface.dialogs.MessageDialog.openError(MessageDialog.java:322) at org.eclipse.jface.util.SafeRunnable.handleException(SafeRunnable.java:60) at org.eclipse.core.runtime.SafeRunner.handleException(SafeRunner.java:68) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:39) at org.eclipse.core.runtime.Platform.run(Platform.java:843) at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:44) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:149) at org.eclipse.jface.viewers.AbstractTreeViewer.doUpdateItem(AbstractTreeViewer.java:849) at org.eclipse.jface.viewers.StructuredViewer$UpdateItemSafeRunnable.run(StructuredViewer.java:465) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:37) at org.eclipse.core.runtime.Platform.run(Platform.java:843) at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:44) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:149) at org.eclipse.jface.viewers.StructuredViewer.updateItem(StructuredViewer.java:1955) at org.eclipse.jface.viewers.AbstractTreeViewer.createTreeItem(AbstractTreeViewer.java:753) at org.eclipse.jface.viewers.AbstractTreeViewer.createAddedElements(AbstractTreeViewer.java:313) at org.eclipse.jface.viewers.AbstractTreeViewer.internalAdd(AbstractTreeViewer.java:257) at org.eclipse.jface.viewers.TreeViewer.internalAdd(TreeViewer.java:810) at org.eclipse.jface.viewers.AbstractTreeViewer.add(AbstractTreeViewer.java:136) at org.eclipse.jdt.internal.ui.packageview.PackageExplorerPart$PackageExplorerProblemTreeViewer.add(PackageExplorerPart.java:260) at org.eclipse.jface.viewers.AbstractTreeViewer.add(AbstractTreeViewer.java:602) at org.eclipse.jdt.internal.ui.packageview.PackageExplorerContentProvider$4.run(PackageExplorerContentProvider.java:629) at org.eclipse.jdt.internal.ui.packageview.PackageExplorerContentProvider$7.run(PackageExplorerContentProvider.java:666) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:123) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3354) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3000) at org.eclipse.jface.operation.ModalContext$ModalContextThread.block(ModalContext.java:158) at org.eclipse.jface.operation.ModalContext.run(ModalContext.java:326) at org.eclipse.jface.wizard.WizardDialog.run(WizardDialog.java:854) at org.eclipse.pde.internal.ui.wizards.imports.PluginImportWizard.performFinish(PluginImportWizard.java:90) at org.eclipse.jface.wizard.WizardDialog.finishPressed(WizardDialog.java:683) at org.eclipse.jface.wizard.WizardDialog.buttonPressed(WizardDialog.java:355) at org.eclipse.jface.dialogs.Dialog$3.widgetSelected(Dialog.java:660) at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:90) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:66) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:928) at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:3377) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:2997) at org.eclipse.jface.window.Window.runEventLoop(Window.java:820) at org.eclipse.jface.window.Window.open(Window.java:796) at org.eclipse.ui.actions.ImportResourcesAction.run(ImportResourcesAction.java:159) at org.eclipse.ui.actions.BaseSelectionListenerAction.runWithEvent(BaseSelectionListenerAction.java:168) at org.eclipse.jface.action.ActionContributionItem.handleWidgetSelection(ActionContributionItem.java:539) at org.eclipse.jface.action.ActionContributionItem.access$2(ActionContributionItem.java:488) at org.eclipse.jface.action.ActionContributionItem$5.handleEvent(ActionContributionItem.java:400) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:66) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:928) at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:3377) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:2997) at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:1914) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:1878) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:419) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.IDEApplication.run(IDEApplication.java:95) at org.eclipse.core.internal.runtime.PlatformActivator$1.run(PlatformActivator.java:78) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:92) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:68) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:400) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:177) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:324) at org.eclipse.core.launcher.Main.invokeFramework(Main.java:336) at org.eclipse.core.launcher.Main.basicRun(Main.java:280) at org.eclipse.core.launcher.Main.run(Main.java:977) at org.eclipse.core.launcher.Main.main(Main.java:952) If you have sometime you can look at what I'm trying out by a completely custom TableViewer-Implementation from my SVN-Repository at: http://publicsvn.bestsolution.at/repos/java/at.bestsolution.jface.custom/trunk Created attachment 47350 [details]
Fix NLP
That's strange are we self the I*Provider? This seems to be the case because the line in ViewerLabelProvider.java:78 reads like this:
77 if (fontProvider != null) {
78 label.setFont(fontProvider.getFont(element));
79 }
Because ViewerLabelProvider implements all interfaces we mimic to have a IFontProvider and without any checks whether we have a IFontProvider, ... is set.
Checking against null before calling the viewer should cure the problem and isn't a bad idea in any case.
Could you give this patch a try or tell me exactly how I can provoke the NLP myself.
You can get this by loading a project from a CVS repository into an empty workspace (In reply to comment #22) > FYI - I've submitted bug 151644 seperately to investigate the possibility of a > shared interface between Table and Tree. > Now that this feature request was refused by the SWT-Team I think we should think about a Proxy mechanism provided by JFace to make it easy for other to use JFace-Viewers without providing too much custom code. What I think about is the following: public abstract class RowPart { public abstract getBounds(int columnIndex); // and some more } public class StructuredViewer { // Subclasses have to implement public RowPart createNewRowPart() { return null; } // Subclasses have to implement public RowPart createRowPartFromItem(Item item) { return(RowPart)item.getData("ROWDATA"); } } public class TableViewer extends StructuredViewer { public RowPart createNewRowPart() { return new TableRowPart(this,SWT.NONE); } public RowPart createRowPartFromItem(Item item) { RowPart part = super.createRowPartFromItem(Item item); if( part == null ) { part = new TableRowPart((TableItem)item); } return part; } } public class TableRowPart extends RowPart { private TableItem item; public TableRowPart(TableViewer viewer, int style) { this(new TableItem(viewer.getTable(),style)); } public TableRowPart(TableItem item) { this.item = item; // should we cache the RowPart // this.item.setData("ROWPART",this); } public Rectangle getBounds(int columnIndex) { return item.getBounds(columnIndex); } } Created attachment 47453 [details]
New Patch to add Tooltip-Support
This patch adds:
----------------
- Tooltip-Support
- Wrappers for TableItem/TreeItem => possibility for other Widget providers than
SWT to use Viewers with very little custom code
Because of the wrappers I was able to move some portions of the code from TableViewer/TreeViewer to StructuredViewer. I think this implementation gives us more flexiblity and we can start to reduce code in many other places of JFace-Viewers.
Created attachment 47454 [details]
Example script to show tooltips usage
One more thing I want to add is that I move some of the Interfaces (likely to change in future maybe) into internal.viewers and the only possiblity from for external users is to use public abstract classes: - ITooltipAddonsProvider => AbstractTooltipProvider - ITableTooltipAddonsProvider => AbstractTableTooltipProvider - RowPart => DefaultRowPartImpl For e.g. Nebula this would mean that they need to provide an implementation ontop of DefaultRowPartImpl instead of RowPart. Another action which needs to be done by e.g. Nebula is to subclass TableViewer/TreeViewer protected RowPart getRowPartFromItem(Widget item); protected RowPart createNewRowPart(RowPart parent, int style, int rowIndex); protected ColumnViewerPart createColumnViewer(Widget columnOwner, ViewerLabelProvider labelProvider); (In reply to comment #33) > Created an attachment (id=47453) [edit] > New Patch to add Tooltip-Support > > This patch adds: > ---------------- > - Tooltip-Support > - Wrappers for TableItem/TreeItem => possibility for other Widget providers > than > SWT to use Viewers with very little custom code > > Because of the wrappers I was able to move some portions of the code from > TableViewer/TreeViewer to StructuredViewer. I think this implementation gives > us more flexiblity and we can start to reduce code in many other places of > JFace-Viewers. > Oh one more thing to add. I think we need to add new abstract classes between StructuredViewer -> TableViewer StructuredViewer -> AbstractTreeViewer Who are able are not dependent on Table/Tree-Widget. What do you think is this worth the effort? Although I think 1st priority should be to finalize provider support and then go-on with Widget independency. Maybe another bug should be filed for this? (In reply to comment #35) > One more thing I want to add is that I move some of the Interfaces (likely to > change in future maybe) into internal.viewers and the only possiblity from for > external users is to use public abstract classes: > - ITooltipAddonsProvider => AbstractTooltipProvider > - ITableTooltipAddonsProvider => AbstractTableTooltipProvider > - RowPart => DefaultRowPartImpl > > For e.g. Nebula this would mean that they need to provide an implementation > ontop of DefaultRowPartImpl instead of RowPart. > > Another action which needs to be done by e.g. Nebula is to subclass > TableViewer/TreeViewer > > protected RowPart getRowPartFromItem(Widget item); > protected RowPart createNewRowPart(RowPart parent, int style, int rowIndex); > protected ColumnViewerPart createColumnViewer(Widget columnOwner, > ViewerLabelProvider labelProvider); > > > > (In reply to comment #33) > > Created an attachment (id=47453) [edit] > > New Patch to add Tooltip-Support > > > > This patch adds: > > ---------------- > > - Tooltip-Support > > - Wrappers for TableItem/TreeItem => possibility for other Widget providers > > than > > SWT to use Viewers with very little custom code > > > > Because of the wrappers I was able to move some portions of the code from > > TableViewer/TreeViewer to StructuredViewer. I think this implementation gives > > us more flexiblity and we can start to reduce code in many other places of > > JFace-Viewers. > > > Created attachment 47472 [details]
Fixing backwards compatility problems
By moving methods to StructuredViewer I introduced a NLP-Exception, the same is true for EditorImpls where those bugs are also fixed now
Tod one more question is there a reason why TreeColumnViewerLabelProvider extends ViewerLabelProvider and not TableColumnViewerLabelProvider? Because I wanted to keep the tree and table metaphor seperate so as not to confuse developers. I would it if as much of our API as possible was Tree/Table independant. Well that doesn't make sense because the TreeColumnViewerLabelProvider doesn't has the possiblity to work with ITable*-Interfaces it can at the moment. (In reply to comment #39) > Because I wanted to keep the tree and table metaphor seperate so as not to > confuse developers. I would it if as much of our API as possible was Tree/Table > independant. > Also TreeColumnViewerLabelProvider supports TreePaths which the TableColumnViewerLabelProvider does not. I see your point though as Trees support ITableLabelProvider we should make thier label provider support it too. Boris and I will be going through this today and we hope to start committing it as as experimental API early next week. That's great. Then we have a starting point and continue to integrate all those very nifty things although many of the things depend on the SWT-Team providing the Base-Classes to Tree/Table TableItem/TreeItem. I think I have to hold off all my work (Tooltips,....) until its clear what way to go. (In reply to comment #41) > Also TreeColumnViewerLabelProvider supports TreePaths which the > TableColumnViewerLabelProvider does not. I see your point though as Trees > support ITableLabelProvider we should make thier label provider support it too. > > Boris and I will be going through this today and we hope to start committing it > as as experimental API early next week. > Tom here is the patch that I am planning to submit to HEAD on Monday . Note all all new API has been marked experimental as I am sure we are all going to want changes. The highlights to my changes are 1) Removed the new tooltip interfaces and added the methods to ViewerLabelProvider. One of the goals of this work was to reduce the proliferation of interfaces we have 2) Removed the internal package. I used package visibility where possible. JFace currently doesn't use internal packages so I would just as soon not add one unless we had a strong reason 3) Made RowPart a class and removed DefaultRowPartImpl 4) Added ColumnViewer between StructuredViewer and the Tree and Table classes and pushed your additions there. You suggested this before and it looks like you were onto the right thing. 5) Added comments to all new API 6) Added Toms name to a bunch of classes he has touched 7) TreeColumnViewerLabelProvider extends TableColumnViewerLabelProvider as suggested. I am going to also suggest that we mark this fixed after I commit as we are up to comment 44 now and 10 attachments so this bug will be impossible to follow. Created attachment 47722 [details]
Patch August 10
(In reply to comment #43) > Tom here is the patch that I am planning to submit to HEAD on Monday . Note all > all new API has been marked experimental as I am sure we are all going to want > changes. > Greate news I'll look at them tomorrow its late in the evening in Europe. Tom (In reply to comment #43) > Tom here is the patch that I am planning to submit to HEAD on Monday . Note all > all new API has been marked experimental as I am sure we are all going to want > changes. > > The highlights to my changes are > > 1) Removed the new tooltip interfaces and added the methods to > ViewerLabelProvider. One of the goals of this work was to reduce the > proliferation of interfaces we have You are right but we don't need the methods getTooltip*(Object object, int columnIndex) because users can access the columnIndex they are working for from getColumnIndex() besides that they get never called from within Viewers and user may get confused. > > 2) Removed the internal package. I used package visibility where possible. > JFace currently doesn't use internal packages so I would just as soon not add > one unless we had a strong reason Accepted. > > 3) Made RowPart a class and removed DefaultRowPartImpl Accepted. If SWT-Team will continue to work on common interface this could maybe be removed. > > 4) Added ColumnViewer between StructuredViewer and the Tree and Table classes > and pushed your additions there. You suggested this before and it looks like > you were onto the right thing. > Great. But I think this is only the first part of the story. I think now that we introduced the concept of RowPart we should provided TableViewer and TreeViewer which are free from Table/Tree, TableItem/TreeItem, TableColumn/TreeColumn. But this has to be addressed in future patches. > 5) Added comments to all new API > Accepted. I hope you've corrected my language mistakes where I had provided an comment ;-) > 6) Added Toms name to a bunch of classes he has touched > Accepted ;-) > 7) TreeColumnViewerLabelProvider extends TableColumnViewerLabelProvider as > suggested. Accepted. > > I am going to also suggest that we mark this fixed after I commit as we are up > to comment 44 now and 10 attachments so this bug will be impossible to follow. > Accepted. Other things I found: - TableViewer.createNewRowPart(RowPart parent, int style, int rowIndex) should use the passed style same is true for TreeViewer.createRowPart - TableViewer.createNewRowPart(RowPart parent, int style, int rowIndex) and TreeViewer.createNewRowPart(RowPart parent, int style, int rowIndex) should have different visibilities and should defined as an abstract method in ColumnViewer - Rename ViewerLabelProvider.getTooltipImageStyle(Object object) => getTooltipStyle(Object object) - Cell should be renamed to CellPart to stay consitent. The rest looks good and I'm looking forward to your submit. The things on my todo list for future bugs or already active ones: - finish TooltipSupport (=> Delays not used at the moment) - finish editor activation customization (e.g. activate on double-click) - finish tab navigation from cell-editor to cell-editor - provide generic TableViewer and TreeViewer Baseclasses free from Tree/Table, ... . No bug report exists for this. What do you think about this? It would provide Viewers "automatically" for e.g. Nebula-Widgets) And one more thing I just found: ViewerLabel doesn't need to restore the Tooltip-Infos because they are accessed directly from the ViewerLabelProvider. Not that I really mind but my name is missing in some of the Files: - TableColumnViewerLabelProvider.java => use column-index - TableColumnViewPart.java => initial API - TreeColumnViewerPart => initial API - TableEditorImpl.java => usage EditingSupport - TreeEditorImpl.java => usage EditingSupport - TableViewer.java => comment wrong because bug 8200 is addressed in ColumnViewer (=> concept of RowPart) - TreeViewer.java => concept of RowPart - EditingSupport.java => comment in class JavaDoc? Patch has been released to HEAD with updated comments. Tom if you could double check that I got you everywhere I would be grateful. I am going to close this bug now - we can open new ones for other issues/discussions. (In reply to comment #48) > Patch has been released to HEAD with updated comments. Tom if you could double > check that I got you everywhere I would be grateful. > > I am going to close this bug now - we can open new ones for other > issues/discussions. > (In reply to comment #48) > Patch has been released to HEAD with updated comments. Tom if you could double > check that I got you everywhere I would be grateful. > > I am going to close this bug now - we can open new ones for other > issues/discussions. > Hi Tod I cann't help but somehow the comments are mixed up some of them are in the Javadoc of the class like e.g. TableColumnViewerPart and in some of them e.g. TreeColumnViewerLabelProvider in the copyright part. It somehow seems to be mixed up. I always thought the standard is to mentionned all contributors in the Copyright header but if I'm wrong here please correct me. What do you think about my comments in comment 46 e.g. "public Point getTooltipShift(Object object, int columnIndex)" is still part of ViewerLabelProvider-API but its not needed and our Viewers won't call. I've one subclasses it an uses it as a Provider for more than one Column he/she can use getColumnIndex() to distinguish between rows. Do you think I should open new bugs for this? > ViewerLabelProvider-API but its not needed and our Viewers won't call. > I've one subclasses it an uses it as a Provider for more than one Column he/she > can use getColumnIndex() to distinguish between rows. > sorry I meant columns not rows. > Do you think I should open new bugs for this? > That is a mistake. It should be in the copyright as you say - I got caught out by code folding I think. Let me know if there are any others. The tooltip work is largely being driven by you so if you think we should change the visibility of a method here and there by all means attach a patch and I'll apply it. Created attachment 47944 [details]
Fix Tooltip-Methods and Comments
This patch fixes the following things:
- ViewerLabelProvider
- remove api-methods with column-index
- rename getTooltipImageStyle => getTooltipStyle
- TreeViewer
- fixed copyright comment (added myself)
- fixed createNewRowPart to use passed style
- TreeEditorImpl
- fixed copyright comment (added myself)
- TreeColumnViewerPart
- fixed copyright comment (added myself)
- TooltipSupport
- getTooltipImageStyle => getTooltipStyle
- TableViewer
- fixed copyright comment (added myself)
- createNewRowPart use passed style
- TableEditorImpl
- fixed copyright comment (added myself)
- TableColumnViewerLabelProvider
- fixed copyright comment (moved from Javadoc to header)
Patch released - thanks. Created attachment 48028 [details]
Example how to use Tooltips
Created attachment 48089 [details]
One more wrong copyright ;-)
Released patch. I took the liberty of spelling your name correctly as well <grin>... Will the changes to the editor activation allow for an editor to activate when a user starts typing? (In reply to comment #57) > Will the changes to the editor activation allow for an editor to activate when > a user starts typing? > This is not really defined at the moment and hasn't been my primary focus. I think editor activation is better discussed in https://bugs.eclipse.org/bugs/show_bug.cgi?id=87733 maybe the label should be changed to "Customizeable Editor Activation". A problem I can think of with KeyActivation is that you don't really know which Cell in the row the user wants to edit. To provide this Table's need to provide the possibility to grab the cell under focus or we need to track it our own in jface, some where I have code that does that but it's not really usable at the moment. The APIs added with this bug need some cleanup:
* all new APIs (ViewerLabel, ViewerLabelProvider, etc.) needs "@since 3.3" tags
* SWT uses "ToolTip" (camel case) in APIs and "tool tip" (lowercase, two words) in javadocs. JFace should follow that convention and
- change APIs to use camel case
(e.g. ViewerLabel.setTooltipText(..) -> setToolTipText(..))
- adapt text in javadocs (
* APIs in ViewerLabel are too terse, e.g.
/**
* @return Returns the tooltipShift.
*/
public Point getTooltipShift() {
- should explain what the "shift" is (or link to
ViewerLabelProvider.getTooltipShift(..), ...)
- terms like "tooltipShift" are inconsistently used and look like internal
names. Should be "tool tip shift", etc.
* Javadocs should be spell- and grammar-checked, e.g. ViewerLabelProvider.getTooltipShift(..) ("This is the amount the used [..]") or ViewerLabelProvider.useNativeTooltip(..) need some polish.
Shall I open a new bug, or do you want to reopen this one?
Verified in 20060919-0100 |