Community
Participate
Working Groups
Tabel viewer allows user to have cell editors, but these cell editor can ONLY be activated through mouse click, no key combinition can activate the cell editor. This would fail the accecibility test. Some default key combinition (arrow key to navigate through column/cell, enter key to activate the cell editor) are needed for the table viewer
See also bug 36230. JDT/UI has already implemented some of these requests in the Change Method Signature refactoring dialog (e.g. F2 or Return to edit, (Shift+)Tab to switch columns, and Up/Down to switch rows). We would be glad to get rid of that custom code (currently distributed all over ChangeParametersControl and TableTextCellEditor).
1. See bug 87733 for an implementation of cell-editor activation. 2. See bug 142655 for Tabbing through Table/Trees (In reply to comment #1) > See also bug 36230. > > JDT/UI has already implemented some of these requests in the Change Method > Signature refactoring dialog (e.g. F2 or Return to edit, (Shift+)Tab to switch > columns, and Up/Down to switch rows). We would be glad to get rid of that > custom code (currently distributed all over ChangeParametersControl and > TableTextCellEditor). >
Created attachment 52385 [details] First rough idea how tab editing support could look like This is first draft to start discussing Tab-Navigation from Cell to Cell
Created attachment 52386 [details] Example to test tab navigation
Paul is the keys guru
Just for reference this is somehow related to bug #164598 which assigned to Eric and so I'll maybe loose track of it.
It would maybe make sense to give ViewerCell/ViewerRow the possibility to provide methods to find all its neighbour-cells (left,right,top,bottom). In think this would make the navigation features much easier. I only want to log this idea to not forget about it.
Created attachment 54627 [details] Update to new APIs
I should be able to review this Thursday. PW
Created attachment 54771 [details] Apply to HEAD cleanly after yesterday's changes
Created attachment 54772 [details] Sorry once more with fixed copyright parts
Created attachment 54802 [details] Slight update for patch This is essentially your patch, with a slight modification to not recreate the mouse and traverse listeners on every activate editor. The patch looks good. I only had 3 comments: 1. Given the changes to Editor
Sigh ... and once more with feeling (and my comments): 1. Given the changes to EditingSupport, does the anonymous inner class in ColumnViewer need as many methods to be overriden? 2. Since setValue(*) in EditingSupport says "Subclasses should overwrite!" maybe that method should still be left as abstract, especially if the default implementation doesn't make sense. 3. This causes compile errors in Snippet19 and Snippet24 that also need a patch (and removal of any methods that the default is fine for). The patch is good, and as long as you address #3 it's ready to be committed. I'll be on IRC all afternoon. PW
Created attachment 54804 [details] Patch to take up Paul's comments
[Because none of you is following the foundation bug #151377 where I have posted this a few minutes ago.] Boris wanted me to explain the great picture for this new feature so here it is. For me cell-selection events are fired under the following circumstances: - click on cell - double click on cell - keystrokes defined using something like ColumnViewer#setCellSelectionStrokes(KeyStroke[]) (maybe we should support standard strokes like SPACE, RETURN using constants) Based upon this information I think EditingSupport will get a new API function like EditingSupport#isEditorActivationEvent(CellSelectionEvent) which will decide if an editor for the currently selected cell is started. CellSelection based on Mouse-Events is nearly ready if this patch goes in CVS but I haven't implemented it in this patch because this bug only talks about the ground work providing all those new functionalities. The reall work on editor activation customization needs to be done in bug #151295. Let's now go to keyboard-activation. To make this happen the first thing we need to support is keyboard navigation inside table/tree's. To accomplish this we need to track the currently selected cell ourselfs by hooking into the keyboard events(Boris could you once more provide the link you sent me while chating I've somehow lost it). We need to provide a methods in CellLabelProvider which deals with layouting the currently selected cell and maybe turn of the system selection using ownerdraw like shown in one of the SWT-Snippets. The rest is easy if the SelectionEvent is fired because one of the defined key-strokes is detected EditingSupport#isEditorActivationEvent(CellSelectionEvent) is checked and the editor in the cell is activated. To support keyboard activation I have no test-code until now but I only wanted to present the great picture to all of you. I'd love to get some comments from all of you interested in this for my understanding really very important feature for Datacentric-Editing-Applications based upon JFace-Viewers.
Created attachment 56213 [details] This is a cumulation patch to implement fully customizable editing This patch holds code from many different bugs, I'll post all patches included in this one but I couldn't seperate what is going to which bug report because all this is working hand in hand. This patch provides the following things: - fully featured keyboard navigation ColumnViewer#setEnableKeyNavigation() - fully customizable keyboard selection ColumnViewer#setKeyBoardSelectionStrategy() - fully customiable editor activation on keyboard events using ColumnViewer#setEditorActivationStrategy() This is really big patch but there are so many things that need to work together please give the included snippet a try and get amazed by how things work out. There some limitations though yet: - not working with our OwnDrawLabel-Provider (investigation needed) - expanding/collapsing of tree-nodes is not possible any more (fixable in TreeViewer but this can wait until TreeViewer)
Created attachment 56220 [details] Modified Patch with much less API and new features This patch is a slighlty revised version from the previous one: - JavaDoc for new methods, ... added - ICellSelectionListener#cellSelected() called for every selection action (any keyboard/mouse action) - ICellSelectionListener#cellSelectionChanged() called when the selected cell is changed - ColumnViewer#setKeyBoardSelectionStrategy() and friends removed in favor of the new selection mechanism ;-) This big patch includes the patches (some slightly modified) from: - bug #151377 - bug #167325 And fixes an undiscovered bug in TreeViewerRow#getColumnCount()
By the way tested this implementation on win32 and linux-gtk could someone please give it a try on motif and carbon?
Created attachment 56269 [details] Further improved the API to allow multi-cell-selections I've now further improved the whole CellSelection implementation to also support multi-item selections. Currently only the API for this is in place the implementation for mouse-selections has started but is not finished till now.
Created attachment 56270 [details] Reverted multi-selection support After banging my head against the whole thing the whole day I decided not take the multi-item support further. I contrast I went back to the old API. The only new thing is that I'll now provide the possibility to color the selected cell using a custom fore- and background color.
Created attachment 56320 [details] Once more a new version Last night I rethought the whole and slightly modified my solution: - the ICellSelectionListener is not a interface and is named CellListener - the CellListener has 2 distinct methods: - cellSelected(CellSelectionEvent) for real selections via mouse/keyboard - keyPressed(KeyEvent) for key events occurring on a selected cell - Activation is controlled by a special event named EditorActivationEvent: - This event object wraps up the CellSelectionEvent and KeyEvent - This event object is used in EditorActivationStrategy to decide if the given event is really a EditorActivationEvent - This event object is passed on to CellEditor.activate(EditorActivationEvent) - setEnableKeyNavigation(boolean) is removed and replaced by setCellSelectionType(int) which opens the opportunity in future to also support other selection-styles. Some of you might ask why does the CellEditor need to get the activation event passed by? Well the reason is fairly simple: If any key event triggers the poping up of the editor you would lose the first entered key. This is a nice feature we get for free with this patch (see bug #87733 comment c13 from Chris) ;-)
Added the bug 169517 although the current patch holds the workaround mentionned in the if it is one
*** Bug 142655 has been marked as a duplicate of this bug. ***
Created attachment 56471 [details] a new patch with comments from me and Boris about TODOs this is the essence of our phone conference today
More todos: - When you start Snippet026TreeViewerTabEditing, it does not have a visible cell selection, but you can still arrow around in the tree (and expand items with arrow-right!), you just don't see the selection. - After fully expanding the tree, if you hold the arrow-up or -down key down to move the selected cell up or down, the CPU goes to almost 100% and it feels slow. - In edit mode, if you move between cells using TAB, there is flicker if the cell you were in is not in the same row as the cell you are tabbing into. - When the cell editor is active, there is extra green background if you are in the first column, but not in the other columns. Should the green background only use the space that ends up being used by the cell editor? - Why can you set the selection color separate for each column?
(In reply to comment #25) > More todos: > - When you start Snippet026TreeViewerTabEditing, it does not have a visible > cell selection, but you can still arrow around in the tree (and expand items > with arrow-right!), you just don't see the selection. That's really strange. I'm trying to find a solution but I need to dig into it. > - After fully expanding the tree, if you hold the arrow-up or -down key down > to move the selected cell up or down, the CPU goes to almost 100% and it feels > slow. Well the problem is that the calculation what's the next row in a tree is is fairly complex. Do you have any suggestions how to avoid this? As said a keyup handler is not a possibility because this leads to flickering in all other situations ;-) > - In edit mode, if you move between cells using TAB, there is flicker if the > cell you were in is not in the same row as the cell you are tabbing into. Maybe time for a setRedraw(false) code? I'll upload the patch you could give a spin shortly afterwards. > - When the cell editor is active, there is extra green background if you are in > the first column, but not in the other columns. Should the green background > only use the space that ends up being used by the cell editor? Well I don't think you are right maybe for Trees without a background color in none selection mode this is true but looks somehow strange if you have an alternate background color set. The first column in a tree behaves differently not only in this respect (e.g. the text content is simply clipped when it does have enough space whereas in other columns you get the famous "...") > - Why can you set the selection color separate for each column? > The question is why not. Would you preferr having: - ColumnViewer#setCellSelectionBackgroundColor() - ColumnViewer#setCellSelectionForegroundColor() What if the current background color of a cell is in none selection mode the same than in Selection mode because you can alter background colors on base of Cells I think for consitency we should also provide this feature for selections maybe we should even provide the possibiity to modify the font?
Created attachment 56547 [details] Patch with potential fixes this is a patch with potential fixes for: - problem 1 from boris (navigation, expanding, ... with no selection) - problem 3 from boris (flickering when tabing from row to row) - added TODO information from Boris into the Patch to keep track was has to be done
Created attachment 56609 [details] Fixed all TODOs except KeyNavigationListener - new delegating possibility to draw cell-selection - programatic activation of editor is also done using EditorActivationEvent The missing part is the KeyNavigation-Customization which could also be done using the Delegate-pattern: - KeyNavigationDelegate#isNavigationEvent(Event) - KeyNavigationDelegate#isCollapseEvent(Event) - KeyNavigationDelegate#isExpandEvent(Event) - KeyNavigationDelegate#findSelectedCell(Event) Comments?
Created attachment 56808 [details] Patch feature and API complete I've no fixed all of the TODO except the ones I: - Can't fix: - CPU load 100% when KeyDown is held - Are features IMHO - different back/foreground color foreach selected cell - colored part for Trees first column The features can be worked around easily by providing ones own selection drawing implementation. Maybe the methods in the delegates could named better maybe Boris can pass them by to native speakers?
Created attachment 57077 [details] Reapply to head and fix small issues with collapsing
Looks promising, but needs more work: 1. It should be possible to use something like SWT's TableCursor to show the currently selected cell, or owner draw (SWT.EraseItem). Custom controls might even have a third way of showing cell selections. I can only believe that our API is flexible enough if we had a working implementation using something like TableCursor (in addition to the current implementation using EraseItem). I expect that TableCursor cannot be used as is, it's OK to start with that code to generalize it. 2. Eight new fields in ColumnViewer is way too much for something that is optional behaviour. It feels like there should be a separate class (similar to AbstractViewerEditor). 3. DrawCellSelectionDelegate should be a separate class. I don't believe we should provide this as the default at the level of ColumnViewer, it is just not general enough. 4. I don't like getSelectedCellForeground/BackgroundColor as a method on CellLabelProvider. This can be configurable per DrawCellSelectionDelegate instead. The strongest argument for this is that the cell selection may be displayed by other means than foreground/background color, for example by drawing a border around it like in Excel.
Created attachment 57223 [details] Patch to address Boris comments ad 1. I think the API is able to deal with this because the DrawingDelegate now registers the listener itself on the control (I try to prove this though afterwards) ad 2. Done we now have a AbstractCellSelectionSupport-Class which is responsible for the whole cell-selection thingie ad 3. Accepted and implemented as OwnerDrawCellSelectionDelegate ad 4. Accepted and implemented as methods in OwnerDrawCellSelectionDelegate
Created attachment 57224 [details] Revised patch to add listener support for editor activation events
Created attachment 57225 [details] Example impl when using TableCursor This is a rough example to start an implementation where we use something like a table-cursor but this is not working 100% at the moment is not meant to go into CVS it's just a try
Created attachment 57226 [details] Sometimes complex things are so simple I've now added support for Canvas-Cursors, we cannot implement a generic cursor in JFace because there are platform specific problems which have to addressed and JFace is free from all this stuff. As an example I've for now implemented a TableCursor example in snippets section (this is not meant to get integrated in CVS only to give you a possibility to play with it). So Boris more comments are welcome
Created attachment 57227 [details] I promise this is the end now :-) Fixed a small problem with CellCursors because double-clicks are not propagated properly
couple of minor things i noticed when working with this patch 1. In AbstractViewerEditor#activateEditor on line 104 "if( ! editorActivationListener.isEmpty() ) {" if you haven't added at least one listener you get an npe. This is caused by the listenerlist not being created until the first listener is added. 2. Would it be possible to develop a mechanism that extending class of ColumnViewer can contribute their own ViewerColumn implementation instead of the default ViewerColumn? Specifically i think if you made ColumnViewer#createViewerColumn protected this would allow extending viewers to determine what type of ViewerColumn is created.
How do you envision would the Nebula grid show the currently selected cell?
> 2. Would it be possible to develop a mechanism that extending class of > ColumnViewer can contribute their own ViewerColumn implementation instead of > the default ViewerColumn? Specifically i think if you made > ColumnViewer#createViewerColumn protected this would allow extending viewers to > determine what type of ViewerColumn is created. ColumnViewer#createViewerColumn exists for backwards compatibility, it creates a ViewerColumn which is not designed to be accessed by clients. The intent was that for each subclass of ColumnViewer, viewer implementers would provide a corresponding subclass of ViewerColumn. This would be used similar to how you set up an SWT Tree or Table: TreeViewer tv = new TreeViewer(parent); TreeViewerColumn tvc1 = new TreeViewerColumn(tv, SWT.NONE); tvc1.setLabelProvider(...) TreeViewerColumn tvc2 = new TreeViewerColumn(tv, SWT.NONE); tvc2.setLabelProvider(...)
(In reply to comment #39) > > 2. Would it be possible to develop a mechanism that extending class of > > ColumnViewer can contribute their own ViewerColumn implementation instead of > > the default ViewerColumn? Specifically i think if you made > > ColumnViewer#createViewerColumn protected this would allow extending viewers to > > determine what type of ViewerColumn is created. > ColumnViewer#createViewerColumn exists for backwards compatibility, it creates > a ViewerColumn which is not designed to be accessed by clients. The intent was > that for each subclass of ColumnViewer, viewer implementers would provide a > corresponding subclass of ViewerColumn. This would be used similar to how you > set up an SWT Tree or Table: > TreeViewer tv = new TreeViewer(parent); > TreeViewerColumn tvc1 = new TreeViewerColumn(tv, SWT.NONE); > tvc1.setLabelProvider(...) > TreeViewerColumn tvc2 = new TreeViewerColumn(tv, SWT.NONE); > tvc2.setLabelProvider(...) We would also like provide the backwards compatibility with the viewers. That way if someone uses an existing Table/TreeViewer snipet as starting point but substitue the GridViewer the snipet would work. Currently to use the GridViewer users are forced to update to the new ViewerColumn style of setting label providers on the column and not the GridViewer. i believe selection for a nebula grid is shown in what ever fashion the cell renderer wants to display the selection. The default cell renderer changes the background color of the cell. Chris can correct me if i am wrong
Created attachment 57544 [details] Get rid of NPE thanks Ross for spotting this
Ross I think what me and Boris interests most is whether the proposed API makes it possible for Nebula to color selected cells and use the new navigation feature?
Boris what just came to my mind is that this whole thing is not about selection color in bug focus coloring so I think I should rename to reflect this?
Created attachment 57712 [details] apply to HEAD
Hi Tom, Boris, I apologize for not spending more time on this issue sooner. I've been a bit swamped. I'm a little confused by the discussion. It seems the editor activation, cell selection coloring, and editor tab traversal are all lumped together here. I'm primarily interested in editor activation and traversal. The Grid already has cell selection and I would like to leave that code in the Grid itself. Obviously thats not possible for Table. I think the best approach would be to allow the viewer to put a thin layer over the existing cell selection in Grid but that does not seem to be possible currently. The code for the selection seems to be private. I need to research some more but I think I'd like some pretty significant changes. -Chris
Thanks for your reply Chris. This current patch doesn't provide the tab-traversal code at all. Tab-Traversal has been applied to M4 and is currently working (bug #166383). Tab-Traversal stays as is and is already working the only thing nebula needs to provide to make this working is the API introduced in bug #167325. The latest patches found in this bug deal with focusing the last selected cell by mouse and navigating the ColumnViewer using the keybord. As you may have discovered for SWT-Widgets this is not an easy thing because SWT doesn't have the possibility to color a single cell in a Column-Widget without some extra work (registering of EaseItem-Listener). I think for Grid the situation I think is much easier you simply need to register a CellSelection-Listener and you get informed about when a cell is selected using mouse or keybord (a good example is my example for SWT-Tables using the TableCursor see the jface-snippets included in the patch). The second part of the patch deals with EditorActivation and customizing it to a very great extent. The major difference to the situation before is that editor-activation is done using events and you can control from the outside which Event-Triggers this activation and you get passed all information about the activation event to your CellEditor this way you can e.g. implement that an editor starts up with any keypress (beside the navigation key) on the currently selected/focus cell and automatically fill in the typed character (I think this feature was requested by you in another bug report). Nevertheless I'm uncertain we can finish this for M5 because Boris is also swamped with work although I think we could close a dozen of bugs against JFace-Viewers.
By the way I could try to provide a listener for Grid to see if the current API is able to deal with it ;-)
(In reply to comment #47) > By the way I could try to provide a listener for Grid to see if the current API > is able to deal with it ;-) > And one more reply do you have somewhere a patch I can apply to see your current GridViewer-Code?
I have the code. Its pretty simplistic thanks the work you've already done. I'll attach it later today.
Created attachment 57841 [details] Grid viewer working code Here's the working copy of our grid viewer code. Not too much different then you'd expect. I haven't implemented the new methods yet. The package names are all weird because we've just been hosting this in a random project, so ignore that.
Created attachment 57931 [details] Update patch only the viewer package I modified some class names and the most important thing is that I removed added passing of the cell loosing focus with the cell-selection event
Created attachment 57932 [details] Patch for Nebula to become a customer of this API
Created attachment 57935 [details] New patch which should not create compile time errors
Created attachment 57939 [details] The best from both worlds for Chris
Created attachment 58027 [details] Final patch to bring this in for M5 This patch includes: - Some internal refacoring in JFace - OwnerDrawSupport for Table/Tree - TableCursor Support for SWT-Table (concrete implementation shown as snippet) - Support for Nebula-Grid The final conculsion is that I think the API is flexible enough to support different widgets as shown by nebula where all focus information is calculated by nebula and used by our classes. A waiting your feedback Boris, Chris, Tod, ...
I'm looking at the patch now, but I had to overcome an initial problem - AbstractCellSelectionSupport#getFocusCell needs to be protected so I can override it.
(In reply to comment #56) > I'm looking at the patch now, but I had to overcome an initial problem - > AbstractCellSelectionSupport#getFocusCell needs to be protected so I can > override it. > Why do you need to overwrite it. The magic is done in the navigation delegate maybe the patch is wrong?
The patch didn't work because getFocusCell in the subclasses wasnt being called - because it wasnt really overriding the super. The new behavior is really good. My only other concern is with the cell selection listeners. Just like with the navigation, currently the viewer code is controlling all this. So the cell selection listeners on the viewer don't report correctly when using cell selection in the Grid. I'd like to be able to do something with this like you just did with the editor activation. That is, let the viewer be able to delegate the firing of the selection listener to the widget itself. Maybe this is possible now, I'm not sure. In order to be able to model Grid cell selection perfectly, the CellSelectionEvent would have to contain an array of selected cells.
Removing target milestone because I think we need to rethink once more and M5 is to near. M5 is API-Freeze (internally) externally it is M6 if all internal consumers of the API agree I'd like to take this up once more for M6 else we need to delay to 3.4 or what ever version comes afterwards. Maybe we can get out at least the editor activation stuff? This would a first step in the right direction and customers of nebula-grid can use keyboard activation because all the nifty navigation stuff is not needed by nebula because it can already deal with cell-selection. Maybe we can meet on IRC (Chris, Boris) ping me when you are available.
To track the editor activation I filed bug #172646
for all who are interested in this bug I'd suggest to take a look at the last patch in bug #172646 I think we have now reach a state where we are not far away from what we all want to provide. So it would be just the right time to give feedback.
Patch released as part of bug #172646
Verified in I20070321-1800 by running Snippet026TreeViewerTabEditing
