Hi Radu Cool, thanks. I'll review and get back to you.
Regards Felix -- Typos caused by my iPhone > Am 25.11.2014 um 13:39 schrieb Radu Cotescu <[email protected]>: > > Hi Felix, > > The API refactoring should now be completed - I've refactored the > RenderContext as well and there's no UnitLocator anymore (its usage was > restricted to only the RenderUnitProvider which can "locate" units by > itself). > > We can review the changes before deciding to merge them to Sling trunk if > you think that we might improve some other areas. > > Regards, > Radu > > > >> On Tue, Nov 25, 2014 at 12:05 AM, Radu Cotescu <[email protected]> wrote: >> >> Hi Felix, >> >> I've pushed some new changes: BaseRenderUnit is now a part of impl. >> RenderContext should indeed become an interface. Also, I think that we can >> get rid of the UnitLocator interface, therefore reducing the size of the >> exported API to the bare minimum. >> >> Thanks, >> Radu >> >> On Mon, Nov 24, 2014 at 12:36 PM, Felix Meschberger <[email protected]> >> wrote: >> >>> Hi Radu >>> >>>> Am 21.11.2014 um 18:39 schrieb Radu Cotescu <[email protected]>: >>>> >>>> Hi Felix, >>>> >>>> I pushed all the required changes to the forked repo. Things left to do: >>>> >>>> * check what we actually need to export for preventing class loading >>>> problems >>> >>> Excellent. I would prefer to really not export most of the RenderUnit >>> stuff incl. BaseRenderUnit and RenderContext. >>> >>> At one point I had a change, where I actually had RenderContext as an >>> interface with a subset of the API, basically just getWriter (does that >>> need to be a StackedWriter after all ?) and a RenderContextImpl which had >>> all the rest but was internal. >>> >>> >>>> * expand JavaDoc >>>> >>>> At this point I think that we can create a patch with our changes and >>> apply >>>> it to trunk. >>> >>> We might want to continue on the fork to find the classes that we really >>> want/need to export. Because this may cause further shuffling of packages >>> etc. >>> >>> >>> Thanks >>> Felix >>> >>>> >>>> Cheers, >>>> Radu >>>> >>>> On Thu, Nov 20, 2014 at 6:53 PM, Felix Meschberger <[email protected]> >>>> wrote: >>>> >>>>> Hi >>>>> >>>>> Thanks for your reply. >>>>> >>>>> For those interested in following, Radu and I are cooperating on these >>>>> changes in my GitHub fork at https://github.com/fmeschbe/sling >>>>> >>>>> >>>>>> Am 19.11.2014 um 22:53 schrieb Radu Cotescu <[email protected]>: >>>>>> >>>>>> Hi Felix, >>>>>> >>>>>> The API reorganisation looks good to me. >>>>>> >>>>>> On Wed, Nov 19, 2014 at 10:49 PM, Felix Meschberger < >>> [email protected]> >>>>>> wrote: >>>>>> >>>>>>> Apart from splitting the base .api package, this are further >>>>> discussions: >>>>>>> >>>>>>> * Exceptions are forming a hierarchy with SightlyEngineException >>> being >>>>> the >>>>>>> root and itself extending from SlingException >>>>>>> >>>>>> >>>>>> Sure, it makes sense. >>>>> >>>>> Done in the fork. The „root“ exception for Sightly is not >>> SightlyException >>>>> (SightlyEngineException renamed to SightlyException). >>>>> >>>>>> >>>>>> >>>>>>> * I wonder, whether we should really drop the *Component abstract >>>>> classes >>>>>>> (RuntimeExtensionComponent and UseProviderComponent). I don’t think >>> they >>>>>>> provide real value at all but have an activate method, which they >>>>> expect to >>>>>>> be called and which they expect to be a DS component annotated with >>> the >>>>>>> Felix annotations. I think this is brittle. >>>>>>> >>>>>> >>>>>> The abstract classes were written specifically for that activate >>> method, >>>>> in >>>>>> order to avoid writing those lines of code for every implementation. >>>>> >>>>> We are working on removing these abstract classes by leveraging service >>>>> properties for >>>>> >>>>> * UseProvider ranking (service.ranking) >>>>> * ExtensionProvider naming >>>>> >>>>>> >>>>>> >>>>>> >>>>>>> * UseProvider defines an ordering method and declares itself being >>>>>>> Comparable. I think this is wrong. The UseProviders should be sorted >>>>> using >>>>>>> regular OSGi service ranking. >>>>>>> >>>>>> >>>>>> If we make the service.ranking property configurable (metatype = >>> true) I >>>>>> guess we could use the OSGi ranking. Theoretically the Use providers >>> can >>>>> be >>>>>> configured to be called in a pre-determined order, depending on how >>> the >>>>>> Sightly projects from an instance make use of the Use-API (Java >>> Use-API >>>>>> objects deployed in bundles / Java Use objects near the component >>> script >>>>> in >>>>>> the repo / JS Use-API, etc.). Having the Use providers called in the >>> most >>>>>> favourable way can increase performance for Sightly applications. >>>>> >>>>> Yes, the service.ranking property can be added to metatype for >>>>> configurable ranking. >>>>> >>>>>> >>>>>> >>>>>>> * I wonder, whether we really have to have all this API exported at >>> all: >>>>>>> Some API I don’t see implemented at all (e.g. BaseRenderUnit — or is >>>>> that >>>>>>> the base class for the Java classes generated from the Sightly >>>>> templates ?) >>>>>>> >>>>>> >>>>>> The BaseRenderUnit is indeed the base class for classes generated from >>>>>> Sightly scripts, which is why we need to export the other classes as >>>>> well. >>>>>> In this regard Sightly is no different from the JSP engine. >>>>> >>>>> Ok. We’d have to test this more. But I suspect we don’t really need to >>>>> export the BaseRenderUnit abstract class. >>>>> >>>>> IIRC the reason for exporting the base JSP servlet is that other >>> classes >>>>> in the same package must be exported. >>>>> >>>>>> >>>>>> >>>>>>> * Last but not least (actually most important of all): There is >>> close to >>>>>>> no JavaDoc. Before cutting a release of this bundle, JavaDoc must be >>>>>>> provided for the API. For example I am not sure, what the Record or >>> the >>>>>>> ObjectModel interfaces are really used for. >>>>>>> >>>>>> >>>>>> It's on my TODO list. >>>>> >>>>> Cool. Thanks. >>>>> >>>>> Regards >>>>> Felix >>>>> >>> >>> >>
