FeatureInstaller.getInstance() and some more Contextualization

[edeso]

also reformatting for mixed spaces/tab indentationed files

[edeso]

this will break some extensions too. let's wait to update them until the I18N changes https://sourceforge.net/p/jump-pilot/bugs/501/ are submitted as well.

[mukoki]

Can you explain the rational of this change Ede ? Everytime I see static methods in the code, I feel like we are loosing something about encapsulation and we are uselessly exposing the system memory. Maybe a purely theoretical example, but will the code be able to manage two workbench if FeatureInstaller is only accessible from FeatureInstaller.getInstance() ?

[edeso]

Can you explain the rational of this change Ede ?

sure :)

Everytime I see static methods in the code, I feel like we are loosing something about encapsulation and we are uselessly exposing the system memory.

the opposite is true. static code actually uses the same memory for each call, which means we are saving memory and useless instantiation computing cycles

Maybe a purely theoretical example, but will the code be able to manage two workbench if FeatureInstaller is only accessible from FeatureInstaller.getInstance() ?

of course not as it is using one identical WorkbenchContext. but i really don't see an issue here as the reason to run two workbenches in one jre is completely lost to me totally ignoring the fact that this functionality was never implemented in the first place.

[edeso]

main reason for the change is actually speeding up OJ start by reusing the same FeatInstaller instead of instantiating copies all over doing the same thing!

[edeso]

Mike,

i'm preparing the I18N changes https://sourceforge.net/p/jump-pilot/bugs/501/ and really like to add both extension breaking patch sets at the sime time. do you still have issues wrt. the above pull?

[mukoki]

Sorry for the delay. I'm still not convinced by the change. If the main reason is to avoid useless instantiations of FeatureInstaller, which I can understand, why not make it an attribute of JUMPWorkbench, accessible through WorkbenchContext and PlugInContext so that all plugins of the JUMPWorkbench singleton can access the same instance of FeatureInstaller during PlugIn initialization ? Also it could be less risky to make FeatureInstaller constructor deprecated instead of private. Using a static mehod or a JUMPWorkbench attribute may not make a big difference as far as JUMPWorkbench is a singleton created by our batch file, but I feel like it could become a problem if we wanted to instantiate workbench programmatically (e.g. for testing). Please, have a second thought about it. No problem about changing #501 at the same time, and ready to review all the extensions after that.

[edeso]

Sorry for the delay.

no problem. busy myself

I'm still not convinced by the change. If the main reason is to avoid useless instantiations of FeatureInstaller, which I can understand, why not make it an attribute of JUMPWorkbench, accessible through WorkbenchContext and PlugInContext so that all plugins of the JUMPWorkbench singleton can access the same instance of FeatureInstaller during PlugIn initialization ?

sounds reasonable

Also it could be less risky to make FeatureInstaller constructor deprecated instead of private.

we are breaking backward compatibility with OJ2 anyway. what risk are you talking about?

Using a static mehod or a JUMPWorkbench attribute may not make a big difference as far as JUMPWorkbench is a singleton created by our batch file, but I feel like it could become a problem if we wanted to instantiate workbench programmatically (e.g. for testing). Please, have a second thought about it.

that's why i put up the pull req. instead of committing it immediatly :)

No problem about changing #501 at the same time, and ready to review all the extensions after that.

i prepared some changes locally, the patchsets will

• do the I18N changes
• example fix skyprinter, adding mvn zip distro packaging
• example add a mvn get skyprinter distro zip instead of needing to add to trunk

please be patient

[mukoki]

Sorry for the delay.

no problem. busy myself

I'm still not convinced by the change. If the main reason is to avoid useless instantiations of FeatureInstaller, which I can understand, why not make it an attribute of JUMPWorkbench, accessible through WorkbenchContext and PlugInContext so that all plugins of the JUMPWorkbench singleton can access the same instance of FeatureInstaller during PlugIn initialization ?

sounds reasonable

Also it could be less risky to make FeatureInstaller constructor deprecated instead of private.

we are breaking backward compatibility with OJ2 anyway. what risk are you talking about?

Maybe an extension which would not depend on JTS is still working with OJ2, but would be definitely broken with this change. Just a guess. But generally, I agree that it is a good time to make breaking change.

Using a static mehod or a JUMPWorkbench attribute may not make a big difference as far as JUMPWorkbench is a singleton created by our batch file, but I feel like it could become a problem if we wanted to instantiate workbench programmatically (e.g. for testing). Please, have a second thought about it.

that's why i put up the pull req. instead of committing it immediatly :)

No problem about changing #501 at the same time, and ready to review all the extensions after that.

i prepared some changes locally, the patchsets will

• do the I18N changes
• example fix skyprinter, adding mvn zip distro packaging
• example add a mvn get skyprinter distro zip instead of needing to add to trunk

please be patient

[edeso]

On 13.06.2021 12:16, Michaël Michaud wrote:

Also it could be less risky to make FeatureInstaller constructor deprecated instead of private. we are breaking backward compatibility with OJ2 anyway. what risk are you talking about? Maybe an extension which would not depend on JTS is still working with OJ2, but would be definitely broken with this change. Just a guess. But generally, I agree that it is a good time to make breaking change.

that's getting even more unlikely after the I18N API change. let's agree to assume that legacy extensions are broken with OJ2, period ;)

[edeso]

how about changing API to FeatureInstaller.getInstance(WorkbenchContext context) but still privatizing the constructor, to control instance creation? i still don't see where we will need multiple contexts for now, but this would enable the reuse of the same installer instance while maintaining the possiblility to create different instances. so it'll be possible in some future.

[mukoki]

how about changing API to FeatureInstaller.getInstance(WorkbenchContext context) but still privatizing the constructor, to control instance creation? i still don't see where we will need multiple contexts for now, but this would enable the reuse of the same installer instance while maintaining the possiblility to create different instances. so it'll be possible in some future.

I'm OK with this ;-)

[edeso]

On 13.06.2021 19:35, Michaël Michaud wrote:

how about changing API to FeatureInstaller.getInstance(WorkbenchContext context) but still privatizing the constructor, to control instance creation? i still don't see where we will need multiple contexts for now, but this would enable the reuse of the same installer instance while maintaining the possiblility to create different instances. so it'll be possible in some future.

I'm OK with this ;-)

finally :))). will prepare a new or update the pull request.

[edeso]

hey Mike,

updated as requested. i'm ready to pull this and https://github.com/openjump-gis/openjump/pull/35 and https://github.com/openjump-gis/openjump/pull/36 if you are too?! As you insisted on encapsulation, you will probably want JUMPWorkbench.getInstance() removed as well?

..sunny regards ede

[mukoki]

Ede,I'll have a look on your changes this week-end if you don't mind. Mostly because i need to understand these changes as I will have to use them soon.You're probably right about JUMPWorkbench.getInstance. I thought that it was OK as long as we can instanciate a new workbench from a public constructor, but there are probably many places were we use JUMPWorkbench.getInstance() which would break if used with another instance. I think that the proper way is to use PlugInContext and WorkbenchContext where possible.Michaëlenvoyé : 18 juin 2021 à 13:19de : edeso @.>à : openjump-gis/openjump @.>cc : Michaël Michaud @.>, Comment @.>objet : Re: [openjump-gis/openjump] Privatize FeatureInstaller constructor in favour of FeatureInstaller.getInstance() (#29) hey Mike,updated as requested. i'm ready to pull this and #35 and #36 if you are too?! As you insisted on encapsulation, you will probably want JUMPWorkbench com.vividsolutions.jump.workbench.JUMPWorkbench.getInstance() removed as well?..sunny regards ede—You are receiving this because you commented.Reply to this email directly, view it on GitHub, or unsubscribe.

[edeso]

You're probably right about JUMPWorkbench.getInstance. I thought that it was OK as long as we can instanciate a new workbench from a public constructor, but there are probably many places were we use JUMPWorkbench.getInstance() which would break if used with another instance. I think that the proper way is to use PlugInContext and WorkbenchContext where possible.

still not convinced about the need for multiple workbenches (even tests would probably just start on workbench for one test and start another for the next). always felt that passing around contexts is just unnecessary bother, when on static class can hold all the references needed. but ok, it was initially designed that way, so we might just keep it, my opinion on that is not really strong :).

let me add the JUMPWorkbench.getInstance() removal on top the FeatureInstaller branch, then there is less to synchronize later. feel free to check out the pull requests as they are already, should be pretty self explanatory!

[edeso]

let me add the JUMPWorkbench.getInstance() removal on top the FeatureInstaller branch, then there is less to synchronize later. feel free to check out the pull requests as they are already, should be pretty self explanatory!

yeah. no. just checked and this goes deeply into some classes, lacking access to some proper context at all. just removed it from AbstractCursorTool (https://github.com/openjump-gis/openjump/commit/54930f66589e75acf357c5977217d9489da0e88d) but already for the next one namely AbstractPlugin i have no clue on how to provide it with a context. problem is that plugins are not initialized with a mandatory context, but instead retrieve one on each initialize(), execute() call. but not every plugin will prob run super.{initialize,execute}()

we probably need to reconsider this now as we have the opportunity to properly fix that (breaking backward compat anyway). but i'll open an extra issue bout that next week i think.

[mukoki]

Ede,

Let me still make a suggestion on this one. To maintain a single FeatureInstaller per WorkbenchContext, you added a static Map<WorkbenchContext,FeatureInstaller> in the FeatureInstaller class. To achieve the same goal, wouldn't it be simpler to add a unique FeatureInstaller in each WorkbenchContext ? Here is my suggestion. with some comments just after

public abstract class WorkbenchContext implements... {
    abstract FeatureInstaller getFeatureInstaller();
}

JUMPWorkbenchContext extends WorkbenchContext {
    private FeatureInstaller featureInstaller = null;
    FeatureInstaller getFeatureInstaller() {
        if (featureInstaller == null) {
            featureInstaller = new Featureinstaller(this);
        }
        return featureInstaller;
    }
}

PlugInContext implements ... {
    public FeatureInstaller getFeatureInstaller() {
        return getWorkbenchContext().getFeatureInstaller();
    }
}

I think, it is

• simpler
• potentially avoid memory retention in a static map (if a workbench and its workbenchContext are garbage collected - purely hypothetical at the moment)
• I think that it is not completely safe to use an object which does not implement equals/hashmap as a map key

Maybe implementation of the getFeatureInstaller can take place in the abstract WorkbenchContext class to be even simpler. Not sure if we can make a reference to "this" in an abstract class though.

One drawback of my solution is that it changes the signature of workbenchcontext. I don't think it is a big problem but maybe you will see other drawbacks I did not see.

[edeso]

I think, it is

• simpler

i'd say just different

• potentially avoid memory retention in a static map (if a workbench and its workbenchContext are garbage collected - purely hypothetical at the moment)

let's rethink this in case we ever cross a bridge called "more than one workbench running, at one time or after one another " not earlier. we keep WorkbenchContext references all over, because of the JUMP design, so it's going to be a whole new "fun" puzzle to solve.

• I think that it is not completely safe to use an object which does not implement equals/hashmap as a map key

lost you here. what is your point?

Maybe implementation of the getFeatureInstaller can take place in the abstract WorkbenchContext class to be even simpler. Not sure if we can make a reference to "this" in an abstract class though.

One drawback of my solution is that it changes the signature of workbenchcontext. I don't think it is a big problem but maybe you will see other drawbacks I did not see.

well mainly the initial one., namely Privatizing FeatureInstaller constructor to prevent duplicate instances ;)

[mukoki]

I think, it is

• simpler

i'd say just different

• potentially avoid memory retention in a static map (if a workbench and its workbenchContext are garbage collected - purely hypothetical at the moment)

let's rethink this in case we ever cross a bridge called "more than one workbench running, at one time or after one another " not earlier. we keep WorkbenchContext references all over, because of the JUMP design, so it's going to be a whole new "fun" puzzle to solve.

OK, I don't need it now. I Just wanted to avoid a later breaking change. But if we don't care about the case where several instances of Workbench are running, why do we care about having several WorkbenchContext ? Does it make sense to have several context (and to add a static map to track references to these instances) ?

• I think that it is not completely safe to use an object which does not implement equals/hashmap as a map key

lost you here. what is your point?

OK, forget this one. I Just wondered if, for a particular workbench, the workbenchContext instance was always the same instance, but it seems to be the case as it is instantiated with the final keyword : private final WorkbenchContext context = new JUMPWorkbenchContext(this))

Maybe implementation of the getFeatureInstaller can take place in the abstract WorkbenchContext class to be even simpler. Not sure if we can make a reference to "this" in an abstract class though. One drawback of my solution is that it changes the signature of workbenchcontext. I don't think it is a big problem but maybe you will see other drawbacks I did not see.

well mainly the initial one., namely Privatizing FeatureInstaller constructor to prevent duplicate instances ;)

Good point. I think we could keep the FeatureInstaller private in my proposition too, but I see that none of my main arguments has convinced you, so go ahead as it is. It is an improvement anyway.

[edeso]

I think, it is

• simpler

i'd say just different

• potentially avoid memory retention in a static map (if a workbench and its workbenchContext are garbage collected - purely hypothetical at the moment)

let's rethink this in case we ever cross a bridge called "more than one workbench running, at one time or after one another " not earlier. we keep WorkbenchContext references all over, because of the JUMP design, so it's going to be a whole new "fun" puzzle to solve.

OK, I don't need it now. I Just wanted to avoid a later breaking change. But if we don't care about the case where several instances of Workbench are running, why do we care about having several WorkbenchContext ?

just saying it works for now and enabling multiple Workbenches is so far away that we shouldn't worry to much about it right now

Does it make sense to have several context (and to add a static map to track references to these instances) ?

hey, you started the multiple workbench issue. i for myself am totally fine with exactly one workbench, which i started implementing years ago (JumpWorkbench.getInstance()), but you obviously think might not be sufficent. hence i started reimplenting the original design which was never completely finished anyway.

you gotta choose ;) there ist the difficult context everywhere method or the simple one workbench, less context that i was heading for. i'm good with either approach.

if we choose multi workbench then we'll need the hashmap to track instances, yes.

• I think that it is not completely safe to use an object which does not implement equals/hashmap as a map key

lost you here. what is your point?

OK, forget this one. I Just wondered if, for a particular workbench, the workbenchContext instance was always the same instance, but it seems to be the case as it is instantiated with the final keyword : private final WorkbenchContext context = new JUMPWorkbenchContext(this))

forgotten already. what was it we were talking about :))

Maybe implementation of the getFeatureInstaller can take place in the abstract WorkbenchContext class to be even simpler. Not sure if we can make a reference to "this" in an abstract class though. One drawback of my solution is that it changes the signature of workbenchcontext. I don't think it is a big problem but maybe you will see other drawbacks I did not see.

well mainly the initial one., namely Privatizing FeatureInstaller constructor to prevent duplicate instances ;)

Good point. I think we could keep the FeatureInstaller private in my proposition too, but I see that none of my main arguments has convinced you, so go ahead as it is. It is an improvement anyway.

look above. the PR holds 3 patches now, one of them doing exactly that to the FeatureInstaller as well ;)

[mukoki]

Ede, I suggest you go ahead and merge this pull request as is. I tried to explain what I think could be improved, but there is nothing I can't live without, and I'm not inclined to invest more time in this area. I understand that you did not work with the hypothesis that there can be multiple workbench instances, which is OK for me at the moment. In this context most of my remarks probably do not fit well with your PR, so it is better to keep it as is so that we preserve its consistency.