Bug 324254

Comment 1 Prakash Rangaraj 2010-09-01 23:45:59 EDT

Merely enabling the menu doesn't help. If you select a text, you expect text related services to be enabled in the Services (like 'TextEdit->New Window containing selected text' should be enabled)

Unless we contribute/access the Services menu, I don't see a point in enabling it.

Comment 2 Brian de Alwis 2010-09-02 12:56:31 EDT

(In reply to comment #1)
> Merely enabling the menu doesn't help. If you select a text, you expect text
> related services to be enabled in the Services (like 'TextEdit->New Window
> containing selected text' should be enabled)

Actually, enabling it does exactly that.

For example, I frequently select text and then use the "Open URL" and "Make New Sticky Note" services.  You can't do this in an Eclipse app since the Services menu is disabled.  After removing the questionable services-disbling code, these services (and others) work fine.

> Unless we contribute/access the Services menu, I don't see a point in enabling
> it.

We're currently preventing the *user* from accessing it.

Comment 3 Prakash Rangaraj 2010-09-02 13:20:23 EDT

Created attachment 178071 [details]
Patch v01

(In reply to comment #2)
> (In reply to comment #1)
> > Merely enabling the menu doesn't help. If you select a text, you expect text
> > related services to be enabled in the Services (like 'TextEdit->New Window
> > containing selected text' should be enabled)
> 
> Actually, enabling it does exactly that.

    I'm trying to enable the Services menu using the attached patch. When I have some text selected in the text editor none of the services are enabled. How are you enabling the Services menu?

Comment 4 Brian de Alwis 2010-09-02 13:42:02 EDT

(In reply to comment #3)
>     I'm trying to enable the Services menu using the attached patch. When I
> have some text selected in the text editor none of the services are enabled.
> How are you enabling the Services menu?

Huh.  I think that must be an issue with StyledText widgets.

I've been doing my testing with an e4 RCP app, where I used plain SWT Text widgets, and it works fine.  But I then tried against R3.6 and, when selecting text entered into a text editor or the JDT/Debug's Display view, there are no applicable services.  If you use a normal text field (e.g., the filter box in the preferences) then the full suite of text services are available.

Comment 5 Prakash Rangaraj 2010-09-02 13:53:36 EDT

(In reply to comment #4)
> Huh.  I think that must be an issue with StyledText widgets.

    Yup! Got it now. It works when normal text widgets have selection.

I'm cc-ing Scott & Lakshmi. They are the Cocoa experts.

Comment 6 Scott Kovatch 2010-09-02 14:36:11 EDT

StyledText is a Composite and therefore doesn't use an NSTextView. To get Services menu requests we need to implement three methods:

validRequestorForSendType:(NSString *)sendType returnType:(NSString *)returnType
writeSelectionToPasteboard:types: (NSServicesRequests protocol)
readSelectionFromPasteboard: (NSServicesRequests protocol)

Moving this into SWT. This would be a very useful thing to support, so I'm going to try to get to this for 3.7.

Comment 7 Brian de Alwis 2010-09-02 16:19:38 EDT

Created attachment 178096 [details]
Patch v02 (platform-ui)

Alternative patch: just get rid of kServicesMenuItem and its uses.

Comment 8 Brian de Alwis 2010-09-02 16:20:09 EDT

Created attachment 178097 [details]
Patch v02 (e4)

Equivanelt patch for e4 codebase

Comment 9 Scott Kovatch 2010-09-03 20:08:22 EDT

Please don't do this. I have a fix for the root problem, which is to add the methods I mentioned in comment #6.

(In reply to comment #7)
> Created an attachment (id=178096) [details]
> Patch v02 (platform-ui)
> 
> Alternative patch: just get rid of kServicesMenuItem and its uses.

Comment 10 Scott Kovatch 2010-09-03 20:14:48 EDT

Created attachment 178206 [details]
Fix to enable Services for StyledText

Prakash, to completely fix this we need to stop disabling the Services menu. Cocoa completely controls whether the menu can be enabled or disabled.

This patch implements the three methods needed to pick up the current selection from a StyledText and pass it to the selected service. Services that read the text and do something with it work fine. I'm still looking for a service that writes something back to the view.

Comment 11 Brian de Alwis 2010-09-03 20:22:48 EDT

(In reply to comment #10)
> Prakash, to completely fix this we need to stop disabling the Services menu.
> Cocoa completely controls whether the menu can be enabled or disabled.

That's exactly the purpose of the two patches you just marked as obsolete  ;-)
 
> This patch implements the three methods needed to pick up the current selection
> from a StyledText and pass it to the selected service. Services that read the
> text and do something with it work fine. I'm still looking for a service that
> writes something back to the view.

This site has several URL shortners (e.g., bit.ly, is.gd) that both reads and writes back:

   http://taotek.com/products.php

Comment 12 Scott Kovatch 2010-09-03 20:57:24 EDT

(In reply to comment #11)
> (In reply to comment #10)
> > Prakash, to completely fix this we need to stop disabling the Services menu.
> > Cocoa completely controls whether the menu can be enabled or disabled.
> 
> That's exactly the purpose of the two patches you just marked as obsolete  ;-)

I'm sorry -- I completely misread the patch. I'll fix that. :-)

I also found a way to wrap a URL shortener into a service, so I'll attach that for testing.

Comment 13 Scott Kovatch 2010-09-03 20:59:42 EDT

Created attachment 178207 [details]
Corrected fix

Found a bug when writing the string back into the Canvas.

Comment 14 Scott Kovatch 2010-09-03 21:01:26 EDT

Created attachment 178208 [details]
URL shortener for testing

Here's an Automator service I wrote that calls a URL shortener on the selected text.

Comment 15 Scott Kovatch 2010-09-03 21:07:30 EDT

I will commit this on Tuesday and then assign the bug over to Prakash for the platform-ui change.

Comment 16 Scott Kovatch 2010-09-07 12:17:56 EDT

After review it was pointed out that referring to StyledText directly from the native SWT components is a no-no, so I'm going to have to rework this a bit. I can use the accessibility API to get the current selection but there's no way to set it until we implement IAccessible2EditableText support. (bug 324005)

The alternative is to implement this at the UI level in the CocoaUIEnhancer, which is above the SWT in the architecture hierarchy. Then it could do the 'instanceof StyledText' check, but that would involve adding the Cocoa methods to SWTCanvasView on the fly.

In any event, you definitely should un-disable the services menu.

Comment 17 Brian de Alwis 2010-09-09 12:12:06 EDT

Patch v02 committed to both codebases.

Comment 18 Scott Kovatch 2010-10-15 19:34:42 EDT

Created attachment 181025 [details]
Fix using accessibility

Patch uses accessibility APIs to get and set the selected text. I'll mark this fixed when StyledText implements AccessibleEdtableTextListener.

Comment 19 Scott Kovatch 2010-10-15 19:37:56 EDT

Fixed > 20101015, but waiting on StyledText change.

Comment 20 Scott Kovatch 2011-01-31 12:47:27 EST

329216 is now fixed, so this bug can now be marked as fixed.