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 >> >> >> >> >
