Community
Participate
Working Groups
Build Identifier: 3.7 Eclipse Indigo Providing content assists for the fields e.g. for Java typed fields we provide content assists that will pull out the Java classes and packages We can refer to the PDE Manifest editor Right now Sapphire doesn't allow us to bind CTRL+SPACE to provide content assist for fields Reproducible: Always Steps to Reproduce: 1. Add CTRL+SPACE to a Action 2. Invalid Key Sequence message is displayed 3.
Doc on Eclipse content proposals facility for text fields: http://help.eclipse.org/helios/index.jsp?topic=/org.eclipse.platform.doc.isv/guide/jface_fieldassist.htm This change should modify DefaultValuePropertyEditorRenderer to provide hookup to the text field. The hookup should be conditioned on availability of PossibleValuesService, where the content proposals should draw its values from. This will ensure that this feature will work regardless of the nature of the content. This is your item to develop if you'd like and unless I hear otherwise.
Yeah will do this development too but not sure I can commit this for 0.4, but will try to see if I can push it for 0.5 Hope that's fine
Sure. Setting the target to Future for now.
I was trying to do an analysis and mock implementation of it and found that your suggestion of having hooked to PossibleValuesService makes sense and it worked quiet spelendidly and i was able to successfully hook CONTROL+SPACE for all non-java type properties I am having tough time hooking up JavaType, when I define a PossibleValuesService to find out all JavaTypeName my editor is now having two "Browse" buttons one for each JavaType Selection Dialog and one for my custom "PossibleValueService" - as per my understanding this not a right behaviour, not sure how can we avoid this My requirement is to keep the change to the DefaultValuePropertyRendere to bare minimum and not introduce any extra plugins to MANIFEST - incase I need to handle the Java Type Search in DefaultValuePropertyRenderer then its becoming clumsy with all addtional stuff like adding JDT plugins to MANIFEST and checking for @Reference, @Type etc., apart form @PossibleValueService Please share your thoughts. Also please change the status to assigned as I have started to work on it, if things goes well i can push this for 0.4 itself.
Yeah... Relying on PossibleValuesService to handle Java content proposals isn't going to work. Not only is there no PossibleValueService implementation for JavaName properties, but the PossibleValueService API isn't tuned for handling large data sets. I would be inclined to introduce another type of service here based on the API of JFace IContentProposalProvider. Let's call it ContentProposalService for now. http://help.eclipse.org/helios/index.jsp?topic=/org.eclipse.platform.doc.isv/reference/api/org/eclipse/jface/fieldassist/IContentProposalProvider.html There would then be two implementations of this service. One based on the content from PossibleValuesService and the other based on JDT API. You should be able to find JDT's implementation of IContentProposalProvider and wrap it to ensure that the behavior is identical to other Java content assist sites.
(In reply to comment #5) > Yeah... Relying on PossibleValuesService to handle Java content proposals isn't > going to work. Not only is there no PossibleValueService implementation for > JavaName properties, but the PossibleValueService API isn't tuned for handling > large data sets. I would be inclined to introduce another type of service here > based on the API of JFace IContentProposalProvider. Let's call it > ContentProposalService for now. > > http://help.eclipse.org/helios/index.jsp?topic=/org.eclipse.platform.doc.isv/reference/api/org/eclipse/jface/fieldassist/IContentProposalProvider.html > Where do want the service to be placed ? in org.eclipse.sapphire.ui ? I need to follow the same Service Design pattern followed similar to @InitialValue service ? > There would then be two implementations of this service. One based on the > content from PossibleValuesService and the other based on JDT API. You should be > able to find JDT's implementation of IContentProposalProvider and wrap it to > ensure that the behavior is identical to other Java content assist sites. If I am going to add this to the org.eclipse.sapphire.ui then it might require us to add additional jdt related plugins to he MANIFEST, Are you OK, let me know otherwise Assume that we have created this service,then the DefaultValuePropertyEditorRenderer should check fo the existence of this ContentProposalService and add the Control +Space Key binding, correct me if I am wrong.
Are you OK in sharing UML diagrams describing this hiearchy, i feel comfortable using UML and helps me in understanding better.?
> Where do want the service to be placed ? in org.eclipse.sapphire.ui ? > I need to follow the same Service Design pattern followed similar to > @InitialValue service ? Yes to both. > If I am going to add this to the org.eclipse.sapphire.ui then it might require > us to add additional jdt related plugins to he MANIFEST, Are you OK, let me > know otherwise No, this dependency is not ok. The reason for introducing a new service API here is to not introduce that dependency. You put service API in sapphire.ui. The service implementation that uses PossibleValuesService goes in sapphire.ui.internal. The implementation that uses JDT API goes in sapphire.java.jdt.ui. > Assume that we have created this service,then the > DefaultValuePropertyEditorRenderer should check fo the existence of this > ContentProposalService This part is correct. > and add the Control +Space Key binding This is somewhat correct. I do not believe you will be adding key binding yourself. You would be using JFace content assist API which takes care of key binding, etc.
> Are you OK in sharing UML diagrams describing this hiearchy, i feel comfortable > using UML and helps me in understanding better.? I do not normally use UML, but if it would help you to run UML diagrams by me for review, then certainly go for it. Just make sure to attach an image of the diagram as I do not have any UML software installed.
I have attached the UML diagram based on this discussion, please review and let me know your comments. There are two implemetations PossibleValuesContentProposal and JavaTypeContentProposal PossibleValuesContentProposal will have PossibleValueService injected to it ( we shall read the annotation and set the value) JavaTypeContentProposal will have the JavaTypeConstraints and JavaTypeKind injected to it ( again we shall read the annotations of the model property to set value) Both will implement the getProposals() method of IContentProvider and any of these implementations could be passed to contentProposalAdapter in the DefaultValuePropertyRender for attaching the Proposal to the underlying textField. Hope i have got he design right. please review and let me know your comments. Based on that I will start implementing the changes
Created attachment 202348 [details] Class Diagram ClassDiagram for ContentService and its implementations
1. AbstractContentProposalService should be just ContentProposalService. We aren't using "prefix Abstract" to abstract classes convention in Sapphire. 2. It looks like AbstractContentProposalService implements JFace IContentProposalProvider. That's not the approach that I'd like to take as the long-term plan for Sapphire UI is to be agnostic of rendering technology (like SWT/JFace). That is, we would have multiple renderers. As such, ContentProposalService API and implementation should not reference any JFace classes. Instead, inside DefaultValuePropertyRender, you should have implementation of IContentProposalProvider that delegates to the generic ContentProposalService API. Makes sense?
you mean to day that there shud be a private static inner class within DefaultValuePropertyRender which should implement IContentProposalProvider and getProposals method of this inner class should delegate the call to the ContentProposalService.proposals() method. Is my understanding right ?
(In reply to comment #13) > you mean to day that there shud be a private static inner class within > DefaultValuePropertyRender which should implement IContentProposalProvider and > getProposals method of this inner class should delegate the call to the > ContentProposalService.proposals() method. > Is my understanding right ? Yes. Also, since proposals() method returns non-primitive objects, you would need to marshall those objects as well to form expected by JFace.
(In reply to comment #14) > (In reply to comment #13) > > you mean to day that there shud be a private static inner class within > > DefaultValuePropertyRender which should implement IContentProposalProvider and > > getProposals method of this inner class should delegate the call to the > > ContentProposalService.proposals() method. > > Is my understanding right ? > > Yes. Also, since proposals() method returns non-primitive objects, you would > need to marshall those objects as well to form expected by JFace. 1.Here you mean to say prposals() should return IContentProposal[] which is expected by JFace ? If thats the case we will have to give two implementations of IContentProposal one for each service PossibleValueContentPorposal and JavaTypeNameContentProposal , these two can again be inner classes for the respctive implementation of ContentProposalService Or 2.If you want to avoid using IContentProposal[] then we shall model ValueObject based on that calleds ContentProposalInfo which will have all the elements required by IContentProposal, and porposals method return a List<ContentProposalInfo> which will be iterated by the DefaultValuePropertyRender$ContentProposalProvider.getProposals() method and return the IContentProposal[] by building JFace#SimpleContentProposal() objects I feel you intended me to do the #2 correct? Let me know otherwise
I meant option #2. The ContentProposal object should not have any dependency on JFace. It should be a final class as it's just data. There should not be any need for ContentProposalService implementations to customize this class.
If am not wrong we also need to introduce an annotation @ContentProposal which will have one attribute called impl which can point to either PossaibleContentProposalService or JavaTypeContentProposalService So in our DefaultValuePropertyEditorRenderer we will check for this annoation and extract the impl from it and pass it to the ContentProposalProvider we have in DefaultValuePropertyEditorRenderer We might also need to register this service via the Sapphire extension . This annotation will be placed in sapphire.ui Please share your thoughts
(In reply to comment #17) > If am not wrong we also need to introduce an annotation @ContentProposal which > will have one attribute called impl which can point to either > PossaibleContentProposalService or JavaTypeContentProposalService > > So in our DefaultValuePropertyEditorRenderer we will check for this annoation > and extract the impl from it and pass it to the ContentProposalProvider we have > in DefaultValuePropertyEditorRenderer > > We might also need to register this service via the Sapphire extension . This > annotation will be placed in sapphire.ui > > Please share your thoughts The annotation isn't necessary. The implementations of ContentProposalService should be registered in sapphire-extension.xml from their respective plugin. The service factory will then control applicability. See how other services registered in sapphire-extension.xml are implemented. Model writers should not need to be aware of this service.
I understand it now. Sorry I was bit confused. when doing ProposalValuesContentProposalService we need to consider the following sceanrios for applicability , either PossibleValueService or PossibleValues + RefrenceService ? Also the services should be registered at the ValuePropertyInstance context ? Please let me know if my understanding is wrong.
A small correction in my previous comment "when doing ProposalValuesContentProposalService we need to consider the following sceanrios for applicability , either PossibleValueService or PossibleValues + RefrenceService ?" it should be "when doing ProposalValuesContentProposalService we need to consider the following sceanrios for applicability , either PossibleValueService or PossibleValues ?
Created attachment 202376 [details] Analysis of Impacted and New Changes The attached patch has the changes to be for this bug fix, the classes are RAW and am working on the refinement, but i want your review on the same to gauge the changes and let me know if I am heading in right direction, just to avoid any pitfalls and multiple iteration down the lane.
Created attachment 202377 [details] mylyn/context/zip
> when doing ProposalValuesContentProposalService we need to consider the > following sceanrios for applicability , either PossibleValueService or > PossibleValues No. Just PossibleValuesService as the framework takes care of converting @PossibleValues annotation into a PossibleValueService for you. I didn't take a deep dive into this iteration, but it looks like you are on the right track. Here are a few comments... 1. In JavaTypeContentProposalService.Factory you should use ModelProperty.isOfType instead of fetching @Type annotation, etc. 2. In DefaultValuePropertyEditorRenderer, skip the part of adding ControlDecoration as it will clash with Sapphire's existing property editor feedback marker. 3. ContentProposalInfo should be an immutable class. 4. Shouldn't ContentProposalService.proposals() take existing string plus cursor position to match IContentProposalProvider? 5. In PossibleValuesContentProposalService.proposals(), I would expect to see filtering of proposals. Computing the static set at init time isn't correct.
(In reply to comment #23) > > when doing ProposalValuesContentProposalService we need to consider the > > following sceanrios for applicability , either PossibleValueService or > > PossibleValues > > No. Just PossibleValuesService as the framework takes care of converting > @PossibleValues annotation into a PossibleValueService for you. > Gotcha! I was thinking about it and your confirmed :) > I didn't take a deep dive into this iteration, but it looks like you are on the > right track. Here are a few comments... > > 1. In JavaTypeContentProposalService.Factory you should use > ModelProperty.isOfType instead of fetching @Type annotation, etc. > Can you please tell me the difference just for the knowledge sake. > 2. In DefaultValuePropertyEditorRenderer, skip the part of adding > ControlDecoration as it will clash with Sapphire's existing property editor > feedback marker. > Yeah I agree but how do we display a tip (the traditonal orange bulb like icon) to tell the user that we have content assist? > 3. ContentProposalInfo should be an immutable class. > You mean no setters only getters ? But why so since we are not going to set any values atfer initialization ? > 4. Shouldn't ContentProposalService.proposals() take existing string plus cursor > position to match IContentProposalProvider? > > 5. In PossibleValuesContentProposalService.proposals(), I would expect to see > filtering of proposals. Computing the static set at init time isn't correct. Can we not do the filtering this way, I have flag in DefaultValuePropertyEditorRenderer#ContentProvider, will delegate the same to this class, am thiking something like init method fetches all the values and proposals method will filter it based on the flag will just alter the proposals() to proposals(content,filtered) ? does i sound sensible ? and it annswers your #4
One thing thats still unclear for me is , how can we specify two @Service for a property, the logic right now we are using in DefaultValuePropertyEditorRenderer is checking for ContentProposalService for adding content assist, which in turn uses PossibleValueService .. how to make this hook work ?
(In reply to comment #25) > One thing thats still unclear for me is , how can we specify two @Service for a > property, the logic right now we are using in DefaultValuePropertyEditorRenderer > is checking for ContentProposalService for adding content assist, which in turn > uses PossibleValueService .. how to make this hook work ? I found the the clue, a matter of i dont even need to add another @Service, it gets automatically hooked by the famework, the mistake i made last time was the the context, i have specified at the Property Instance level rather than Property Model level After i chaged it to Sapphire.Property.MetaModel it worked. Can you please explain the contexts with respect to the services ?
>> 1. In JavaTypeContentProposalService.Factory you should use >> ModelProperty.isOfType instead of fetching @Type annotation, etc. >> >Can you please tell me the difference just for the knowledge sake. Same result, except the framework already processed that annotation for you. >> 2. In DefaultValuePropertyEditorRenderer, skip the part of adding >> ControlDecoration as it will clash with Sapphire's existing property >> editor feedback marker. >> >Yeah I agree but how do we display a tip (the traditonal orange bulb like icon) >to tell the user that we have content assist? Will likely need to integrate the hint with the existing feedback marker. Let's set aside this aspect for now. Once the rest is done, we can brainstorm. >> 3. ContentProposalInfo should be an immutable class. >> >You mean no setters only getters ? But why so since we are not going to set >any values atfer initialization ? Since it makes no sense for the caller of ContentProposalService to call these setters, it would make for cleaner API if the object is immutable once returned by the service. >> 4. Shouldn't ContentProposalService.proposals() take existing string >> plus cursor position to match IContentProposalProvider? >> >> 5. In PossibleValuesContentProposalService.proposals(), I would expect >> to see filtering of proposals. Computing the static set at init time isn't >correct. >Can we not do the filtering this way, I have flag in >DefaultValuePropertyEditorRenderer#ContentProvider, will delegate the same to >this class, am thiking something like init method fetches all the values and >proposals method will filter it based on the flag will just alter the >proposals() to proposals(content,filtered) ? does i sound sensible ? and it >annswers your #4 The reason you do filtering when calculating proposals is that frequently enumerating the entire space of possibilities is exceptionally expensive. Consider the Java type example. There could be tens of thousands (if not hundreds of thousands of types) visible in a given project. Take a look at how JDT implements IContentProposalProvider. We should be delegating to that implementation in JavaTypeContentProposalService. >> One thing thats still unclear for me is , how can we specify two >> @Service for a property, the logic right now we are using in >> DefaultValuePropertyEditorRenderer >> is checking for ContentProposalService for adding content assist, >> which in turn uses PossibleValueService .. how to make this hook work ? >I found the the clue, a matter of i dont even need to add another @Service, it >gets automatically hooked by the famework, the mistake i made last time was >the the context, i have specified at the Property Instance level rather than >Property Model level After i chaged it to Sapphire.Property.MetaModel it >worked. > >Can you please explain the contexts with respect to the services ? For future reference, you can register multiple services by using @Services annotation. Regarding service contexts... A service in the context of Sapphire.Property.MetaModel only sees the property. A service in the context of Sapphire.Property.Instance sees the property and the model element. Think of MetaModel/Instance relationship as similar to Class/Object relationship. The ContentProposalService implementations should be instance-level services as you need the model element for many PossibleValueService implementations and certainly for querying types from context project. What tripped you up here is the API for accessing the service. ModelProperty.service( serviceType ) accesses MetaModel context. IModelElement.service( property, serviceType ) accesses Instance context. Makes sense?
(In reply to comment #27) > >> 1. In JavaTypeContentProposalService.Factory you should use > >> ModelProperty.isOfType instead of fetching @Type annotation, etc. > >> > >Can you please tell me the difference just for the knowledge sake. > > Same result, except the framework already processed that annotation for you. > OK > >> 2. In DefaultValuePropertyEditorRenderer, skip the part of adding > >> ControlDecoration as it will clash with Sapphire's existing property > >> editor feedback marker. > >> > >Yeah I agree but how do we display a tip (the traditonal orange bulb like icon) > >to tell the user that we have content assist? > > Will likely need to integrate the hint with the existing feedback marker. Let's > set aside this aspect for now. Once the rest is done, we can brainstorm. > OK > >> 3. ContentProposalInfo should be an immutable class. > >> > >You mean no setters only getters ? But why so since we are not going to set > >any values atfer initialization ? > > Since it makes no sense for the caller of ContentProposalService to call these > setters, it would make for cleaner API if the object is immutable once returned > by the service. > nice pattern, will remember this for my future API designs :) > >> 4. Shouldn't ContentProposalService.proposals() take existing string > >> plus cursor position to match IContentProposalProvider? > >> > >> 5. In PossibleValuesContentProposalService.proposals(), I would expect > >> to see filtering of proposals. Computing the static set at init time isn't > >correct. > >Can we not do the filtering this way, I have flag in > >DefaultValuePropertyEditorRenderer#ContentProvider, will delegate the same to > >this class, am thiking something like init method fetches all the values and > >proposals method will filter it based on the flag will just alter the > >proposals() to proposals(content,filtered) ? does i sound sensible ? and it > >annswers your #4 > > The reason you do filtering when calculating proposals is that frequently > enumerating the entire space of possibilities is exceptionally expensive. > Consider the Java type example. There could be tens of thousands (if not > hundreds of thousands of types) visible in a given project. Take a look at how > JDT implements IContentProposalProvider. We should be delegating to that > implementation in JavaTypeContentProposalService. > makes sense and i have updated the code for filtering, let investiage the JDT API for the content proposal implementation and see how i can delegate to that, last time when I tried accessing it i had some restriction warnings thrown whence refrained from trying it, let me try again. > >> One thing thats still unclear for me is , how can we specify two > >> @Service for a property, the logic right now we are using in > >> DefaultValuePropertyEditorRenderer > >> is checking for ContentProposalService for adding content assist, > >> which in turn uses PossibleValueService .. how to make this hook work ? > >I found the the clue, a matter of i dont even need to add another @Service, it > >gets automatically hooked by the famework, the mistake i made last time was > >the the context, i have specified at the Property Instance level rather than > >Property Model level After i chaged it to Sapphire.Property.MetaModel it > >worked. > > > >Can you please explain the contexts with respect to the services ? > > For future reference, you can register multiple services by using @Services > annotation. > > Regarding service contexts... A service in the context of > Sapphire.Property.MetaModel only sees the property. A service in the context of > Sapphire.Property.Instance sees the property and the model element. Think of > MetaModel/Instance relationship as similar to Class/Object relationship. > > The ContentProposalService implementations should be instance-level services as > you need the model element for many PossibleValueService implementations and > certainly for querying types from context project. > > What tripped you up here is the API for accessing the service. > > ModelProperty.service( serviceType ) accesses MetaModel context. > IModelElement.service( property, serviceType ) accesses Instance context. > > Makes sense? really nice, but then when I tried to use set it at instance level am not able to get the handle to ContentProposalService and when I changed to MetaModel level am able to access it .. you will find that stuff in the next patch i send you and you will be able to explain me if i have done something wrong.
> really nice, but then when I tried to use set it at instance level am not able > to get the handle to ContentProposalService and when I changed to MetaModel > level am able to access it .. you will find that stuff in the next patch i send > you and you will be able to explain me if i have done something wrong. In DefaultValuePropertyEditorRenderer, you are making a "property.service(ContentProposalService.class)" invocation, so you are accessing the MetaModel context. Thus changing the context of the service made the service lookup work, but you will not get much further with this since you will not be able to access the model element from the service implementation. You should instead be making "element.service(property,ContentProposalService.class)" invocation.
Great ! I now understand it clearly :), by making that mistake other than adding the content assisst i was not able to get to any further where in our JavaType search logic the modelElement was null and it flunked . but after following your explanation it materalized, am now able to see JavaTypes as well as Possible values with CONTROL + SPACE, now i need to refine the Java Search logic to filter classes based onthe typeConstraints. Also let me if its possible to show the Label Decorations for the proposals ? Its looks ugly when we just show the classname alone ( I hope we can do that with the Hit decoration) .. by the way you are not sleeping tonight ? I usually never get your replies @ this time ???
> Also let me if its possible to show the Label Decorations for the proposals ? > Its looks ugly when we just show the classname alone ( I hope we can do that > with the Hit decoration) .. How does this work in JDT's implementation? > by the way you are not sleeping tonight ? I usually never get your replies @ > this time ??? I frequently work odd hours. :)
(In reply to comment #31) > > Also let me if its possible to show the Label Decorations for the proposals ? > > Its looks ugly when we just show the classname alone ( I hope we can do that > > with the Hit decoration) .. > > How does this work in JDT's implementation? > I checked and gave a LabelProvider implementation called ContentProposalLabel Providers, i also altered the class ContentProposalInfo to have addtional attribute called "image" so that we can set the Java UI related images from sapphire.jdt.ui, in most of the places I am using the Shared images from PlatformPluginUI or JavaUI I will try to see if I can send you the files tonight (mytime) for your review, am just struck up with looping issue in JavaTypeSearch which I am trying to figure out. Since all the JDT search and content assist realted stuff are inside jdt.internal package I am trying to give a similar implementation in sapphire.jdt.ui, but dont worry they are not big changes.
> I checked and gave a LabelProvider implementation called ContentProposalLabel > Providers, i also altered the class ContentProposalInfo to have addtional > attribute called "image" so that we can set the Java UI related images from > sapphire.jdt.ui, in most of the places I am using the Shared images from > PlatformPluginUI or JavaUI Keep in mind that you cannot use SWT's Image or ImageData in ContentProposalService API. Image interchange in Sapphire API should happen with Sapphire's ImageData class. To construct Sapphire ImageData objects, see various factory methods and constructors on that class. Maybe the byte[] constructor? To go the other way and get an SWT image, use getPart().getImageCache().getImage( imageData ) > Since all the JDT search and content assist realted stuff are inside > jdt.internal package I am trying to give a similar implementation in > sapphire.jdt.ui, but dont worry they are not big changes. Ok.
(In reply to comment #33) > > I checked and gave a LabelProvider implementation called ContentProposalLabel > > Providers, i also altered the class ContentProposalInfo to have addtional > > attribute called "image" so that we can set the Java UI related images from > > sapphire.jdt.ui, in most of the places I am using the Shared images from > > PlatformPluginUI or JavaUI > > Keep in mind that you cannot use SWT's Image or ImageData in > ContentProposalService API. Image interchange in Sapphire API should happen with > Sapphire's ImageData class. To construct Sapphire ImageData objects, see various > factory methods and constructors on that class. Maybe the byte[] constructor? To > go the other way and get an SWT image, use getPart().getImageCache().getImage( > imageData ) > Yeah will check on the same . but for now i will work on the real piece (class search) and once thats working i will update the image handling part. > > Since all the JDT search and content assist realted stuff are inside > > jdt.internal package I am trying to give a similar implementation in > > sapphire.jdt.ui, but dont worry they are not big changes. > > Ok. I am unble to see why my search is not working,it was working earlier with TypeNameRequestor, but i changed it to TypeNameMatchRequestor so that I can filter the types by the "type" attribute of @JavaTypeConstraint .. but now it flunked :(, even if triggered it goes in to loop, shall I attach sample project am using for testing and the updated patch for your reivew and you figure out what am missing?, meanwhile i will continue to debug the issue ?
Created attachment 202666 [details] Patch v3 Changes: 1. Implemented the Content proposals for both Possible Values and JavaTypes 2. Service Enteries for the two ContentProposalService implementation 3. Sapphire.jdt.ui - Manifest Entry for adding org.eclipse.jface.text for providing Java Content Assist 4. Customized Label Provider and ImageContentProposal to make the content proposal in sync with other sites and similar content proposal. TODO: ------- 1. Label Decoration to show information that Content Assist available 2. Your comments on Image manipulation and update the code for Image manipulation ( not sure its still valid now - please review and let me know)
Created attachment 202667 [details] mylyn/context/zip
Created attachment 202668 [details] Test Project A dummy test project which can be I used to test this change, though its not sophisticated but still can be used (if you wish)
1. This patch appears to contain significant amount of copied JDT code. Maintaining this code in the future is going to be a liability. JDT code should be re-used in place or better explanation given for why that is not possible. 2. What is the purpose of the "filtered" switch throughout the API and implementation. I there a case where proposals would not be filtered? 3. ImageContentProposal and ContentProposalLabelProvider should be hidden as part of ContentProposalProvider implementation, not exposed as separate classes. 4. JavaTypeContentProposalService is implemented in stateful manner that assumes that only one client is ever conversing with it at a time. That is not correct. Consider what would happen if the same property instance is presented by multiple property editors concurrently. The service should not be maintaining the state of the conversation. 5. PossibleValuesService is not static, yet PossibleValuesContentProposalService caches its state during init. Interraction with PossibleValuesService should happen during the proposals call. The only thing that can happen during the init call is lookup of PossibleValuesService. Speaking of which, the lookup should happen in the init method rather than in the Factory.create method. This would eliminate the need for several not null checks. 6. I see ContentProposalInfo.setImage() method, but it doesn't appear to be used. 7. ContentProposalInfo methods should loose "get" prefix as this class is not a bean and we are starting to follow compact method naming convention in services. 8. ContentProposalInfo.hashCode implementation seems unnecessary complex. A simple delegation to this.content.hashCode() should be sufficient. Content should never be null. This should be checked in the constructor. 9. In ContentProposalInfo.equals implementation, "if (getClass() != obj.getClass())" would be better implemented using instanceof operator. Once you simplify this class to not allow null content to enter it, the implementation of equals method should get much simpler too. 10. In both of the content proposal service implementations, you cache ContentProposalInfo objects, but then re-create them during the proposals call. There is never a need to re-create ContentProposalInfo objects as they are immutable. > 2. Your comments on Image manipulation and update the code for Image > manipulation ( not sure its still valid now - please review and let me know) Still relevant. The service API (ContentProposalService and ContentProposalInfo) cannot reference SWT.
Thanks for your thoughts. (In reply to comment #38) > 1. This patch appears to contain significant amount of copied JDT code. > Maintaining this code in the future is going to be a liability. JDT code should > be re-used in place or better explanation given for why that is not possible. > 1. The reason for this reimplementation is that all the JDT or PDE code related to the Java Type/Package content assist are in internal packages and throwing a "Discouraged Access" when I tried resusing them, hence resorted to give a similar implementation. 2. I persoanlly felt that its better to maintain this content assist code at our plugin level to enable us to change the implementation and decouple it from the JDT/PDE(implementation), which might change in future - I meant JDT/PDE > 2. What is the purpose of the "filtered" switch throughout the API and > implementation. I there a case where proposals would not be filtered? > I guess we could remove it, i aways filter the values for better performance > 3. ImageContentProposal and ContentProposalLabelProvider should be hidden as > part of ContentProposalProvider implementation, not exposed as separate classes. > The ContentProposalLabelProvider is attached to the ContentProposalAdapter, may be i will give a inline implementation of the same rather than hniding in ContentProposalProvider as LabelProvider has no link with ContentProposalProvider Hope I make sense here, i will try to address these issues and others and send you a v4 for review.
(In reply to comment #38) > 2. What is the purpose of the "filtered" switch throughout the API and > implementation. I there a case where proposals would not be filtered? > Removed the usage of the "filtered" switch, the services will now compute when to filter and when to not filter. > 3. ImageContentProposal and ContentProposalLabelProvider should be hidden as > part of ContentProposalProvider implementation, not exposed as separate classes. > Implemented this change, ImageContentProposal and ContentProposalLabelProvider are not inner classes of the ContentProposalProvider > 4. JavaTypeContentProposalService is implemented in stateful manner that assumes > that only one client is ever conversing with it at a time. That is not correct. > Consider what would happen if the same property instance is presented by > multiple property editors concurrently. The service should not be maintaining > the state of the conversation. > Corrected the state related issued and try to check with to editors opened parllely and playing with same property value. Can you please verify and tell me if you still see the class mantains the state ? I have also introduced another method called reset() which the ContentProposalService implementors need to impement and provide the necessary state reset required inside that method. whenever init() is called it will also call reset() to reset the class's state Am I now making sense ? > 5. PossibleValuesService is not static, yet PossibleValuesContentProposalService > caches its state during init. Interraction with PossibleValuesService should > happen during the proposals call. The only thing that can happen during the init > call is lookup of PossibleValuesService. Speaking of which, the lookup should > happen in the init method rather than in the Factory.create method. This would > eliminate the need for several not null checks. > Corrected these small issues and i see your value comment here. Now my class looks clean and neat. > 6. I see ContentProposalInfo.setImage() method, but it doesn't appear to be > used. Removed. Earlier i used to set the image via this method when I was making the trial and error, but forgot to remove it :( > > 7. ContentProposalInfo methods should loose "get" prefix as this class is not a > bean and we are starting to follow compact method naming convention in services. > Done! I will keep this comment also in mind for future implementations rule of thumb " All service class show not follow JavaBean accessors/mutators method naming" > 8. ContentProposalInfo.hashCode implementation seems unnecessary complex. A > simple delegation to this.content.hashCode() should be sufficient. Content > should never be null. This should be checked in the constructor. > > 9. In ContentProposalInfo.equals implementation, "if (getClass() != > obj.getClass())" would be better implemented using instanceof operator. Once you > simplify this class to not allow null content to enter it, the implementation of > equals method should get much simpler too. > Done ! i now throw a IllegalStateException when the a null content vlaue is passed to the ContentProposalInfo. Please verify and let me know if you still see issues with it. > 10. In both of the content proposal service implementations, you cache > ContentProposalInfo objects, but then re-create them during the proposals call. > There is never a need to re-create ContentProposalInfo objects as they are > immutable. > Corrected it . Please verify and let me know if you still want the changes. > > 2. Your comments on Image manipulation and update the code for Image > > manipulation ( not sure its still valid now - please review and let me know) > > Still relevant. The service API (ContentProposalService and ContentProposalInfo) > cannot reference SWT. One comment, this is the first time am making this big or complex implementation with Eclipse PDE so please excuse me for silly mistakes and increase the Y <<<< X , will try to keep them as minimal as possible.
Created attachment 202704 [details] Patch_v3.1 Code Review updates and implementations for comment#40
Created attachment 202705 [details] mylyn/context/zip
> > 1. This patch appears to contain significant amount of copied JDT code. > > Maintaining this code in the future is going to be a liability. JDT code > > should be re-used in place or better explanation given for why that is not > > possible. > > 1. The reason for this reimplementation is that all the JDT or PDE code related > to the Java Type/Package content assist are in internal packages and throwing a > "Discouraged Access" when I tried resusing them, hence resorted to give a > similar implementation. > 2. I persoanlly felt that its better to maintain this content assist code at > our plugin level to enable us to change the implementation and decouple it from > the JDT/PDE(implementation), which might change in future - I meant JDT/PDE There is usually less risk involved in re-using internal code than in forking/re-implementing it. Once the amount of code goes beyond a few dozen lines, the scales tip pretty firmly in favor of re-use. The only time I would consider forking is when we cannot achieve the behavior that we are after by re-using the code in place. I would expect only JDT to be involved here (no PDE). Whatever is used in the new Java Class wizard... If this isn't possible and forking/re-implementation is necessary, then we need more documentation regarding why re-use is impossible, how much of the code has been forked as opposed to re-implemented from scratch, original copyright statements need to be maintained on forked code, etc. There is also an issue with the size of this patch. Once a patch exceeds 250 lines, the contribution must be verified by IP legal before it can be merged, which can take a considerable amount of time. See http://www.eclipse.org/legal/EclipseLegalProcessPoster.pdf Given these issues, I think it would be prudent to split Java content assist into a separate enhancement request. This one can then focus on base API and the implementation based on PossibleValuesService. > Corrected the state related issued and try to check with to editors opened > parllely and playing with same property value. Can you please verify and tell > me if you still see the class mantains the state ? > > I have also introduced another method called reset() which the > ContentProposalService implementors need to impement and provide the necessary > state reset required inside that method. whenever init() is called it will > also call reset() to reset the class's state That's still not going to work. The code needs to be written such that reset behavior is not necessary. For JavaTypeContentProposalService, that means no fields like computedProposals and previousContent. > Done ! i now throw a IllegalStateException when the a null content vlaue is > passed to the ContentProposalInfo. Please verify and let me know if you still > see issues with it. It looks better, but this block of code: if (!this.content.equals(other.content)) { return false; } return true; Can be more simply expressed like this: return this.content.equals(other.content); > > 5. PossibleValuesService is not static, yet > > PossibleValuesContentProposalService > > caches its state during init. Interraction with PossibleValuesService should > > happen during the proposals call. The only thing that can happen during the > > init call is lookup of PossibleValuesService. Speaking of which, the lookup > > should happen in the init method rather than in the Factory.create method. > > This would eliminate the need for several not null checks. > > Corrected these small issues and i see your value comment here. Now my class > looks clean and neat. This is not addressed as your are still caching. Now you are caching during proposals call rather than init, but the effect is still the same. When PossibleValuesService changes, the proposals will not update accordingly. You should be calling into PossibleValuesService with every invocation of proposals method. > > 10. In both of the content proposal service implementations, you cache > > ContentProposalInfo objects, but then re-create them during the proposals > > call. > > There is never a need to re-create ContentProposalInfo objects as they are > > immutable. > > Corrected it . Please verify and let me know if you still want the changes. I still see at least one place where ContentProposalInfo is re-created unnecessarily. See PossibleValuesContentProposalService.filterProposals.
I think I figured out what the caching/reset, etc. is all about in your implementation... Once content assist is activated, you can increase performance of work performed on every key stroke by progressively filtering the proposals originally computed. I think that if this caching simply moved from the service implementation to the IContentProposalProvider implementation in DefaultValuePropertyEditorRenderer then the issue would be resolved.
(In reply to comment #43) > > > 1. This patch appears to contain significant amount of copied JDT code. > > > Maintaining this code in the future is going to be a liability. JDT code > > > should be re-used in place or better explanation given for why that is not > > > possible. > > > > 1. The reason for this reimplementation is that all the JDT or PDE code > related > > to the Java Type/Package content assist are in internal packages and throwing > a > > "Discouraged Access" when I tried resusing them, hence resorted to give a > > similar implementation. > > 2. I persoanlly felt that its better to maintain this content assist code at > > our plugin level to enable us to change the implementation and decouple it > from > > the JDT/PDE(implementation), which might change in future - I meant JDT/PDE > > There is usually less risk involved in re-using internal code than in > forking/re-implementing it. Once the amount of code goes beyond a few dozen > lines, the scales tip pretty firmly in favor of re-use. The only time I would > consider forking is when we cannot achieve the behavior that we are after by > re-using the code in place. > > I would expect only JDT to be involved here (no PDE). Whatever is used in the > new Java Class wizard... > > If this isn't possible and forking/re-implementation is necessary, then we need > more documentation regarding why re-use is impossible, how much of the code has > been forked as opposed to re-implemented from scratch, original copyright > statements need to be maintained on forked code, etc. > > There is also an issue with the size of this patch. Once a patch exceeds 250 > lines, the contribution must be verified by IP legal before it can be merged, > which can take a considerable amount of time. > > See http://www.eclipse.org/legal/EclipseLegalProcessPoster.pdf > > Given these issues, I think it would be prudent to split Java content assist > into a separate enhancement request. This one can then focus on base API and the > implementation based on PossibleValuesService. > If I understand you correctly you just want me delegate the proposals compution to the JDT class and still resuse it though there is plug-in restriction on it ? Ideally i can supress the restriction and resuse it ? > > Corrected the state related issued and try to check with to editors opened > > parllely and playing with same property value. Can you please verify and tell > > me if you still see the class mantains the state ? > > > > I have also introduced another method called reset() which the > > ContentProposalService implementors need to impement and provide the necessary > > state reset required inside that method. whenever init() is called it will > > also call reset() to reset the class's state > > That's still not going to work. The code needs to be written such that reset > behavior is not necessary. For JavaTypeContentProposalService, that means no > fields like computedProposals and previousContent. > > > Done ! i now throw a IllegalStateException when the a null content vlaue is > > passed to the ContentProposalInfo. Please verify and let me know if you still > > see issues with it. > > It looks better, but this block of code: > > if (!this.content.equals(other.content)) { > return false; > } > > return true; > > Can be more simply expressed like this: > > return this.content.equals(other.content); > > > > 5. PossibleValuesService is not static, yet > > > PossibleValuesContentProposalService > > > caches its state during init. Interraction with PossibleValuesService should > > > happen during the proposals call. The only thing that can happen during the > > > init call is lookup of PossibleValuesService. Speaking of which, the lookup > > > should happen in the init method rather than in the Factory.create method. > > > This would eliminate the need for several not null checks. > > > > Corrected these small issues and i see your value comment here. Now my class > > looks clean and neat. > > This is not addressed as your are still caching. Now you are caching during > proposals call rather than init, but the effect is still the same. When > PossibleValuesService changes, the proposals will not update accordingly. You > should be calling into PossibleValuesService with every invocation of proposals > method. > Added my comments for the same in you next sequential comment. > > > 10. In both of the content proposal service implementations, you cache > > > ContentProposalInfo objects, but then re-create them during the proposals > > > call. > > > There is never a need to re-create ContentProposalInfo objects as they are > > > immutable. > > > > Corrected it . Please verify and let me know if you still want the changes. > > I still see at least one place where ContentProposalInfo is re-created > unnecessarily. See PossibleValuesContentProposalService.filterProposals.
(In reply to comment #44) > I think I figured out what the caching/reset, etc. is all about in your > implementation... Once content assist is activated, you can increase performance > of work performed on every key stroke by progressively filtering the proposals > originally computed. > > I think that if this caching simply moved from the service implementation to the > IContentProposalProvider implementation in DefaultValuePropertyEditorRenderer > then the issue would be resolved. I have corrected the caching issue by moving the pieces of code to DefaultValuePropertyEditorRenderer#IContentProposalProvider , now the proposals method will always comput the proposal. I have also added another method filterProposals to the ContentProposalService to enable the filtering of proposals. So that we can pass the cached proposals from DefaultValuePropertyEditorRenderer#IContentProposalProvider to the service class. I hope this resolves the caching issue ? Let me know otherwise Regarding the JavaTypeContentProposal - i see that the JDT TypeCompletionProcessor is implemented differently and it requires text controls work with wherein we have decoupled the usage of text control and computing just proposals by using our Framework Service Pattern . Am not sure we could reuse the code, when I meant PDE its not including PDE plugin since PDE is already implicity included when we are building the plugin(s) those classes are already in the classpath via the requirements for ManifestEditor. If there are IP problems with me submitting this patch, you can take the JavaTypeProposal service and publish under your contribution.
Created attachment 202735 [details] Patch v3.2 - Comments related to caching is resolved and for your review and verification - Minor updates done to clean up the code and Service method signatures for caching and filtering changes Please validate and let me know your comments
Created attachment 202736 [details] mylyn/context/zip
> Regarding the JavaTypeContentProposal - i see that the JDT > TypeCompletionProcessor is implemented differently and it requires text > controls work with wherein we have decoupled the usage of text control and > computing just proposals by using our Framework Service Pattern . Am not sure > we could reuse the code, Ok. That's a good reason for re-use being not feasible. > when I meant PDE its not including PDE plugin since PDE is already implicity > included when we are building the plugin(s) those classes are already in the > classpath via the requirements for ManifestEditor. PDE plugins aren't on the classpath of Sapphire runtime bundles. I think you might be confusing runtime environment with development environment. If you based your implementation on PDE, then we will have to fork the code. > If there are IP problems with me submitting this patch, you can take the > JavaTypeProposal service and publish under your contribution. That would be a violation of Eclipse Legal rules. The quickest and least-effort way to get any of this work merged is to split off Java type proposal portion into a separate bugzilla entry with a separate patch that builds on this one. With less code to review, we can finish this part quicker. Then we can move on to the Java type portion where I can go over the process involved when you copy code from another Eclipse project. > I have corrected the caching issue by moving the pieces of code to > DefaultValuePropertyEditorRenderer#IContentProposalProvider , now the > proposals method will always comput the proposal. > I have also added another method filterProposals to the > ContentProposalService to enable the filtering of proposals. So that we can > pass the cached proposals from > DefaultValuePropertyEditorRenderer#IContentProposalProvider to the service > class. That's a pretty messy bi-directional coupling. I was going on the assumption that filtering logic is universal and as such can be housed in IContentProposalProvider implementation. Now I realize that it isn't a simple filter, but can in fact add new proposals (as in what happens when you traverse through a dot for Java type proposals). Given that, I think a new approach at the service API is necessary. Perhaps a session pattern can be useful here. How about this... ContentProposalService { // filter is the text box string from start to cursor position. // Always returns a new Session object Session session( String filter ); Session { // Returns the current filter String filter(); // Appends the provided characters to the filter and adjusts // the proposals accordingly void advance( String characters ); // Returns the current set of proposals based on the current filter. List<ContentProposal> proposals(); } } This solves the issue of caching in ContentProposalService object which could be servicing multiple sessions concurrently. Since the Session object is not shared with anyone, it can cache the proposals. This approach also doesn't require passing proposals cached externally back to the ContentProposalService implementation. Let me know if anything is unclear.
(In reply to comment #49) > > when I meant PDE its not including PDE plugin since PDE is already implicity > > included when we are building the plugin(s) those classes are already in the > > classpath via the requirements for ManifestEditor. > > PDE plugins aren't on the classpath of Sapphire runtime bundles. I think you > might be confusing runtime environment with development environment. If you > based your implementation on PDE, then we will have to fork the code. > In fact i didn't resuse the entire code of the PDE just one class from pde.ui which does the work similar to our JavaType content processor. So we might not even need any PDE plugins as we rely only on org.eclipse.jface > > If there are IP problems with me submitting this patch, you can take the > > JavaTypeProposal service and publish under your contribution. > > That would be a violation of Eclipse Legal rules. The quickest and least-effort > way to get any of this work merged is to split off Java type proposal portion > into a separate bugzilla entry with a separate patch that builds on this one. > With less code to review, we can finish this part quicker. Then we can move on > to the Java type portion where I can go over the process involved when you copy > code from another Eclipse project. > Yes i will create a new ticket right away for handling the Java Type portion of it and i will try to implement the session pattern with this Possible Values and close this bug. Once you merge the this one i will take an update and work on the Java Type ticket with your comments on the Eclipse Legal Process et al.
(In reply to comment #49) > That would be a violation of Eclipse Legal rules. The quickest and least-effort > way to get any of this work merged is to split off Java type proposal portion > into a separate bugzilla entry with a separate patch that builds on this one. > With less code to review, we can finish this part quicker. Then we can move on > to the Java type portion where I can go over the process involved when you copy > code from another Eclipse project. > I have created a new enhancement 356744 to work on this enhancement. > That's a pretty messy bi-directional coupling. I was going on the assumption > that filtering logic is universal and as such can be housed in > IContentProposalProvider implementation. Now I realize that it isn't a simple > filter, but can in fact add new proposals (as in what happens when you traverse > through a dot for Java type proposals). > > Given that, I think a new approach at the service API is necessary. Perhaps a > session pattern can be useful here. How about this... > > ContentProposalService > { > // filter is the text box string from start to cursor position. > // Always returns a new Session object > > Session session( String filter ); > > Session > { > // Returns the current filter > String filter(); > > // Appends the provided characters to the filter and adjusts > // the proposals accordingly > void advance( String characters ); > > // Returns the current set of proposals based on the current filter. > List<ContentProposal> proposals(); > } > } > > This solves the issue of caching in ContentProposalService object which could be > servicing multiple sessions concurrently. Since the Session object is not shared > with anyone, it can cache the proposals. > > This approach also doesn't require passing proposals cached externally back to > the ContentProposalService implementation. > > Let me know if anything is unclear. Couple of things thats unclear for me here is 1. The factory inside the impementations will now create a Session object of the Content Proposal Service, the IContentProvider in DefaultValuePropertyRenderer will get an hnadle of the session and will interact with Session instead of the Service object itself ? 2. Can you please be bit more detailed on this session pattern , pointer or links that might help ? Is this the same SessionFacade that we used to follow for EJB ? I will now start working on this and try to close bug tomorrow for yoru review and merge.
> Couple of things thats unclear for me here is > 1. The factory inside the impementations will now create a Session object of > the Content Proposal Service, The factory would still create the service. > the IContentProvider in DefaultValuePropertyRenderer will get an hnadle of > the session and will interact with Session instead of the Service object > itself ? IContentProposalProvider would locate the service, then open a session. As long as the user keeps typing forward, IContentProposalProvider can re-use the session. If the user goes backwards or otherwise starts with a new base string, you would go back to the service and open a new session. > 2. Can you please be bit more detailed on this session pattern , pointer or > links that might help ? Is this the same SessionFacade that we used to follow > for EJB ? The session pattern is simply a way to maintain state (in this case to cache) in API that cannot maintain state. In this case, the service doesn't maintain state, but the session object that you create from it does maintain state. All clear?
Created attachment 202789 [details] Patch v4 Removed the Java Type portion from this patch as that will be handled by Bug 35674 I have addressed your session related comments (ref comment #52) and modified the ContentProposalService to include Session Handling and updated the implementation to PossibleValuesContentProposalService to adapt to Session pattern Hope thing looks much better now, please verify and move the bug to next stage, once I happen to hear from you on OK i will synch with head and work on next bug
Created attachment 202790 [details] mylyn/context/zip
Is these fixes OK, if so can you pelase verify and move the bug to next state, also let meknow when I shall take the update and work on the next bug.
You aren't done yet... 1. Image situation is still unresolved. 2. The ContentProposalService and Session API isn't what I described. See my Comment 49 for what I am expecting to see. The current implementation allows the session to be reset by calling proposals method multiple times and it doesn't track the current string or the progress. There may be other issues. I will take a deeper dive when these larger pieces are resolved. Also... This patch is missing unit tests, a section in the services doc and a section in what's new for 0.4 doc.
(In reply to comment #56) > You aren't done yet... > > 1. Image situation is still unresolved. > I am still not clear with the image part of it rather i am not sure how we can use shared images path alone here i combination with ImageData, if you have seen my code i have still marked it TODO. If you can show me a sample then i can follow, i understand how to create the ImageData.. > 2. The ContentProposalService and Session API isn't what I described. See my > Comment 49 for what I am expecting to see. The current implementation allows the > session to be reset by calling proposals method multiple times and it doesn't > track the current string or the progress. > > There may be other issues. I will take a deeper dive when these larger pieces > are resolved. > the initial contents and other pieces are handled by the DefaultValuePropertyRender#ContentProposalProvider, i saw couple of other similar implementations and they have made the filtering responsibility to the ContentProposal provide rather than processor and I also personally feel thats the way, we dont need to call the Service if we are just filtering the values > Also... This patch is missing unit tests, a section in the services doc and a > section in what's new for 0.4 doc. Yes this is sure piece I totally forgot amidst other discussions, will work on it
> I am still not clear with the image part of it rather i am not sure how we > can use shared images path alone here i combination with ImageData, if you > have seen my code i have still marked it TODO. If you can show me a sample > then i can follow, i understand how to create the ImageData.. See Sapphire ImageData class and the various factory methods in that class like readFromUrl, readFromClassLoader, etc. Converting from Sapphire ImageData to SWT Image or ImageDescriptor is easy. Just use getPart().getImageCache().getImage( imageData ) call. Converting from SWT Image/ImageData/ImageDescriptor to Sapphire ImageData is likely impossible. That's because Sapphire ImageData is actual image file contents, while SWT versions are post-decoding in-memory representations. So you would need to locate the actual source of the images and use factory methods in Sapphire ImageData like readFromClassLoader(). Let me know if that is still unclear. > the initial contents and other pieces are handled by the > DefaultValuePropertyRender#ContentProposalProvider, i saw couple of other > similar implementations and they have made the filtering responsibility to > the ContentProposal provide rather than processor and I also personally feel > thats the way, we dont need to call the Service if we are just filtering the > values Well.. That's not really what you are doing either. Your Session class has a filter method on it. Using the term "filter" in reference to what happens when you advance through the completion is pretty confusing. In some cases it's filtering (like PossibleValuesService), but in other cases (like Java types when you advance through a dot) it's actually adding new proposals. That's why if you look at the API that I laid out, Session keeps track of the string and the advance() method triggers implementation-specific re-computation of the proposals. Also note how the current proposals are retrivable directly without triggering any actions. Take another look at the API that I've laid out and if you have questions about why certain parts of it are one way or another, we can discuss.
(In reply to comment #58) > > I am still not clear with the image part of it rather i am not sure how we > > can use shared images path alone here i combination with ImageData, if you > > have seen my code i have still marked it TODO. If you can show me a sample > > then i can follow, i understand how to create the ImageData.. > > See Sapphire ImageData class and the various factory methods in that class like > readFromUrl, readFromClassLoader, etc. > > Converting from Sapphire ImageData to SWT Image or ImageDescriptor is easy. Just > use getPart().getImageCache().getImage( imageData ) call. > > Converting from SWT Image/ImageData/ImageDescriptor to Sapphire ImageData is > likely impossible. That's because Sapphire ImageData is actual image file > contents, while SWT versions are post-decoding in-memory representations. So you > would need to locate the actual source of the images and use factory methods in > Sapphire ImageData like readFromClassLoader(). > > Let me know if that is still unclear. > Let me try and get back to you. > > the initial contents and other pieces are handled by the > > DefaultValuePropertyRender#ContentProposalProvider, i saw couple of other > > similar implementations and they have made the filtering responsibility to > > the ContentProposal provide rather than processor and I also personally feel > > thats the way, we dont need to call the Service if we are just filtering the > > values > > Well.. That's not really what you are doing either. Your Session class has a > filter method on it. Using the term "filter" in reference to what happens when > you advance through the completion is pretty confusing. In some cases it's > filtering (like PossibleValuesService), but in other cases (like Java types when > you advance through a dot) it's actually adding new proposals. That's why if you > look at the API that I laid out, Session keeps track of the string and the > advance() method triggers implementation-specific re-computation of the > proposals. Also note how the current proposals are retrivable directly without > triggering any actions. > > Take another look at the API that I've laid out and if you have questions about > why certain parts of it are one way or another, we can discuss. Ah! i see where you are going now to , let me try to reimplement that stuff on comment#49 and come back to you. I am bit held up for next couple of days on my official work - so will take a deatiled look on it on sat/sun .. Meanwhile i shall try to rethink on them and get back to you on my questions/clarifications.
Some of the questions that i have right away and some to double check my undertanding. 1. filter() - is string that user has entered before the proposals were shown ? e.g when user enter abc for the first time then filter() returns "abc" next when user enters "de" then filter will return "abcde" 2. proposals() - method will always do filtering based on the value of filter() 3. On the summmary all filtering and computation is the the job of the Session inside the ContentProposal service class and ContentProvider inside DefaultValuePropertyRender just decides when to call advance() and when to call proposals() 4. When the user presses backspace or postion == 0 , then we need to create new session() and build all the proposals ? 5. advance() method will keep updating the filter() value whever it is called ? (per session basis)
> 1. filter() - is string that user has entered before the proposals were shown > ? e.g when user enter abc for the first time then filter() returns "abc" next > when user enters "de" then filter will return "abcde" Correct. > 2. proposals() - method will always do filtering based on the value of filter() Somewhat correct. I would expect the proposals() method to fetch already-computed proposals. That is, the computations of proposals based on the filter string should happen elsewhere (during session initialization and during advance call). > 3. On the summmary all filtering and computation is the the job of the Session > inside the ContentProposal service class and ContentProvider inside > DefaultValuePropertyRender just decides when to call advance() and when to call > proposals() Correct. > 4. When the user presses backspace or postion == 0 , then we need to create > new session() and build all the proposals ? Correct. > 5. advance() method will keep updating the filter() value whever it is > called ? (per session basis) Correct. Update the filter and re-compute the proposals.
(In reply to comment #61) > > 2. proposals() - method will always do filtering based on the value of > filter() > > Somewhat correct. I would expect the proposals() method to fetch > already-computed proposals. That is, the computations of proposals based on the > filter string should happen elsewhere (during session initialization and during > advance call). > If i get your point correctly we could have a prive mehtod inside session which will do proposal computation lets say computeProposals(), when during Session() or advance they will call computeProposals() which does proposal computation based on filter() and stores the value in an Session class instance variable called propsoalInfos, the proposals() will just return the propsoalInfos
> If i get your point correctly we could have a prive mehtod inside session > which will do proposal computation lets say computeProposals(), when during > Session() or advance they will call computeProposals() which does proposal > computation based on filter() and stores the value in an Session class > instance variable called propsoalInfos, the proposals() will just return the > propsoalInfos Yes. I would be inclined to make all public methods on Session class final. Then have an abstract protected refresh() method that is the only thing that the extenders have to implement.
Actually, maybe something like this: protected abstract List<ContentProposalInfo> compute(); That way, you don't have to expose the proposals member field to subclasses.
This is the skeleton that i made from your API public abstract class Session { protected String filter; protected List<ContentProposalInfo> proposalInfos; public Session(String filter){ this.filter=filter; } /** * * @return */ public String filter(){ return this.filter; } /** * * @param characters */ public abstract void advance(String characters); /** * this method will prepare the list of proposals that needs to be * returned, the concrete implementator needs to provide the * implementation * * @param contents * - the existing content, will be used in case we do * filtering * @return list of {@link ContentProposalInfo} */ public abstract List<ContentProposalInfo> proposals(); /** * * @return */ protected abstract void computeProposals(); } Since the computeProposals() is going to modify the only the internal cached session i feel we dont need to return the list, just update the cached collection with the filter, so when the user goes back or clears the filter then its going to be new session all together, in this way this method is callable from Session() as well as from the advance(), the external world will call only proposals() to get the computedProposals() Am I making sense now ?
Created attachment 202926 [details] Session Implmentation Review This patch is for your review of the Session implementation and how its called form the ContentProposalProvider, if you feel this implementation is OK i will work on the image part, doc and test cases and submit the next patch for your review.
Created attachment 202927 [details] mylyn/context/zip
I would expect filter, advance and proposals methods to be final and implemented in this class. I would further expect filter and proposalInfos fields to be private. The extender should only need to implement the compute method. By returning the list instead of having it set the field internally, we can keep the field private, which reduces the API surface.
(In reply to comment #68) > I would expect filter, advance and proposals methods to be final and implemented > in this class. I would further expect filter and proposalInfos fields to be > private. > i can make filter and proposals as final and not inclined in making the advance method be final, because when we implement the Java Type the advance method will have a slightly different logic and hence willbe required to be overriden Also making filter and proposalInfos private will prevent it from accessible from the session() method of the implementor (PossibleValueProposalService) and thereby we will not be able to update it when advance is called so i will retain them as protected > The extender should only need to implement the compute method. By returning the > list instead of having it set the field internally, we can keep the field > private, which reduces the API surface. No going by my previous comment the implementor should implement both advance and computeProposals, again thinking when we give the JavaType portion these methods will need to be implemented in a different way, Hope I make sound sensible in the modifiers details of this implementation ?
Yes... That does complicate matters. Having said that, I do not like API that exposes fields. That makes them harder to maintain. How about this: protected abstract List<ContentProposalInfo> compute( String filter, String delta, List<ContentProposalInfo> previousProposals ); During session creation, it gets called like this: this.proposals = compute( filter, null, null ); During advance call, it gets called like this: this.proposals = compute( filter, delta, this.proposals );
does this mean that we dont need the advance method any more ? the client will simply call compute method passing the old filter and new filter, i dont want to pass the previousProposals to this method as that will go back one of our prebvious implementation and the client need to maintain the preivoous proposals, rather we could have the proposals method as protected and final to just act as accessor for returning the private final List of proposals. Tell me if you feel other wise ?
also one more thing from your comment #70 , i guess am not differing from your concept and with my implementations, i guess we are alinged on that . As i said in my previous comment i dont want to take propsals taken to client and client passing it everytime to the Service as thats an uncessary overhead and maintainnence at two places .. A matter of fact i dont want to expose the compute to the external world, why should they know how am computing ? all they hey need is advance and proposals thats it., I feel lets have it simple and not complicate things further.
> does this mean that we dont need the advance method any more? No. There will still be the advance method. > A matter of fact i dont want to expose the compute to the external world That's precisely the point. The compute method is protected. It is implemented by subclasses of Session and is used as part of Session implementation. The users of Session interact with advance/proposals methods.
here is the subclass implementation that uses Session (PossibleValueService) @Override protected void computeProposals() { if (this.proposalInfos == null) { this.proposalInfos = new ArrayList<ContentProposalInfo>(); } SortedSet<String> values = PossibleValuesContentProposalService.this.possibleValuesService .values(); for (String value : values) { ContentProposalInfo cpInfo = new ContentProposalInfo(value, null, value); if (!this.proposalInfos.contains(cpInfo)) { this.proposalInfos.add(cpInfo); } } } @Override public void advance(String characters) { this.filter = characters; String lcFilterContents = this.filter.toLowerCase(); ListIterator<ContentProposalInfo> listIterator = this.proposalInfos .listIterator(); while (listIterator.hasNext()) { ContentProposalInfo contentProposalInfo = listIterator .next(); String compareString = contentProposalInfo.content() .toLowerCase(); /* * remove the proposals, that are not matching, since we are * advancing we dont need the proposals that doesn't match * the current filter criteria */ if (!compareString.startsWith(lcFilterContents, 0)) { listIterator.remove(); } } } ideally th external world will just have two methods adavce() and proposals() - everyting else is hidden .Are we aligned here ?
Advance method should be final, not like in the implementation you are showing. Also, the subclasses should not be accessing Session fields.
I guess i made a clear implementation, find below the PossibleValueService implementatin of session ... new Session(filter) { /* * (non-Javadoc) * * @see * org.eclipse.sapphire.ui.services.ContentProposalService.Session * #computeProposals(java.lang.String, java.lang.String, * java.util.List) */ @Override protected List<ContentProposalInfo> computeProposals(String filter, String delta, List<ContentProposalInfo> previousProposals) { /* * this will be done when proposoals is requested for first time * or when the session is recreated */ if (previousProposals == null) { previousProposals = new ArrayList<ContentProposalInfo>(); SortedSet<String> values = PossibleValuesContentProposalService.this.possibleValuesService .values(); for (String value : values) { ContentProposalInfo cpInfo = new ContentProposalInfo( value, null, value); if (!previousProposals.contains(cpInfo)) { previousProposals.add(cpInfo); } } } //This is always called when we do the filtering of the existing proposals else { String lcFilterContents = filter.toLowerCase(); ListIterator<ContentProposalInfo> listIterator = previousProposals .listIterator(); while (listIterator.hasNext()) { ContentProposalInfo contentProposalInfo = listIterator .next(); String compareString = contentProposalInfo.content() .toLowerCase(); /* * remove the proposals, that are not matching, since we * are advancing we dont need the proposals that doesn't * match the current filter criteria */ if (!compareString.startsWith(lcFilterContents, 0)) { listIterator.remove(); } } } return previousProposals; } }; .... hope that we both are now aligned ? But something which am not clear is that about "delta" and when it will be used ? Does this requires us toi change the method signature of advance(String characters) to advance(String characters, String delta) ??? Can you please clarify me on the usage of "delta" here ??
if you are fine with the session impl now, i shall go ahead with he doc and test cases . i want to close this bug this week !!! it has been dragging ... Also we went through multiple iterations , but i feel these iterations are worth discussion but peronsall i feel some iterations could be avoid if we have our communitcation sorted out, some times either my language or your language is misunderstood by either of us and the implication is more iterations ... Hope we can improve up on that down the lane.
> But something which am not clear is that about "delta" and when it will be used > ? Does this requires us toi change the method signature of advance(String > characters) to advance(String characters, String delta) ??? Can you please > clarify me on the usage of "delta" here ?? "delta" is the same thing as "characters" in advance. The method signature of advance should not be changed. When advance implementation is calling compute method, it should pass in old filter + the change (aka the delta). > if you are fine with the session impl now, i shall go ahead with he doc and > test cases . Go for it.
I need to add the "what's new" section in the enhancements.html right ? or is there any other palce i need to update ? Just to want play safe to avoid rework :)
We need a new section in the enhancements.html and a section in the new doc that documents the services in detail (services/index.html). The section in the enhancements doc should be a relatively short summary of the fuller content in the services doc with the link to the full documentation. See how that's done for DependenciesService and FileExtensionsService (in 0.3.1 what's new).
(In reply to comment #80) > We need a new section in the enhancements.html and a section in the new doc > that documents the services in detail (services/index.html). The section in the > enhancements doc should be a relatively short summary of the fuller content in > the services doc with the link to the full documentation. See how that's done > for DependenciesService and FileExtensionsService (in 0.3.1 what's new). I have done with the document and testcases but i have test case failure for a property with @PossibleValues annotation, i remember you telling me that @PossibleValues will be hooked to PossibleValuesService by the framework and hence will naturally inherit the ContentProposalService but right now its no happening, I can get the proposals only for the PossibleValuesService properties and not for @PossibleValues annotated ones. I am not sure I have missed something in the implementation to handle this case. I have attached the latest patch for your review, i you can let me know what i have missed i can implement the same and bundle another patch with documentation and test cases tomorrow. ~Kamesh
Created attachment 203355 [details] Patch v4 (Review for @PossibleValues) This patch is for your initial review of the, 1. Documentation 2. Content Proposal service is failing for @PossibleValues properties, need your help for reviewing and let me know what am missing to implement the missing piece. PS: The test cases are incomplete without fixing the @PossibleValues issue i cant proceed to other cases.
(In reply to comment #82) > Created attachment 203355 [details] > Patch v4 (Review for @PossibleValues) > > This patch is for your initial review of the, > > 1. Documentation > 2. Content Proposal service is failing for @PossibleValues properties, need > your help for reviewing and let me know what am missing to implement the > missing piece. > > > PS: The test cases are incomplete without fixing the @PossibleValues issue i > cant proceed to other cases. I am also thinking about one more case where we give @PossibleValues(property="x") and use the ReferenceValueService Ideally this should also allow the user to CTRL + SPACE
It might take me a day or two to get to reviewing this patch. I am trying to clear our 0.3.1 backlog as that release date is coming up quick.
(In reply to comment #84) > It might take me a day or two to get to reviewing this patch. I am trying to > clear our 0.3.1 backlog as that release date is coming up quick. Shouldn't be a problem, please take your time as 0.3.1 is much more important than this. I also have to clear some backlog that is there with my contribution to Liferay and my official work. I will try to clear all of these and get back to Sapphire once I hear back from you. Hope you are OK with that ?
(In reply to comment #84) > It might take me a day or two to get to reviewing this patch. I am trying to > clear our 0.3.1 backlog as that release date is coming up quick. I guess you will be busy until we release 0.3.1. Please let me know once you have reviewed it, we shall discuss further on this and close this bug and move to next one ASAP. Meanwhile am also quiet held up with my other releases so we can catch up post 0.3.1 release. Let me know your thoughts.
Still quite swamped. I might be another week before I can get to this.
(In reply to comment #87) > Still quite swamped. I might be another week before I can get to this. Konstantin, Let me know if we can proceed further on this, need your review before i can work it to completion. And want to move with 355751 as I have a requirement for it in Liferay IDE where I am using Sapphire extensively for building new XML based editors. I will park the Java Content proposal part of until i complete the 355751 as that is more important for me. Let me know your thoughts on the same.
Just getting back to this. Sorry it took so long. > I can get the proposals only for the PossibleValuesService properties and not > for @PossibleValues annotated ones. The issue was that you are looking up PossibleValuesService from property metamodel context as opposed to property instance context. In PossibleValuesContentProposalService.Factory.applicable, it should be: final IModelElement element = context.find( IModelElement.class ); if( element != null ) { final ValueProperty property = context.find( ValueProperty.class ); if( property != null ) { return ( element.service( property, PossibleValuesService.class ) != null ); } } return false; Similarly, PossibleValuesContentProposalService.init() should look like this: protected void init() { super.init(); final IModelElement element = context( IModelElement.class ); final ValueProperty property = context( ValueProperty.class ); this.possibleValuesService = element.service( property, PossibleValuesService.class ); } With the above two changes, I was able to get @PossibleValues scenarioes to work. The docs changes should be ok. I will do a few edits when you have the final patch. Note that you cannot include image files in the patch. They get garbled. Please attached two separately.
> I will park the Java Content proposal part of until i complete the 355751 as > that is more important for me. Sure. It would be best to wrap this one up as soon as possible as can be hard to come back after a long pause, but Java content assist is separate enough that it can be handled later.
Created attachment 205292 [details] Patch v5.1 Images the images that needs to go in to the org.eclipse.sapphire.doc/services/images, removed it from patch and attaching seperately
Created attachment 205294 [details] Completed Implementation and Test Cases Fixed the issues with respect to the changes required for handling the PossibleValues and its related test cases, documentation.
Created attachment 205295 [details] mylyn/context/zip
Please let me know once you have reviewed and merged it to the HEAD, and moved the bug to next stage, i will take an update and will work on 355751
Comment on attachment 205294 [details] Completed Implementation and Test Cases Missing copyrights for a Test Class updated it with with patch 5.1
Created attachment 205477 [details] Patch v5.1 Fixed the missing copyrights in test class for services.t0004
(In reply to comment #96) > Created attachment 205477 [details] > 354199 - Implementation Completed > > Fixed the missing copyrights in test class for services.t0004 Did you had chance to review it ? If so let me know your comments/or if you have merged the code I shall take the update and work on other bugs
My apologies for the slow turnaround. Things have been really hectic recently. I have one more item to complete first and then I will be able to take a look at the latest revision.
(In reply to comment #98) > My apologies for the slow turnaround. Things have been really hectic recently. > I have one more item to complete first and then I will be able to take a look > at the latest revision. No worries! I am bit free for next 1 week and thought to close the open ones :).. Anyways take your time and priorities and let me know once you are back on this..
By any chance did you happen to review this ?
Review comments for Patch v5.1: 1. In ContentProposalService API, it seems that session( "abc" ).advance( "def" ) and session( null ).advance( "abcdef" ) are semantically equivalent. My preference would be to simplify this API by not having the session method take the parameter. The session can always start with an empty string and the first advance call can be used to establish context. 2. It appears that the delta argument isn't being treated as a delta in computeProposals() and advance() method calls. I would expect to see something equivalent to this.filter = this.filter + delta in the advance() method. Similarly, I would expect the caller of advance to only pass in the delta (characters added to the last filter state or a suffix) instead of passing in the whole string. Further, you have to be careful in how advance() calls computerProposals() so that the filter/delta parameters make sense from the perspective of the implementation of the computerProposals() method. Since we are passing this in separately, my expectation would that filter parameter is the old filter (as opposed to new filter with delta appended). I'd like to see these items resolved before taking a deeper dive into the implementation.
Created attachment 206021 [details] Patch_v5.2 Fixed the review comments 1. In ContentProposalService API, it seems that session( "abc" ).advance( "def" ) and session( null ).advance( "abcdef" ) are semantically equivalent. My preference would be to simplify this API by not having the session method take the parameter. The session can always start with an empty string and the first advance call can be used to establish context. Done, the filter value will be set in the first call to the advance() 2. It appears that the delta argument isn't being treated as a delta in computeProposals() and advance() method calls. I would expect to see something equivalent to this.filter = this.filter + delta in the advance() method. Similarly, I would expect the caller of advance to only pass in the delta (characters added to the last filter state or a suffix) instead of passing in the whole string. Further, you have to be careful in how advance() calls computerProposals() so that the filter/delta parameters make sense from the perspective of the implementation of the computerProposals() method. Since we are passing this in separately, my expectation would that filter parameter is the old filter (as opposed to new filter with delta appended). Ensured that the caller passes only the delta to the content proposal service and also ensured that when handling the computeProposals() we pass the oldFilter without delta
It looks like Patch v5.2 was not derived from v5.1. In particular, the fix previously applied to get content proposals for @PossibleValues to work is not there and the unit test is missing all the work shown in v5.1. Could you re-apply the last round of changes to v5.1 so that we can have a single patch that shows all the latest work?
Created attachment 206044 [details] Patch v6 I took an update of the current 0.4.x branch and on top of it i applied my v5 patch and then updated the sources to reflect your review comments. I have merged all the the patches from v5 to this v6. It includes test cases, docs, API related changes based on your review comments. I am still wondering how these things were missing though i was able to see them in my local patch :( ... Since mylyn is not working in Juno-42 am using the browser to upload the patch and I check the Content Type checkbox for "Patch" ... Hope you will have all the consolidated updates in this patch.
I've accepted Patch v6 with following modifications: 1. I re-wrote documentation content. There were two problems with content as contributed: (a) there was insufficient focus on ContentProposalService being generic API that can be implemented outside of possible values scenario, and (b) the attached screen capture images appear to be corrupted (nothing on my computer could open them). 2. I renamed ContentProposalInfo to ContentProposal. 3. I moved ContentProposalService and PossibleValuesContentProposalService to sapphire.modeling plugin. In my view these aren't specific to UI and there might be some further meshing between ContentProposalService and PossibleValuesService in the future. One could argue that ContentProposalService covers all PossibleValuesService usecases and as such PossibleValuesService is not necessary. Will need to consider this further... 4. Modified ContentProposal constructor to take ImageData instead of string. We should not assume any one specific method of image retrieval. 5. Added single-argument constructor to ContentProposal class. 6. The session interraction logic in ContentProposalProvider.getProposal() was quite wrong in a number of cases. It didn't account for position argument properly, it failed to update the filter if string length didn't change but it's content changed, etc. I was seeing exceptions and incorrect proposals in use. Re-wrote that block from scratch. 7. Moved ContentProposalLabelProvider and ImageContentProposal so that they are siblings to ContentProposalProvider instead of nested in it. 8. Removed auto activation characters logic. Auto activation characters are domain specific. For java types, a dot makes sense. For other domains, not so much. We will need to revisit this aspect late. 9. Cumulative filter with content proposals popup wasn't working initially. Not sure which one of my changes made it work, but it is working now. 10. Moved parsing of content assist key stroke into a static block since the resulting key stroke object is effectively a constant. 11. Removed ContentProposalService.DEFAULT_PROPOSAL_IMAGE. This piece was non-functional as written anyway. 12. ContentProposalService.Session.proposals() would return null after creation. The initial set of proposals needed to be computed during creation. 13. In PossibleValuesContentProposalService session compute method, during proposals initialization loop, quadratic algorithm was used to prevent duplicates. I don't believe this duplicates check is necessary as input is already de-duplicated. In either case, a quadratic algorithm shouldn't be used. Removed the de-duplication. 14. In PossibleValuesContentProposalService session compute method, the list of old proposals was modified. ContentProposalService was not doing anything to prevent the list belonging to its proposals() method from being modified. This is bad API practice. The compute method should be returning a new list. Modified the two classes accordingly. 15. ContentProposal.equals() was doing case-insensitive comparison. That's invalid. It is appropriate to match proposals in case-insensitive manner, but if PossibleValuesService returns "Abc" and "abc", both of those need to appear in the list of matched proposals. I removed equals() and hashCode() methods from ContentProposalInfo class as we need to examine any places where they might be necessary and evaluate what is actually needed in those situations. 16. Deleted TestModel.java from the test case. It was completely unused class. 17. Removed Unnecessary re-lookup of known properties in TestServices0004. Variety of other cleanup. 18. Consolidated four unit tests into two to reduce duplication of code/cycles. 19. Simplified ContentProposalService.computeProposals() method into a zero argument compute() method. The reason is that all of the information being passed in was already available via proposals() and filter() methods. We may need to revisit as part of implementing java type completion support, but this is all that's necessary today. 20. Removed "(non-Javadoc)" comments and other formulaic/generated comments. Re-wrote javadoc on API classes. Please verify.
Comment on attachment 205292 [details] Patch v5.1 Images Marking as obsolete since I was not able to open the contained images. In the future, when attaching images, it would be better to attach them directly instead of in a zip. That way a quick click-through can be used to see/verify images.
(In reply to comment #106) > Comment on attachment 205292 [details] > Patch v5.1 Images > > Marking as obsolete since I was not able to open the contained images. In the > future, when attaching images, it would be better to attach them directly > instead of in a zip. That way a quick click-through can be used to see/verify > images. Will take care of it future. Thanks.
Verified! Thanks a lot for making these changes, now it really looks good. Hope i will learn these tricks en-course of my sapphire contributions.
Thanks for verifying. Closing.
