Hi all Radu and I have completed the refactoring of the Sightly Engine API in our fork at [1] and I am currently committing the large patch resulting from that refactoring for SLING-4206 [2]. The major part of this patch is about moving classes into „impl“ packages.
So, there will be a number of commit messages coming along … Regards Felix [1] https://github.com/fmeschbe/sling.git [2] https://issues.apache.org/jira/browse/SLING-4206 > Am 25.11.2014 um 15:46 schrieb Felix Meschberger <[email protected]>: > > 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 >>>>>> >>>> >>>> >>>
