Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 325230

Summary: Table and Tree with SWT.SINGLE should not allow user to deselect item
Product: [Eclipse Project] Platform Reporter: Markus Keller <markus.kell.r>
Component: SWTAssignee: Scott Kovatch <skovatch>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: daniel_megert, eclipse.felipe, schulz.johan, Silenio_Quarti
Version: 3.7   
Target Milestone: 3.7 M3   
Hardware: PC   
OS: Mac OS X   
Whiteboard:
Bug Depends on:    
Bug Blocks: 325223    
Attachments:
Description Flags
Fix for Tree, List, & Table none

Description Markus Keller CLA 2010-09-14 07:29:34 EDT
I20100914-0100 Cocoa

Table and Tree with SWT.SINGLE should not allow the user to deselect the selected item. On Windows, deselecting the item is not possible (bug 88288), and the platform inconsistency is causing trouble (bug 325223).
Comment 1 Markus Keller CLA 2010-09-14 07:32:22 EDT
> causing trouble (bug 325223).

In 3.7 M1, this was not an issue, since the SWT.Selection event didn't get through when the table's shell didn't have focus (but MouseDown/MouseUp were delivered). Now, the selection event is also delivered in our scenario.
Comment 2 Dani Megert CLA 2010-09-14 10:32:50 EDT
We'd like to see this fixed for M2 if possible. If not, please let us know asap, so that we can add a workaround on our side.
Comment 3 Scott Kovatch CLA 2010-09-14 12:50:50 EDT
Bug 88288 was closed as WONTFIX, but I disagree with the analysis that 'once a Table has selection it always has a selection' is (or should be) platform behavior. That's water under the bridge, I guess. 

This is easy to fix, however. [NSTableView setAllowsEmptySelection:NO] should do the job. Give me an hour or so to test Table and Tree.
Comment 4 Markus Keller CLA 2010-09-14 13:05:53 EDT
(In reply to comment #3)
+1. It's always bad if platform differences shine through SWT APIs.

> This is easy to fix, however. [NSTableView setAllowsEmptySelection:NO] should
> do the job. Give me an hour or so to test Table and Tree.

I didn't try it with setAllowsEmptySelection:NO, but the documentation says you cannot even set an empty selection programmatically any more. But on Windows and GTK, that's still possible (and a SINGLE table has nothing selected initially).
Comment 5 Scott Kovatch CLA 2010-09-14 14:13:04 EDT
Created attachment 178861 [details]
Fix for Tree, List, & Table

Implemented a delegate method that allows you to modify the selection. I want to check with Silenio if it should still be possible to clear the selection programmatically, though.
Comment 6 Silenio Quarti CLA 2010-09-14 14:22:36 EDT
We should fix this after 3.7 M2 given that we need more testing.
Comment 7 Scott Kovatch CLA 2010-09-14 16:23:38 EDT
(In reply to comment #4)
> I didn't try it with setAllowsEmptySelection:NO, but the documentation says you
> cannot even set an empty selection programmatically any more. But on Windows
> and GTK, that's still possible (and a SINGLE table has nothing selected
> initially).

Right -- I hit that early on and wound up taking another path. The attached patch works for user clicks and programmatic selection and deselection.
Comment 8 Dani Megert CLA 2010-09-15 01:34:31 EDT
(In reply to comment #6)
> We should fix this after 3.7 M2 given that we need more testing.
Agreed. Markus, please release the workaround for M2.
Comment 9 Scott Kovatch CLA 2010-09-20 18:38:04 EDT
Fixed > 20100920
Comment 10 Silenio Quarti CLA 2010-09-21 10:36:55 EDT
I took a closer look at the patch. Isn't the NSIndexSet allocated in selectionIndexesForProposedSelection() leaking?
Comment 11 Scott Kovatch CLA 2010-09-21 11:08:41 EDT
(In reply to comment #10)
> I took a closer look at the patch. Isn't the NSIndexSet allocated in
> selectionIndexesForProposedSelection() leaking?

Yep, you're right. They need an autorelease before being returned. Fixed.
Comment 12 Dani Megert CLA 2010-09-23 08:00:36 EDT
I've reverted the workaround in Platform Text.

Scott, I don't have a Mac at hand, can you verify in tomorrows build (N20100923-2000) that bug 325223 is gone?
Comment 13 Scott Kovatch CLA 2010-09-23 12:36:49 EDT
(In reply to comment #12)
> Scott, I don't have a Mac at hand, can you verify in tomorrows build
> (N20100923-2000) that bug 325223 is gone?

Okay, if you tell me where I can find it in the UI. I looked in the 3.7M2 N&N but didn't see any new features that look like what's going on in 325223.
Comment 14 Dani Megert CLA 2010-09-24 03:04:38 EDT
(In reply to comment #13)
> (In reply to comment #12)
> > Scott, I don't have a Mac at hand, can you verify in tomorrows build
> > (N20100923-2000) that bug 325223 is gone?
> 
> Okay, if you tell me where I can find it in the UI.
Sure. Paste the following code into the Package Explorer:
-- %< ---
class C  {
	public String toString() {
		return "";
	}
}
-- %< ---
Now click on "toString" while holding down the Command modifier. This should bring up a little shell with two options. Click on one of them to jump to that method. In I20100914-0100 (had bug) it did not jump but simply deselect the item.

> I looked in the 3.7M2 N&N  but didn't see any new features that look like 
> what's going on in 325223.
It's not a new feature but one that got broken by changes in SWT.
Comment 15 Dani Megert CLA 2010-09-28 06:08:58 EDT
Scott, were my steps clear enough to verify it?
Comment 16 Scott Kovatch CLA 2010-09-28 11:36:22 EDT
Yes, sorry about that. Eclipse.org downloads have been especially slow lately. I verified it with N20100926-2000 just now.
Comment 17 Felipe Heidrich CLA 2011-04-18 14:18:44 EDT
Part for the fix for this problem (according to CVS log) was to call translateTraversal() from inside Control#insertText(), this does not show in the patch and does not (apparently) relates to problem described in here. Maybe the code was released with the wrong comment in it. Anyhow, this code is causing bug 343093 (input method is broken for single line text).
Comment 18 Felipe Heidrich CLA 2011-04-19 12:22:58 EDT
(In reply to comment #17)
> Part for the fix for this problem (according to CVS log) was to call
> translateTraversal() from inside Control#insertText(), this does not show in
> the patch and does not (apparently) relates to problem described in here. Maybe
> the code was released with the wrong comment in it. Anyhow, this code is
> causing bug 343093 (input method is broken for single line text).

The log in the bugzilla is wrong, it reads:
325230 - check for translateTraversal in insertText.
but it should be:
325222 - check for translateTraversal in insertText.