You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by Emmanuel Lécharny <el...@symas.com> on 2015/03/12 19:02:14 UTC
[Studio] Questions about LdifSourceViewerConfiguration class
Hi,
I'm currently reviewing a few classes, trying to remove some potential
NPE. While doing so, I found things that I'm not sure are necessary or
correct in the LdifSourceViewerConfiguration class. Typically :
line 165 :
damagerRepairer = new LdifDamagerRepairer( this.editor );
this.presentationReconciler.setDamager( damagerRepairer,
IDocument.DEFAULT_CONTENT_TYPE );
this.presentationReconciler.setRepairer( damagerRepairer,
IDocument.DEFAULT_CONTENT_TYPE );
this.presentationReconciler.setDamager( damagerRepairer,
LdifPartitionScanner.LDIF_RECORD );
this.presentationReconciler.setRepairer( damagerRepairer,
LdifPartitionScanner.LDIF_RECORD );
Why do we set the damaRepairer twice ? My understanding is that the
second setting is enough, am I correct ?
Same for line 217 :
this.contentAssistant = new DialogContentAssistant();
this.contentAssistProcessor = new
LdifCompletionProcessor( editor, contentAssistant );
this.contentAssistant.setContentAssistProcessor(
this.contentAssistProcessor,
LdifPartitionScanner.LDIF_RECORD );
this.contentAssistant.setContentAssistProcessor(
this.contentAssistProcessor,
IDocument.DEFAULT_CONTENT_TYPE );
we set the ContentAssistProcessor twice, with two different values, and
I suspect that the second setting is wrong.
Pardon me if these are dumb questions, I'm quite a virging in Eclipse
programming...
Thanks !
Re: [Studio] Questions about LdifSourceViewerConfiguration class
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 12/03/15 20:40, Stefan Seelmann a écrit :
> Hi Emmanuel,
>
> On 03/12/2015 07:48 PM, Emmanuel Lécharny wrote:
>> OTOH, all those setters are called in the
>> LdifEditorWidget.createWidget() method, when we call
>> sourceViewer.configure( sourceViewerConfiguration );
>>
>> That makes me wonder why we don't create all the elements in the
>> constructor. Any good reason ?
> To be honest I don't remember if there is/was a good reason. When
> looking into other implementations the objects are also created lazily
> when calling the getter, however the objects are not assigned to a
> member variable.
Yes, this is what I saw when looking for code samples on internet. My
understanding is that as all those methods have parameters that are not
available in the constructor, so it makes sense to have them initialized
in their setters.
>
> It would be interesting to test if the getters are called more than
> once.
I'll do that, but even if it's the case, the test (if blah == null then
create blah) cost almost nothing.
> You can try to move the creation to the constructor and see what
> happens. Please note that the class is not only used by the LDIF editor
> itself, but also from a number of ModificationLogsView, SearchLogsView,
> BatchOperationWizardPage, and LdifEditorSyntaxColoringPreferencePage.
Yep. I think I will leave the code as is.
>
> Another thought, I think e4 supports dependency injection, so maybe it
> makes sense to try to migrate (later, much much later) to e4 and avoid
> hand-wiring all the stuff.
Probably, but I'm way too a newbie in eclipse to do that :-)
FTR, I'm creating a LDIF anonymizer that I'd like to have added to the
Ldif editor, this is why I'm looking at this code (which is quite old...
I realized it was written more than 8 years ago !)
Thanks Stefan !
Re: [Studio] Questions about LdifSourceViewerConfiguration class
Posted by Stefan Seelmann <ma...@stefan-seelmann.de>.
Hi Emmanuel,
On 03/12/2015 07:48 PM, Emmanuel Lécharny wrote:
> OTOH, all those setters are called in the
> LdifEditorWidget.createWidget() method, when we call
> sourceViewer.configure( sourceViewerConfiguration );
>
> That makes me wonder why we don't create all the elements in the
> constructor. Any good reason ?
To be honest I don't remember if there is/was a good reason. When
looking into other implementations the objects are also created lazily
when calling the getter, however the objects are not assigned to a
member variable.
It would be interesting to test if the getters are called more than
once. You can try to move the creation to the constructor and see what
happens. Please note that the class is not only used by the LDIF editor
itself, but also from a number of ModificationLogsView, SearchLogsView,
BatchOperationWizardPage, and LdifEditorSyntaxColoringPreferencePage.
Another thought, I think e4 supports dependency injection, so maybe it
makes sense to try to migrate (later, much much later) to e4 and avoid
hand-wiring all the stuff.
Kind Regards,
Stefan
Re: [Studio] Questions about LdifSourceViewerConfiguration class
Posted by Emmanuel Lécharny <el...@gmail.com>.
OTOH, all those setters are called in the
LdifEditorWidget.createWidget() method, when we call
sourceViewer.configure( sourceViewerConfiguration );
That makes me wonder why we don't create all the elements in the
constructor. Any good reason ?
Le 12/03/15 19:23, Emmanuel Lécharny a écrit :
> Ok, another question.
>
> What's the best style : defining the elements like Reconciler,
> contentAssistent, etc, in the constructor, or instanciate them when they
> are requested like what is done in the flollowing code.
>
> public ITextDoubleClickStrategy getDoubleClickStrategy(
> ISourceViewer sourceViewer, String contentType )
> {
> if ( this.doubleClickStrategy == null )
> {
> this.doubleClickStrategy = new LdifDoubleClickStrategy();
> }
>
> return this.doubleClickStrategy;
> }
>
>
> vs
>
> public LdifSourceViewerConfiguration( ILdifEditor editor, boolean
> contentAssistEnabled )
> {
> super();
>
> this.editor = editor;
> this.contentAssistEnabled = contentAssistEnabled;
>
> // Instanciate the DoubleClickingStartegy
> doubleClickStrategy = new LdifDoubleClickStrategy();
> ...
> }
>
>
> public ITextDoubleClickStrategy getDoubleClickStrategy(
> ISourceViewer sourceViewer, String contentType )
> {
> return this.doubleClickStrategy;
> }
>
> My understanding is that the first style will only instanciate the
> elements when there are absolutely necessary (ie, only when they are
> used) vs a style where we create everything at startup, and return the
> element in the getter.
>
> My guts say that I prefer the second style, but my guts also tell me
> that there is a good reason why it's not done this way (like : why
> creating some resource which will never be used ?)
>
> Thanks !
Re: [Studio] Questions about LdifSourceViewerConfiguration class
Posted by Emmanuel Lécharny <el...@gmail.com>.
Ok, another question.
What's the best style : defining the elements like Reconciler,
contentAssistent, etc, in the constructor, or instanciate them when they
are requested like what is done in the flollowing code.
public ITextDoubleClickStrategy getDoubleClickStrategy(
ISourceViewer sourceViewer, String contentType )
{
if ( this.doubleClickStrategy == null )
{
this.doubleClickStrategy = new LdifDoubleClickStrategy();
}
return this.doubleClickStrategy;
}
vs
public LdifSourceViewerConfiguration( ILdifEditor editor, boolean
contentAssistEnabled )
{
super();
this.editor = editor;
this.contentAssistEnabled = contentAssistEnabled;
// Instanciate the DoubleClickingStartegy
doubleClickStrategy = new LdifDoubleClickStrategy();
...
}
public ITextDoubleClickStrategy getDoubleClickStrategy(
ISourceViewer sourceViewer, String contentType )
{
return this.doubleClickStrategy;
}
My understanding is that the first style will only instanciate the
elements when there are absolutely necessary (ie, only when they are
used) vs a style where we create everything at startup, and return the
element in the getter.
My guts say that I prefer the second style, but my guts also tell me
that there is a good reason why it's not done this way (like : why
creating some resource which will never be used ?)
Thanks !
Re: [Studio] Questions about LdifSourceViewerConfiguration class
Posted by Emmanuel Lécharny <el...@gmail.com>.
Ahhh, my bad.
We have two content types, DEFAULT_CONTENT_TYPE and LDIF_RECORD, so the
double registration makes sense.
Le 12/03/15 19:02, Emmanuel Lécharny a écrit :
> Hi,
>
> I'm currently reviewing a few classes, trying to remove some potential
> NPE. While doing so, I found things that I'm not sure are necessary or
> correct in the LdifSourceViewerConfiguration class. Typically :
>
> line 165 :
>
> damagerRepairer = new LdifDamagerRepairer( this.editor );
>
> this.presentationReconciler.setDamager( damagerRepairer,
> IDocument.DEFAULT_CONTENT_TYPE );
> this.presentationReconciler.setRepairer( damagerRepairer,
> IDocument.DEFAULT_CONTENT_TYPE );
>
> this.presentationReconciler.setDamager( damagerRepairer,
> LdifPartitionScanner.LDIF_RECORD );
> this.presentationReconciler.setRepairer( damagerRepairer,
> LdifPartitionScanner.LDIF_RECORD );
>
> Why do we set the damaRepairer twice ? My understanding is that the
> second setting is enough, am I correct ?
>
> Same for line 217 :
> this.contentAssistant = new DialogContentAssistant();
>
> this.contentAssistProcessor = new
> LdifCompletionProcessor( editor, contentAssistant );
> this.contentAssistant.setContentAssistProcessor(
> this.contentAssistProcessor,
> LdifPartitionScanner.LDIF_RECORD );
> this.contentAssistant.setContentAssistProcessor(
> this.contentAssistProcessor,
> IDocument.DEFAULT_CONTENT_TYPE );
>
> we set the ContentAssistProcessor twice, with two different values, and
> I suspect that the second setting is wrong.
>
> Pardon me if these are dumb questions, I'm quite a virging in Eclipse
> programming...
>
> Thanks !