Community
Participate
Working Groups
This is used to take out the editor activation from bug #151295 and provide this feature for M5.
maybe we need to retarget but I hoping for M5
Created attachment 58144 [details] Fully customized editor activation Man I'm comfortable with patch it's much smoother than what we had before.
+1 I think this approach is much better. Putting the cell selection stuff on top of this later should be easy. I'm hopeful this can go in M5 too.
Created attachment 58157 [details] Small fix this fixes a small problem when tabing from row to row the selection hasn't been update correctly
Created attachment 58201 [details] Slightly modified approach Boris I think I've implemented all we talked about today on the phone. I left the EditorActivationEvent-Thing in because I envision that this gives us more power (e.g. when editor is activated we can show informations in other views, ...) the only thing I changed is the fact that one can now veto the deactivation
Boris before commit could you rename the constants in EditorActivationEvent: MOUSE_CLICK_SELECTION => MOUSE_CLICK MOUSE_DOUBLE_CLICK_SELECTION => MOUSE_DOUBLE_CLICK KEY_PRESSED => KEY_PRESS
Created attachment 58739 [details] Fixed to apply to CVS-HEAD after M5 with some minor changes like method/class namings
Created attachment 58764 [details] KeyNavigation + Editor Activation Once more I need to provide both things in one patch because else I think I'll miss a class. Editor-Activation is fairly the same than before (once more small renamings happend) but now this also includes new support classes to make keyboard-activation available for SWT's-Tree/Table-Control.
One more note. Currently I don't documented any thing this will be done immediately after the code is accepted because it takes enough time to code the whole thing and if not accepted this would be simply lost time on my side.
Created attachment 59015 [details] next iteration... Here is my result of looking at this today. I renamed a lot of the classes. Remaining questions/comments: 1. System.err.printlns need to be removed 2. CellEditorDeactivationEvent: Can we use one event type (CellEditorActivationEvent) for both activation and deactivation 3. CellSupport/SWTCellSupport: Not happy with the names, this might be because I am not sure what the responsibilities are for these two classes 4. ColumnViewer.setTabEditingStyle: It would be good if we could remove this method and only expose it on EditingSupport (in the constructor?) 5. setEnableEditorActivationWithKeyboard: Can we move this into the constructor, for example as a style bit? 6. ColumnViewerEditorActivationSupport: name too long :) 7. Overall: setting this up (see snippet) is a very complicated multi-step process. Can we simplify this for the common case (i.e. pick reasonable defaults for table and tree and make this case easy without losing the flexibility)? 8. We need to test this on all the platforms...
1. Accepted and removed on my localsystem 2. I'm not happy with that because there are so many informations we can not provide when we decativate an editor and maybe in future we want to extend the Deactivation e.g. passing the current value of the editor 3. CellSupport: add cell support for viewers (1. CellFocusListener infrastructure, 2. API to get current focus cell) SWTCellSupport: specialized implementation for SWT-Controls who don't provide cell support out of the box 4. Tabbing is an overall setting for the Control and doesn't change from Column to Column where editing support works so I don't think this is set using EditingSupport constructor 5. Accepted and done 6. I don't have a better term maybe a native speaker can help us here? 7. What would be if we provide factory methods in TableViewer, TreeViewer, ... 8. I can test on WinXP and Linux-Gtk
Boris you somehow broke editing startup in the second column
(In reply to comment #12) > Boris you somehow broke editing startup in the second column This might just be something that I changed in the snippet.
Created attachment 59270 [details] Patch to fix some of the mentionned bugs by the way you are right it was a modification you made in the Snippet
Created attachment 59808 [details] Moving much of the code to EditingSupport i've started moving over great bunches of code from ColumnViewerEditor to EditingSupport maybe this helps Databinding. Now it's your turn Boris because you are the only one who knows both worlds to open the API so that it can be of use for the Databinding API or give me hints how this has to be done
one more thing maybe we need to remove the event canceling in when the editor is deactivated because this could bring us into trouble
Created attachment 59921 [details] start for next iteration This is the first part it brings the editor-activation with events and move TABEDITING to ColumnViewerEditor
Created attachment 59931 [details] Heureka der Patch is da Boris now we have the API we mused about yesterday. Chris time for a comment from yourside?
ad Bradley, I've now added the following function in EditingSupport for Databinding: - getCellEditorValue() - setCellEditorValue() Maybe there's more API you need but I think Boris and you are the ones to work this out :-)
I was thinking something a little more along the lines of... class EditingSupport { public CellEditor editElement(Object element, ViewerColumn column); public void cancelEdit(Object element, ViewerColumn column); public void applyEdit(Object element, ViewerColumn column); } The roles would become: 1. ViewerColumnEditor - widget specific logic. Provides notification of the editing lifecycle to EditingSupport. 2. EditingSupport - element specific and widget agnostic logic. By providing this separation you'll be allowing for knowledge of the element and the edited element to be configurable external to the viewer. For Data Binding we could provide columns that hold metadata about the attribute being displayed and edited. Then in implementations of EditingSupport provided by Data Binding we'd be able to setup the Binding and cancel, apply, and rollback changes when notified by ViewerColumnEditor. A default version of JFace EditingSupport could provide template methods for the retrieving of the value and setting of the value like it does today.
Created attachment 60115 [details] Fixed small outstanding bugs - the concret classes are now only accessible through static method - fixed problem with focus moving when tabing - fixed problem when SWT-table/tree receives focus the first time via keyboard (top-left cell selected) Ad Chris I added a comment for you in GridViewerEditor#updateFocusCell(ViewerCell focusCell) where we would need a Grid#setFocusCell(Point) to mark to move the focus cell on tabbing. Looking forward to your comments
I'm still looking through everything but generally it looks real good. Only one question so far... when tabbing through the row selection seems to change. In Grid, the selection models for cell selection vs row selection are quite different. We don't want the viewer to be changing the selected row (or even the selected cell) when tabbing through. Can we change that?
Sorry Chris for coming back to you that late but I was in Italy last week giving lectures. Currently configuring the selection updating is not possible but I'd say we could move this to a method where implementors could overwrite, I'll take a look tomorrow or later today.
Created attachment 60666 [details] Fix to apply cleanly to HEAD
(In reply to comment #22) > I'm still looking through everything but generally it looks real good. Only > one question so far... when tabbing through the row selection seems to change. > In Grid, the selection models for cell selection vs row selection are quite > different. We don't want the viewer to be changing the selected row (or even > the selected cell) when tabbing through. Can we change that? > this is now changed and you control the selection updateing when tabbing now completely yourself in ColumnViewer#updateFocusCell()
I am happy with the design and API, with a minor modification which will make data binding easier, and a couple of typos and unused methods that should be corrected/deleted. We should also have more Javadoc. SUGGESTED MODIFICATIONS FOR DATA BINDING: In ColumnViewerEditor.activateCellEditor(): - Object value = part.getEditingSupport().getValue(element); - part.getEditingSupport().setCellEditorValue(cellEditor, value); + part.getEditingSupport().initializeCellEditorValue(cellEditor, cell); In ColumnViewerEditor.saveEditorValue(): - Object value = part.getEditingSupport().getCellEditorValue(cellEditor); - part.getEditingSupport().setValue(tableItem.getData(),value); + part.getEditingSupport().saveCellEditorValue(cellEditor, cell); The default implementation of the two new methods in EditingSupport would be: void initializeCellEditorValue(cellEditor, cell) { setCellEditorValue(cellEditor, getValue(cell.getElement())); } void saveCellEditorValue(cellEditor, cell) { setValue(cell.getElement(), getCellEditorValue(cellEditor)); } SOME TYPOS: CellNavigationStrategy.cancleEvent -> shouldCancelEvent ColumnViewerEditor.processTraversEvent -> processTraverseEvent ColumnViewerEditorActivationEvent.PROGRAMATIC -> PROGRAMMATIC ColumnViewerEditorActivationEvent.cancle -> cancel SWTFocusCellManager.getPrimaryFocusCell -> getInitialFocusCell (Some of these are systematic typos, could you please do a search if the same typo was made in other places too?) UNUSED API, SHOULD BE REMOVED: ColumnViewer.setEditorActivationSupport ColumnViewerEditor.setEditorActivationStrategy TableViewerEditor.createSelection TreeViewerEditor.createSelection It seems that the following method should not be called by clients and could be made package-private: ColumnViewerEditorActivationStrategy.setEnableEditorActivationWithKeyboard
Tom, I'm sending the API request to the mailing list in the hope that you can address my comments before tomorrow morning Ottawa time.
Tom, did we address Chris' comment #22?
(In reply to comment #28) > Tom, did we address Chris' comment #22? > Yes we did as stated in my comment #25
Created attachment 61074 [details] Fixing final issues - fixed the issues Boris found yesterday including support for Databinding - fixed problem with editElement() when a TreePath is passed by overloading editElement() in TreeViewer, maybe for reusability this can be moved upwards after 3.3 by adding a new protected method subclasses can call editElement(Item,int) - updated snippet to also test editElement using a TreePath when clicking the button
Created attachment 61075 [details] Fixed small issue when editor is activated using editElement
+1
We have started working on this feature on ~8 months ago made ~50 full patches most of them about 100 KB and now can say: "Released >= 20070316"
Verified in I20070321-1800 by running Snippet026TreeViewerTabEditing
