Community
Participate
Working Groups
At the moment the concrete classes to provide a table or a tree require that the underlying Widget is an org.eclipse.widgets.Table and org.eclipse.widgets.Tree. It would make sense to extract all code parts who don't depend on the concrete-widget to an AbstractBaseClass between: - ColumnViewer > TableViewer => ColumnViewer > AbstractBaseTableViewer > TableViewer - AbstractTreeViewer > TreeViewer => AbstractTreeViewer > AbstractBaseTreeViewer > TreeViewer
Created attachment 48162 [details] First rough Version for TableViewer independency This patch already incooperates the the base class for TableViewerImpl and to avoid breakage of TableTree and maybe the API-Contracts a class named AbstractTableViewerEditorImpl. Maybe some words about what's done in this patch: - AbstractTableViewer widget independet BaseClass for viewers representing Tables all parts where the native control is used are marked as abstract and start with "native" maybe those could be moved to ColumnViewer because they are common to all Table/TreeViewers although this would be we need to update TableTreeViewer - AbstractViewerEditor all common things of TableEditorImpl/TreeEditorImpl - AbstractTableViewerEditorImpl common things for TableControls - TableViewer implements the abstract methods needed - TableViewerRow new method to clear item - TreeViewerRow new method to clear item - ViewerRow new abstract method to clear item (maybe this should be changed to provide a default implementation sub-classes can overwrite if they are not happy) The process for TreeViewer would be similar.
Created attachment 48271 [details] Fix some small visibility bugs
Created attachment 48273 [details] Example how a Viewer for Nebula-Grid is programmed Just as an example and how easy it would be to use other widgets ;-)
Created attachment 48301 [details] Fixed more Visibilities Most important visiblity changes: - EditingSupport public class so external packages like nebula could subclass it - EditingSupport made protected so subclasses from other packages could overwrite - ViewerColumn#setEditingSupport now public - ViewerColumn: removed dead commentened code parts
I've now made some test with this code and couldn't find any problems maybe you Boris and Tod could look at it and tell me what you think about this some day this week on IRC.
I don't think Boris has looked yet but here are some comments 1) The basic refactoring looks to me to be moving all of TableViewer to abstract table viewer and then factoring out all of the code that references the table directly correct? 2) I am not sure I like having the table show up in two fields (one on each class) although I am not that upset about it. 3) The code moves away from the idea of accessing the widget directly by having the nativeGet* methods. Although this is different from our usual pattern I think it is neccessary here. This is fair amount to absorb as a lot of it has no javadoc yet - we should talk over IRC this week.
(In reply to comment #6) > I don't think Boris has looked yet but here are some comments > > 1) The basic refactoring looks to me to be moving all of TableViewer to > abstract table viewer and then factoring out all of the code that references > the table directly correct? Yes. That's the idea. So all is widget independent only Widget and Item are used for now and even Item wouldn't really be necessary but there are some methods in StructuredViewer who need Item although when looking internally they don't need it (I think it was associate/disassociate). > > 2) I am not sure I like having the table show up in two fields (one on each > class) although I am not that upset about it. > Maybe an abstract method like getControl() could help here. So the table is only stored in the concrete class which would make sense? > 3) The code moves away from the idea of accessing the widget directly by having > the nativeGet* methods. Although this is different from our usual pattern I > think it is neccessary here. Well I also thought about a delegate pattern so we won't bloat the interface with all those nativeGet* methods. So you would pass SWTTableDelegate, NebulaGridDelegate, ... to AbstractTableViewer > > This is fair amount to absorb as a lot of it has no javadoc yet - we should > talk over IRC this week. > Sorry for that. Yes I left out javadoc for now because first of all I wanted to here from you what you think before I put too much effort into something not acceptable because of my design.
Created attachment 48460 [details] Updated patch based on discussions with Tom
Created attachment 48844 [details] Javadoc and updated to latest cvs - added javadoc - fixed to cleanly apply to head after Boris' last bug fix to TableViewer (all changes incooperated into AbstractTableViewer)
(In reply to comment #9) > Created an attachment (id=48844) [edit] > Javadoc and updated to latest cvs > > - added javadoc > - fixed to cleanly apply to head after Boris' last bug fix to TableViewer > (all changes incooperated into AbstractTableViewer) > oh forgot to remove Tooltip-Changes from bug 83200 sorry.
Created attachment 49090 [details] Fixed to apply clean to HEAD
Created attachment 51037 [details] Patch to apply to head This is the start of the widget independency refactory. The unit-test suite is passed without any error. A thing I noticed is the following comment in TableViewer: ---------------8<--------------- <p> * Users of SWT.VIRTUAL should also avoid using getItems() from the Table within * the TreeViewer as this does not necessarily generate a callback for the * TreeViewer to populate the items. It also has the side effect of creating all * of the items thereby eliminating the performance improvements of SWT.VIRTUAL. * </p> ---------------8<---------------
Created attachment 51826 [details] updated patch I have updated the patch. Overall, I like it, but I am not sure how it affects the performance of TableViewer. Also, before we commit to this as API, I would like it to be validated by others, minimally Nebula. (Would this be useful for anybody else other than Nebula?) Does Nebula support SWT.VIRTUAL at all? If not, does it still make sense to have the VIRTUAL support in BaseTableViewer? I will attach a corresponding patch against Nebula with an updated GridViewer and a snippet on bug 146945.
(In reply to comment #13) > Created an attachment (id=51826) [edit] > updated patch > > I have updated the patch. Overall, I like it, but I am not sure how it affects > the performance of TableViewer. Also, before we commit to this as API, I would > like it to be validated by others, minimally Nebula. (Would this be useful for > anybody else other than Nebula?) > Do you think that the function calls really matter? For large Tables where it might matter one uses SWT.VIRTUAL. > Does Nebula support SWT.VIRTUAL at all? If not, does it still make sense to > have the VIRTUAL support in BaseTableViewer? Well I think Chris mentionned somewhere that he'll work on it. But IMHO it doesn't harm us, does it? The only thing we wouldn't need then are the 3 API functions dealing with virtual. > > I will attach a corresponding patch against Nebula with an updated GridViewer > and a snippet on bug 146945. > Great work Boris.
Yes I do plan to implement SWT.VIRTUAL in the Grid. The Grid performs better than a normal Table (on Win anyway) but would still benefit from virtual support. More follow up comments in bug 146945.
Ok. After we decided that we won't provide a widget independent version of TableViewer I took a look at TreeViewer and don't think that we need to take any action on the JFace-side. We already have an widget independent implementation namely AbstractTreeViewer Nebula could provide it's own implementation on top of it. Most parts of TreeViewer only deal with the VIRTUAL-case which Grid doesn't support at the moment.
(In reply to comment #16) > Ok. After we decided that we won't provide a widget independent version of > TableViewer ... Just for the record - we won't provide a widget independent table viewer base class unless there is a client for it who can validate our new API. Without a client, we won't produce additional API. The "updated patch" (2006-10-11) can be applied against HEAD as of that date to see how the refactoring would have worked, should this become interesting again in the future. Chris, have you tried subclassing AbstractTreeViewer?
I haven't personally, but another guy on my team Ross has. Apparently there are alot of package level methods/classes that are in the way. From Ross: StructuredViewer.buildLabel() CustomHashTable TreeEditorImpl These are preventing us from just subclassing AbstractTreeViewer.
(In reply to comment #18) > I haven't personally, but another guy on my team Ross has. Apparently there > are alot of package level methods/classes that are in the way. From Ross: > > StructuredViewer.buildLabel() > CustomHashTable > TreeEditorImpl > > These are preventing us from just subclassing AbstractTreeViewer. > I'm currently working on a base implementation for TreeEditorImpl/TableEditorImpl it would have been part of Widget independency see bug #153993. Nevertheless I until today I'm not convinced that nebface doesn't want to provide 2 viewers for its Grids (one Tablebased, one Treebased) because I think many people (e.g. /me) will use it as an replacement for Table because of many limitations found in the native tables. It would be the same as if we would make TableViewer a subclass of AbstractTreeViewer.
Tom I'm all twisted up by your last comment. Not sure if you are advocating one viewer or two.? My latest thinking was simply to provide both viewers to keep in step with JFace.
(In reply to comment #20) > Tom I'm all twisted up by your last comment. Not sure if you are advocating > one viewer or two.? > > My latest thinking was simply to provide both viewers to keep in step with > JFace. > If you read Boris comment carefully ;-) (In reply to comment #17) > (In reply to comment #16) > > Ok. After we decided that we won't provide a widget independent version of > > TableViewer ... > > Just for the record - we won't provide a widget independent table viewer base > class unless there is a client for it who can validate our new API. Without a > client, we won't produce additional API. > I'm adocating 2 Viewers: - nebface.TableViewer based upon BaseTableViewer - nebface.TreeViewer based upon AbstractTreeViewer (or if needed BaseTreeViewer) Maybe time for another chat session?
(In reply to comment #18) > I haven't personally, but another guy on my team Ross has. Apparently there > are alot of package level methods/classes that are in the way. From Ross: > > StructuredViewer.buildLabel() > CustomHashTable > TreeEditorImpl Chris regarding TreeEditorImpl could you take a look at AbstractViewerEditor from bug #153993 > > These are preventing us from just subclassing AbstractTreeViewer. > if this suites your needs and Boris and Tod agree with my proposal there are only 2 items left ;-) Although I'm uncertain we should provide access to StructuredViewer.buildLabel()
Looks like the AbstractViewerEditor would work for nebula. My initial comments I gave chris was using the 3.2 release. I went back and pulled the latest version from cvs. Using the TreeViewer as a template I tried to create a quick and dirty NTreeViewer but ran into the following visibility problems when simply extending from AbstractTreeViewer. AbstractTreeViewer.internalFindItems(Object parentElementOrTreePath) AbstractTreeViewer.internalRefreshStruct(Widget widget, Object element, boolean updateLabels) ColumnViewer.updateCell(ViewerRow rowItem, int column) CustomHashTable StructuredViewer.newHashtable(int capacity) StructuredViewer.buildLabel ViewerColumn.COLUMN_VIEWER_KEY I didn't have a chance to actually see if these methods really needed to have their visibility modified. However if TreeViewer needed to make use of them then it stands to reason the nebula tree viewer will need them also. Alternatively if we introduced AbstractBaseTreeViewer then i don't think the majority if not all of the methods would need to have their visibility changed. Again I did this somewhat quickly and didn't have a chance to do a full investigation into just extending AbstractTreeViewer.
Thanks Ross for you info. I'm getting that my first feeling on BaseTreeViewer was not appropriate and that we should provide real widget independency. In my view AbstractTreeViewer is not really sufficient and is to some extend designed badly (e.g. overwriting getCellModifier(),...), internal methods (newItem, ...) to declared as such, ... . In an ideal world my JFace-Viewer-Structure would look like this: + ColumnViewer (provide all common to column based viewers) + BaseTableViewer (widget independent implementation for Table like Widgets) + VirtualTableViewer (virtual support for table based widgets) + [SWT]TableViewer (Real implementation for org.eclipse.swt.widgets.Table) + GridTableViewer (Real implementation for org.eclipse.nebula.wigets.Grid) + AbstractTreeViewer (needed for backwards compatility) + BaseTreeViewer (provide support for Tree based widgets) + VirtualTreeViewer (virtual support for tree based widgets) + [SWT]TreeViewer(Real implementation for org.eclipse.swt.widgets.Tree) + GridTreeViewer (Real implementation for org.eclipse.nebula.wigets.Grid) That would be an ideal world which we maybe cannot provide without breaking backwards compatility but in this world ever class would be responsible for what it is designed. Table/TreeViewer code would be so much easier to read if virtual /non virtual would be dealt with in one viewer. But I go along with Boris that if Nebula won't provide a GridTableViewer it wouldn't make sense to JFace to provide such a "dangerous" refactoring and blowing up of API if there's no customer. For TreeViewer aka BaseTreeViewer I'll try to come up with something in the next few days/weeks eventually after M3 is out.
Nebula will definitely include the viewers and I expect that my team will become consumers of these new viewers. So we should be able to put the code and API through its paces before 3.3 gets released. I just need to get mgmt buy in.
Created attachment 52278 [details] Just for reference Ross just for reference how a BaseTreeViewer with AbstractViewerEditor would look like. BaseTreeViewer as outlined in my above comment doesn't support VIRTUAL Trees but that doesn't hurt Grid, its just for refernce, I haven't tested nor validated it against any thing.
(In reply to comment #25) > Nebula will definitely include the viewers and I expect that my team will > become consumers of these new viewers. So we should be able to put the code > and API through its paces before 3.3 gets released. I just need to get mgmt > buy in. > Great news Chris. This means BaseTableViewer has a customer if I get you right ;-) I think bug #153993 with this one will provide the flexibility we are all after see me implementation of BaseTreeViewer (although created just within 1h6mins) is a good starting point.
Some issues here should BaseTableViewer be called AbstractTableViewer in order to be consistent with AbstractTreeViewer? the ViewerRow#clear() method is not well documented. I know what it is doing because I know the code base well but it would never be clear to anyone else maintaining this.
(In reply to comment #28) > Some issues here > > should BaseTableViewer be called AbstractTableViewer in order to be consistent > with AbstractTreeViewer? > Well and how do we then name the widget independent TreeViewer AbstractTreeViewer is not possible? I think if we provide 2 widget-independent viewers their naming should be consitent, so we need to find a name for Tree and Table.
FWIW Grid will support virtual soon. Please don't design the classes assuming Grid won't have virtual support.
> Just for the record - we won't provide a widget independent table viewer base > class unless there is a client for it who can validate our new API. Without a > client, we won't produce additional API. We've gotten the go ahead and my team will upgrade to the latest 3.3 milestone in the next week or so. We will adopt the milestones as we progress. So we can be a heavy consumer of the new API. We're looking forward to it as it will really save us alot of code.
Created attachment 54624 [details] Patch to make TableViewer widget independent
Created attachment 54727 [details] Patch after discussing with Boris - class renamed BaseTableViewer to AbstractTableViewer - internal* changed to do* to stay consitent with other Viewers - removed clearing of items in internalRefreshAll hence no ViewerRow#clear() needed
This bug is getting longer and longer should we recreate a new one for TreeViewer widget independency, rename this one so we could mark the TableViewer fixed and check that easily in the next M-Build?
Some of the methods were still called internal..., so I renamed them to do... Also, I cleaned up the Javadoc a bit and added @since 3.3 tags.
Changed patch released >20061208.
Thank you Boris. I only renamed the methods I add to do*. All of you who are interested about widget independent support for Tree-style widgets. Should subscribe to bug #167323.
Verified in I20061212-0010 by inspecting code
