Bugzilla – Full Text Bug Listing
| Summary: | [Viewers] [CellEditors] Table with SWT.MULTI and TableViewerEditor problem | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Alex Bernstein <alexberns> | ||||||||||
| Component: | UI | Assignee: | Thomas Schindl <tom.schindl> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||
| Severity: | major | ||||||||||||
| Priority: | P3 | CC: | agueda, alex.tugarev, bokowski, daniel_megert, galileo, jon.jekeli, klara.ward, Lars.Vogel, loskutov, magdalena+eclipse, marcus, marek.chodorowski, mistria, nicholas.karwath, niklas.deutschmann, oryandunn, pavanesh, pwebster, sptaszkiewicz, strider80, tom.schindl, zalapa | ||||||||||
| Version: | 3.4 | ||||||||||||
| Target Milestone: | 4.8 M4 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Windows XP | ||||||||||||
| See Also: |
https://git.eclipse.org/r/104279 https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=da427eaeb7c25a1a5f391c0d3fc043e91027b7d8 https://git.eclipse.org/r/110983 https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=679a28a9a149a4a5985121f848cc95b50069f39a |
||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
Is this a regression in 3.4 w.r.t. the table viewer and cell editors? PW I don't know; this is new functionality we are putting in now. Adding Tom for his knowledge of Cell Editors. PW I don't know; this is new functionality we are putting in now. Is there a combination of class types/arguments/flags that should/should not be used and I am overlooking? I have not found any references to this behavior in the javadoc. Well the thing you are seeing is that the visual selection information is removed from the control by the FocusCellOwnerDrawHighlighter. If you query the underlying control (getSelection()) you'll notice that the selection is still there. It boils down to the fact that you'll have to write your own FocusCellOwnerDrawHighlighter which is not removing the selection information but marks the focus cell differently. The problem we are having with this keyboard navigation stuff is how keyboard navigation works in a multi-selection mode (what happens if the focus-cell is moving to a row which is not selected, ....) we are not facing in single selection. The bug requesting this feature is bug #206692. At least we should fix the JavaDoc as suggested there describing the current situation. Well, I 'd say that cell focus moving out of the selected rows area is by itself fine. It is the modifyCell event that should (maybe) clear the selected rows. Or maybe not. I see no problem in having several rows highlighted as well as some cell highlighted too, especially if cell focus can draw focus rectangle around selected cell (and/or use different background color). There are some operations in the table (and tree) that are applicable to the selection of rows (one or more) and then there are operations that are applicable to cells only. So having both things is fine. Writing my own FocusCellOwnerDrawHighlighter does not look like a good idea; I would prefer that existing one to support both behaviors. This could ensure that every table and tree have similar look and feel. (Plus, looking at FocusCellOwnerDrawHighlighter source code, there are some calls to methods that are not visible if I wanted to write/replicate the existing behavior). People could overwrite getSelectedCell*Color methods to return some color to distinguish cells from rows, or maybe there would some default (non-null) colors for that. > Writing my own FocusCellOwnerDrawHighlighter does not look like a good idea; I
> would prefer that existing one to support both behaviors. This could ensure
> that every table and tree have similar look and feel. (Plus, looking at
> FocusCellOwnerDrawHighlighter source code, there are some calls to methods that
> are not visible if I wanted to write/replicate the existing behavior). People
> could overwrite getSelectedCell*Color methods to return some color to
> distinguish cells from rows, or maybe there would some default (non-null)
> colors for that.
>
We could but this is M6 week and we are in API-freeze mode - we also can't simply modify the current behaviour because others might depend on the current one. I'll try to take look later or tomorrow my time how such a highlighter could look like. Boris, Paul?
Tom
I don't think we should try to squeeze something in for M6. Stability is more important than feature completeness. We need to document that SWT.MULTI is not supported when you add cell navigation. Boris, The multi-select is supported, it is just not visible. Tom, The default behavior can be as it is today. If there is a flag that flips this, I could use it to have a look-and-feel that I need (i.e. make selected rows visible). Created attachment 128469 [details]
API to control the background of unselected cells
Please note this is not the final solution because inconjunction with Keyboard navigation there are still problems. Boris I thinkl it would be save to include this as API in M6.
Created attachment 128470 [details]
Test snippet
Alex: I also took a look at the implementation. There's only one call to an protected method which and you could use reflection to call it. Hitesh is now responsible for watching bugs in the [Viewers] component area. It doesn't look like this change has been added to 3.5. How about 3.6? What's the best way to build a custom jface plugin that includes this change? I'd like to have this feature for my RCP app. Created attachment 197612 [details]
FocusCellHighlighter that can handle multiselection and keyboard navigation (HACK!)
I created my own FocusCellHighlighter that can handle multiselection and keyboard navigation. Maybe somebody can use it. ATTENTION: this is a hack, I needed to use reflection to access a protected method from the ColumnViewer.
I was having the same issue. Thank you for your "hack", it seems to work fine. It also solves the issue I had that even with single selection, the selected line was not highlighted, at least not very clearly. (In reply to comment #11) > Created attachment 128469 [details] > API to control the background of unselected cells > Tom, is this still a viable solution for this problem? PW Well you know this is from nearly 3 years ago - the API looks save to me although not removing the selection at all as done in the FocusHighlighter (HACK!) is probably even better (we could control this using a style-constant or boolean) Looking at the custom FocusCellHighlighter (HACK!) i think it can be rewritten to use real already existing API because one can fetch the ViewerRow using ColumnViewer#getCell(Point).getViewerRow() and the point can be extracted using TableItem#getBounds(int) - though the performance might not be 100% ok. Maybe adding an API that allows one to retrieve the ViewerRow based on the Domain-Object though makes more sense so we could add an API like ColumnViewer#getViewerRow(Object). I'll try to look into this next week - if no progress is made please ping the bug there are many things on my schedule and I can't spend any company time on this. (In reply to comment #19) > Well you know this is from nearly 3 years ago - the API looks save to me > although not removing the selection at all as done in the FocusHighlighter > (HACK!) is probably even better (we could control this using a style-constant > or boolean) > > Looking at the custom FocusCellHighlighter (HACK!) i think it can be rewritten > to use real already existing API because one can fetch the ViewerRow using > ColumnViewer#getCell(Point).getViewerRow() and the point can be extracted using > TableItem#getBounds(int) - though the performance might not be 100% ok. > > Maybe adding an API that allows one to retrieve the ViewerRow based on the > Domain-Object though makes more sense so we could add an API like > ColumnViewer#getViewerRow(Object). > > I'll try to look into this next week - if no progress is made please ping the > bug there are many things on my schedule and I can't spend any company time on > this. Hello Tom Have you had the chance to continue the work on this bug? Created attachment 211087 [details]
patch v0.1
I think the patch created by Dominik can be implemented without reflection now. The current API allows to access getViewerRowFromItem() method. Could you please check if this patch is working for you?
Patch v0.1 looks ok Is there any update on this? Any ideas on when this patch will be implemented in the API? Any news on this? Having the same problem. (In reply to Klara Ward from comment #25) > Any news on this? Having the same problem. It would be useful if someone who is interested in the fix, will provide a Gerrit review based on patch from comment 21. Does Klara need to be an Eclipse committer to review this? (In reply to Marcus Hirt from comment #27) > Does Klara need to be an Eclipse committer to review this? From what I understand, a committer needs to create a Gerrit review? You should have the approval to sign the ECA in you mailbox. :) My ECA is approved. Looking into creating a Gerrit review. My ECA is approved. Looking into creating a Gerrit review. Gerrit review at: https://git.eclipse.org/r/#/c/104279/ (Had thought it would be automatically added to this bug) (In reply to Klara Ward from comment #32) > Gerrit review at: https://git.eclipse.org/r/#/c/104279/ > (Had thought it would be automatically added to this bug) Only if you would have written "Bug" ;-) I'll try to get to this end of the week - please ping the bug if nothing has happened until next week (In reply to Thomas Schindl from comment #33) > (In reply to Klara Ward from comment #32) > > Gerrit review at: https://git.eclipse.org/r/#/c/104279/ > > (Had thought it would be automatically added to this bug) > > Only if you would have written "Bug" ;-) I tried to amend that, but not sure it helped :) (In reply to Thomas Schindl from comment #33) > (In reply to Klara Ward from comment #32) > > Gerrit review at: https://git.eclipse.org/r/#/c/104279/ > > (Had thought it would be automatically added to this bug) > > Only if you would have written "Bug" ;-) I tried to amend that, but not sure it helped :) (In reply to Thomas Schindl from comment #34) > I'll try to get to this end of the week - please ping the bug if nothing has > happened until next week Thanks. I tried to add a test, but it proved quite difficult, the color does not change after programmatic selection, or maybe I messed up. Should there also be a new snippet? (In reply to Klara Ward from comment #37) > I tried to add a test, but it proved quite difficult, the color does not > change after programmatic selection, or maybe I messed up. Did you try to create and send an SWT event to mimic a user action and trigger all listeners? (In reply to Mickael Istria from comment #38) > (In reply to Klara Ward from comment #37) > > I tried to add a test, but it proved quite difficult, the color does not > > change after programmatic selection, or maybe I messed up. > > Did you try to create and send an SWT event to mimic a user action and > trigger all listeners? Nope, did not try that. Will make an attempt. I found no other tests that change a selection and check the color, but maybe they are in other projects than eclipse.platform.ui? Gerrit change https://git.eclipse.org/r/104279 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=da427eaeb7c25a1a5f391c0d3fc043e91027b7d8 You probably didn't install the API baseline and hence did not see the error about the missing @since tag on the new API. Please fix this. What should the since version be? (In reply to Klara Ward from comment #42) > What should the since version be? As said before, you should set the API Baseline. Out of the box you should have an error on the project saying that you have to do that. Once you set it, it will give you the error about the missing @since tag and also offer a Quick Fix to add the correct tag. New Gerrit change created: https://git.eclipse.org/r/110983 Gerrit change https://git.eclipse.org/r/110983 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=679a28a9a149a4a5985121f848cc95b50069f39a (In reply to Klara Ward from comment #42) > What should the since version be? Klara, here is some documentation about the API baseline. If you need more info, please let me know, I'm happy to extend the description. http://www.vogella.com/tutorials/EclipsePlatformDevelopment/article.html#configure-api-baseline (In reply to Lars Vogel from comment #46) > (In reply to Klara Ward from comment #42) > > What should the since version be? > > Klara, here is some documentation about the API baseline. If you need more > info, please let me know, I'm happy to extend the description. > > http://www.vogella.com/tutorials/EclipsePlatformDevelopment/article.html#configure-api-baseline > Why do you reopen the fixed bug? Sorry my fault - but I thought that OOMPH correctly setup the complete eclipse with a base-line. I must have failed that Oomph step somehow. Will make a new attempt. Thanks Andrey for fixing. I don't think you failed a step because i tested your patch in a oomph workspace and did not see an error |
Build ID: 3.4 Steps To Reproduce: 1. Create Table and TableViewer with couple of columns. Use SWT.MULTI|SWT.H_SCROLL|SWT.V_SCROLL|SWT.FLAT|SWT.FULL_SELECTION flags. 2. Add CellModifiers for each column. 3. Add this code to add cell focus support and enable keyboard navigation: TableViewer tv = (TableViewer) viewer; ColumnViewerEditorActivationStrategy cveas = new ReqActivationStrategy(tv); CellNavigationStrategy cns = new CellNavigationStrategy(); TableViewerFocusCellManager tvfcm = new TableViewerFocusCellManager( tv, new FocusCellOwnerDrawHighlighter( tv )); TableViewerEditor.create(tv, tvfcm, cveas, ColumnViewerEditor.TABBING_HORIZONTAL|ColumnViewerEditor.KEYBOARD_ACTIVATION ); where ReqActivationStrategy is protected class ReqActivationStrategy extends ColumnViewerEditorActivationStrategy { public ReqActivationStrategy(ColumnViewer viewer) { super(viewer); } @Override protected boolean isEditorActivationEvent( ColumnViewerEditorActivationEvent event) { if( super.isEditorActivationEvent(event)) return true; if( event.eventType == ColumnViewerEditorActivationEvent.KEY_PRESSED ) { return event.keyCode == '\r'; } return false; } }; More information: Problem: -------- Table stops support multi-select. Selecting multiple rows is impossible.