search

openjump-gis / openjump

OpenJUMP, the Open Source GIS with more than one trick in its kangaroo pocket, takes the leap from svn to git. join the effort!
http://openjump.org
GNU General Public License v2.0
28 stars 14 forks source link

rework I18N as outlined in sf.net ticket #501 (new version) #35

Closed edeso closed 3 years ago

edeso commented 3 years ago

mistakenly closed other pull request #34 can't be reopened.

create this new one.

mukoki commented 3 years ago

Ede,

I tried to update an extension (measure-toolbox-extension) using this PR. The main problem I found is with the classloader. Firstly, I have just created a new I18N instance using the constructor with the String parameter and using a categoryPrefix pointing to the extension bundle. But it did not load until I added also the following static instruction : static {I18N.setClassLoader(MeasureExtension.class.getClassLoader());} This is probably not the intended way to create/initialize an I18N extension. Maybe we need to pass a class of the extension to the constructor so that we can get the right classloader. You probably know this problem better than me.

There two other non blocking points I'd like to share : In the first version of the PR, I asked about a static I18N.get() and you answered that we can't have both a static get() and an instance get() method. Right. But what about a static attribute I18N = I18N.getInstance() so that we can do I18N.get() from this instance rather than I18N.getInstance().get() everytime? Not a big deal, but having to do I18N.getInstance() let me feel that it is more difficult to get a value now than before for the most frequent usage.

Another point : to adapt measure extension, I had to rename the resource bundle as it was not names category/language/jump. Wonder why we would have to add language/jump automatically to the bundle name. Letting the user decide about the exact path and name of its bundle seems enough and more flexible to me. What do you think ?

And not to omit anything from my test, there are two errors in the javadoc.

edeso commented 3 years ago

Ede,

I tried to update an extension (measure-toolbox-extension) using this PR. The main problem I found is with the classloader. Firstly, I have just created a new I18N instance using the constructor with the String parameter and using a categoryPrefix pointing to the extension bundle. But it did not load until I added also the following static instruction : static {I18N.setClassLoader(MeasureExtension.class.getClassLoader());} This is probably not the intended way to create/initialize an I18N extension. Maybe we need to pass a class of the extension to the constructor so that we can get the right classloader. You probably know this problem better than me.

shouldn't be necessary as the PluginClassloader should load all by now. can you clarify on your setup? what are your run parameters? how do you load the extension during development?

There two other non blocking points I'd like to share : In the first version of the PR, I asked about a static I18N.get() and you answered that we can't have both a static get() and an instance get() method. Right.

static solution i see is just adding a static field in class for reusage

static I18N i18n = I18N.getInstance("my.cool.extension");

But what about a static attribute I18N = I18N.getInstance() so that we can do I18N.get() from this instance rather than I18N.getInstance().get() everytime? Not a big deal, but having to do I18N.getInstance() let me feel that it is more difficult to get a value now than before for the most frequent usage.

what do you mean. can you give a more elaborate example?

Another point : to adapt measure extension, I had to rename the resource bundle as it was not names category/language/jump. Wonder why we would have to add language/jump automatically to the bundle name. Letting the user decide about the exact path and name of its bundle seems enough and more flexible to me. What do you think ?

it was a default i kept from the original implementation. felt it was a good default setting. it's described in the I18N.get() javadoc. simply use the File parameter to freely choose how you want to name your language files e.g.

I18N.getInstance(new File("my/cool/extension/messages")); will look for my/cool/extension/translations/messages.properties my/cool/extension/translations/messages_fr.properties. my/cool/extension/translations/messages_fr_FR.properties. ...

And not to omit anything from my test, there are two errors in the javadoc.

namely? feel free to comment in the commit!.. ede

edeso commented 3 years ago

And not to omit anything from my test, there are two errors in the javadoc.

namely? feel free to comment in the commit!.. ede

or even better offer a pull request to my branch ;)

mukoki commented 3 years ago

Ede, I tried to update an extension (measure-toolbox-extension) using this PR. The main problem I found is with the classloader. Firstly, I have just created a new I18N instance using the constructor with the String parameter and using a categoryPrefix pointing to the extension bundle. But it did not load until I added also the following static instruction : static {I18N.setClassLoader(MeasureExtension.class.getClassLoader());} This is probably not the intended way to create/initialize an I18N extension. Maybe we need to pass a class of the extension to the constructor so that we can get the right classloader. You probably know this problem better than me.

shouldn't be necessary as the PluginClassloader should load all by now. can you clarify on your setup? what are your run parameters? how do you load the extension during development?

I made a CORE distribution from your i18n branch. I changed the measure extension in order to use the new I18N class. I instanciated it in the MeasureExtension class like this 

static { I18N.setClassLoader(MeasureExtension.class.getClassLoader());} // get english keys without this
public static I18N I18NPlug = I18N.getInstance("org.openjump.core.ui.plugin.measuretoolbox");

There two other non blocking points I'd like to share : In the first version of the PR, I asked about a static I18N.get() and you answered that we can't have both a static get() and an instance get() method. Right.

static solution i see is just adding a static field in class for reusage

static I18N i18n = I18N.getInstance("my.cool.extension");

But what about a static attribute I18N = I18N.getInstance() so that we can do I18N.get() from this instance rather than I18N.getInstance().get() everytime? Not a big deal, but having to do I18N.getInstance() let me feel that it is more difficult to get a value now than before for the most frequent usage.

what do you mean. can you give a more elaborate example?

Yes, I mean for OpenJUMP itself, it could be static I18N I18N = I18N.getInstance();

Another point : to adapt measure extension, I had to rename the resource bundle as it was not names category/language/jump. Wonder why we would have to add language/jump automatically to the bundle name. Letting the user decide about the exact path and name of its bundle seems enough and more flexible to me. What do you think ?

it was a default i kept from the original implementation. felt it was a good default setting. it's described in the I18N.get() javadoc. simply use the File parameter to freely choose how you want to name your language files e.g.

Agree it is a good default setting. Just wonder if it has to be a mandatory setting (mainly because several extensions currently don't use this defaut)

I18N.getInstance(new File("my/cool/extension/messages")); will look for my/cool/extension/translations/messages.properties my/cool/extension/translations/messages_fr.properties. my/cool/extension/translations/messages_fr_FR.properties. ...

Oh, I thought that File was used for the case where bundle is external (not packaged into the jar). But I see that File is just processed as a String. Wonder why it has been added now. String parameter transformed by the addition of "language/jump" suffix and File parameter is interpreted as a String without adding language/jump. Sounds strange to me. Wouldn't it be enough to have String parameter without adding anything automatically ?

And not to omit anything from my test, there are two errors in the javadoc.

namely? feel free to comment in the commit!.. ede

edeso commented 3 years ago

I made a CORE distribution from your i18n branch. I changed the measure extension in order to use the new I18N class. I instanciated it in the MeasureExtension class like this

static { I18N.setClassLoader(MeasureExtension.class.getClassLoader());} // get english keys without this
public static I18N I18NPlug = I18N.getInstance("org.openjump.core.ui.plugin.measuretoolbox");

please push a branch to the repo and i'll try to reproduce your issue. when tested with skyprinter all was well.

do i understand correctly

  1. you packaged a CORE snapshot
  2. replaced the contained extension jar with new one ?

There two other non blocking points I'd like to share : In the first version of the PR, I asked about a static I18N.get() and you answered that we can't have both a static get() and an instance get() method. Right.

static solution i see is just adding a static field in class for reusage static I18N i18n = I18N.getInstance("my.cool.extension");

But what about a static attribute I18N = I18N.getInstance() so that we can do I18N.get() from this instance rather than I18N.getInstance().get() everytime? Not a big deal, but having to do I18N.getInstance() let me feel that it is more difficult to get a value now than before for the most frequent usage.

what do you mean. can you give a more elaborate example?

Yes, I mean for OpenJUMP itself, it could be static I18N I18N = I18N.getInstance();

for a static final constant capitalization would make sense. but are you actually suggesting to place it in I18N class like I18N.I18N ? looks a bit stupid. if you wanna go that way I18N.JUMP would probably be better.

i was considering a shortcut like gettext offers, e.g. _(key,param1,param2,...) (underscore), that'd would make CORE pretty clean, but be only usable in CORE but not for extensions.

Another point : to adapt measure extension, I had to rename the resource bundle as it was not names category/language/jump. Wonder why we would have to add language/jump automatically to the bundle name. Letting the user decide about the exact path and name of its bundle seems enough and more flexible to me. What do you think ?

it was a default i kept from the original implementation. felt it was a good default setting. it's described in the I18N.get() javadoc. simply use the File parameter to freely choose how you want to name your language files e.g.

Agree it is a good default setting. Just wonder if it has to be a mandatory setting (mainly because several extensions currently don't use this defaut)

i like it. it enables you to develop your extension in package 'my.cool.ext' and just deliver the same package as String and your files in language/jump* will automagically work. also it kind of enforces a default, which will make it easier for other devs finding a known file layout in your extension.

I18N.getInstance(new File("my/cool/extension/messages")); will look for my/cool/extension/translations/messages.properties my/cool/extension/translations/messages_fr.properties. my/cool/extension/translations/messages_fr_FR.properties. ...

Oh, I thought that File was used for the case where bundle is external (not packaged into the jar). But I see that File is just processed as a String. Wonder why it has been added now. String parameter transformed by the addition of "language/jump" suffix and File parameter is interpreted as a String without adding language/jump. Sounds strange to me. Wouldn't it be enough to have String parameter without adding anything automatically ?

again i just adopted what was there and that is why the String version is called categoryPrefix and the File variant path. the getInstance methods can maybe have a better documentation on that or point to get() javadoc at least, which can be enhanced as well, ture.

And not to omit anything from my test, there are two errors in the javadoc.

namely? feel free to comment in the commit!.. ede

are those coming?

mukoki commented 3 years ago

I made a CORE distribution from your i18n branch. I changed the measure extension in order to use the new I18N class. I instanciated it in the MeasureExtension class like this

static { I18N.setClassLoader(MeasureExtension.class.getClassLoader());} // get english keys without this
public static I18N I18NPlug = I18N.getInstance("org.openjump.core.ui.plugin.measuretoolbox");

please push a branch to the repo and i'll try to reproduce your issue. when tested with skyprinter all was well.

do i understand correctly

  1. you packaged a CORE snapshot
  2. replaced the contained extension jar with new one ?

Yes, exactly Here you can upload or clone the measure extension I used for the test https://github.com/mukoki/oj-measure-extension-i18n-test

There two other non blocking points I'd like to share : In the first version of the PR, I asked about a static I18N.get() and you answered that we can't have both a static get() and an instance get() method. Right.

static solution i see is just adding a static field in class for reusage static I18N i18n = I18N.getInstance("my.cool.extension");

But what about a static attribute I18N = I18N.getInstance() so that we can do I18N.get() from this instance rather than I18N.getInstance().get() everytime? Not a big deal, but having to do I18N.getInstance() let me feel that it is more difficult to get a value now than before for the most frequent usage.

what do you mean. can you give a more elaborate example?

Yes, I mean for OpenJUMP itself, it could be static I18N I18N = I18N.getInstance();

for a static final constant capitalization would make sense. but are you actually suggesting to place it in I18N class like I18N.I18N ? looks a bit stupid. if you wanna go that way I18N.JUMP would probably be better.

Why stupid ? Just make a static import and you just have to write I18N.get(key), which is simple and familiar for everyone who has already developped with OpenJUMP. I can't see the necessity to change the word to JUMP which says nothing about what it is and what it does. i was considering a shortcut like gettext offers, e.g. _(key,param1,param2,...) (underscore), that'd would make CORE pretty clean, but be only usable in CORE but not for extensions.

Same remark as for JUMP. I prefer something more explicit. _ is a bit criptic and I can't see much advantage. I18N.I18N would for OpenJUMP, but nothing prevent to create a static I18N instance in my.extension and to use it all over the extension.

Another point : to adapt measure extension, I had to rename the resource bundle as it was not names category/language/jump. Wonder why we would have to add language/jump automatically to the bundle name. Letting the user decide about the exact path and name of its bundle seems enough and more flexible to me. What do you think ?

it was a default i kept from the original implementation. felt it was a good default setting. it's described in the I18N.get() javadoc. simply use the File parameter to freely choose how you want to name your language files e.g.

Agree it is a good default setting. Just wonder if it has to be a mandatory setting (mainly because several extensions currently don't use this defaut)

i like it. it enables you to develop your extension in package 'my.cool.ext' and just deliver the same package as String and your files in language/jump* will automagically work. also it kind of enforces a default, which will make it easier for other devs finding a known file layout in your extension.

OK, as you like. Sometimes, extension gather several older plugins which have their own language files, but we'll always have the possibility to use the File parameter if we don't want to do too much refactoring.

I18N.getInstance(new File("my/cool/extension/messages")); will look for my/cool/extension/translations/messages.properties my/cool/extension/translations/messages_fr.properties. my/cool/extension/translations/messages_fr_FR.properties. ...

Oh, I thought that File was used for the case where bundle is external (not packaged into the jar). But I see that File is just processed as a String. Wonder why it has been added now. String parameter transformed by the addition of "language/jump" suffix and File parameter is interpreted as a String without adding language/jump. Sounds strange to me. Wouldn't it be enough to have String parameter without adding anything automatically ?

again i just adopted what was there and that is why the String version is called categoryPrefix and the File variant path. the getInstance methods can maybe have a better documentation on that or point to get() javadoc at least, which can be enhanced as well, ture.

And not to omit anything from my test, there are two errors in the javadoc.

namely? feel free to comment in the commit!.. ede

are those coming? It comes, thought that your IDE would warn you about this kind of mistake.

mukoki commented 3 years ago

And not to omit anything from my test, there are two errors in the javadoc.

namely? feel free to comment in the commit!.. ede are those coming?

It comes, thought that your IDE would warn you about this kind of mistake.

Finally, I'll fix it afterward. Don't want to clone your clone just for two lines of javadoc.

edeso commented 3 years ago

Yes, I mean for OpenJUMP itself, it could be static I18N I18N = I18N.getInstance();

for a static final constant capitalization would make sense. but are you actually suggesting to place it in I18N class like I18N.I18N ? looks a bit stupid. if you wanna go that way I18N.JUMP would probably be better.

Why stupid ?

ok, silly then :). just feel it's looking like an error written like that I18N.I18N also it is not obvious what it does. as we are dealing with I18N instances I18N.JUMP would be more obvious to implementers

Just make a static import and you just have to write I18N.get(key), which is simple and familiar for everyone who has already developped with OpenJUMP. I can't see the necessity to change the word to JUMP which says nothing about what it is and what it does.

was talking about the import above. further on in implementig code it'd look more elegant that way, agreed.

SNIP

Agree it is a good default setting. Just wonder if it has to be a mandatory setting (mainly because several extensions currently don't use this defaut)

i like it. it enables you to develop your extension in package 'my.cool.ext' and just deliver the same package as String and your files in language/jump* will automagically work. also it kind of enforces a default, which will make it easier for other devs finding a known file layout in your extension.

OK, as you like. Sometimes, extension gather several older plugins which have their own language files, but we'll always have the possibility to use the File parameter if we don't want to do too much refactoring.

renaming language files/folder or using File parameter seem both easy enough to me :)

SNIP

okay, as you are insisting - how about i rework this whole branch according your to I18N constant idea and we can finally pull it and fix further issues in main?

mukoki commented 3 years ago

Your implementation is OK. I read again what are the best practice for singleton, and it generally ends up with a private instance called "instance" which is lazilly initialized and a public getInstance() method as you did. I think that instance is usually made private to avoid accessing it before it has been fully initialized (lazy initialization). In our case, I think it is not important, but let's stick to the usual and simple form with just the public getInstance() and a private instance attribute as you did. If I want to use an instance called I18N (to be able to write I18N.get() rather than the longer I18N.getInstance().get() method, it is still possible to add it wher I need it : I18N I18N = I18N.getInstance()

For the convention for language files, I just did not know well how it has worked until now. I'm not sure the "language/file" convention is needed, but I've lived with it for 15 years, and if you find it good, go ahead.

Anyway, interesting discussion, sorry for the delay.

edeso commented 3 years ago

just because it's possible i renamed the instance to JUMP and made it publicly available. devs can hence choose to use

I18N.getInstance() or I18N.JUMP whatever tickles their fancy.

rebased the branch against the current trunk, feel free to pull or tell me to.

edeso commented 3 years ago

Anyway, interesting discussion, sorry for the delay.

btw. same here :)

also. let's commit https://github.com/openjump-gis/openjump/pull/36 right after this one, so we can easily integrate fixed up extensions later. also we might want to commit the https://github.com/openjump-gis/openjump/pull/29 FeatInst changes before that too.

mukoki commented 3 years ago

Merged this one. PR #29 has many conflicting file though. Not sure what is the best way to resolve.

edeso commented 3 years ago

will need to rebase https://github.com/openjump-gis/openjump/pull/29 and then we can go ahead of course.

btw. i still have more. JumpWorkbench.getInstance() cleanup in the pipeline, which is going to be pretty invasive. unfortunately no way around if we need to pass a context to everywhere it's needed. anyway, you will see what i mean when i push the branch.