Skip to content

Enable to display CJK characters for sdk v3.2#281

Closed
hangingman wants to merge 2 commits into
jMonkeyEngine:v3.2from
hangingman:issues/73_fix_display_cjk_characters_v3.2
Closed

Enable to display CJK characters for sdk v3.2#281
hangingman wants to merge 2 commits into
jMonkeyEngine:v3.2from
hangingman:issues/73_fix_display_cjk_characters_v3.2

Conversation

@hangingman
Copy link
Copy Markdown

@hangingman hangingman commented Aug 8, 2020

fix #73

Whats' this PR do ?

image

Problem

Solution

Main modification

Gradle

  • Add a new task copyEngineLibs that copy dependency jar files under ./engine directory into lib directory.

    • This task retrieve corelibs, optlibs, testdatalibs, examplelibs jar files name starting with jme3-, and renaming.
    • Exceptionally, com.github.nifty-gui:nifty:1.4.2 jar will be copied.
  • Remove dependencies.resolve() lines

    • This line requires gradle to get dependency jars from maven repository
    • If there is no dependency jar files in local lib directory, this line causes error
    • Current PR retrieve jar files with copyEngineLibs instead of dependencies.resolve()
// example
copyBaseLibs.inputs.files configurations.corelibs.resolve()
copyBaseLibs.outputs.dir "jme3-core-baselibs/release/modules/ext/"
copyBaseLibs.outputs.dir "jme3-core-libraries/release/modules/ext/"
  • misc
    • I compiled sdk with Java8
    • If project is compiled by Java11, the library javax.annotation.api will be required

Copy link
Copy Markdown
Member

@MeFisto94 MeFisto94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the main change is okay, but the gradle changes need to be reverted. I know it didn't compile because of the removal of jogl, but it changes a lot of things that we don't want and also makes it even more platform dependant. I've marked the two changes we could keep for the build.gradle, but they aren't really necessary.

* DejaVu doesn't support CJK glyphs
* @return true if user locale is "ja" or "ko" or "zh"
*/
private Boolean userLangIsCJK() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this for glyphs from thailand or india, I guess they would have similar problems?

Copy link
Copy Markdown
Author

@hangingman hangingman Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe so.
If so, I started to think it's better to find the list of what language is supported or not.
I will research what language is curretly supported by DejaVu fonts 2.34.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created a list with property file with using this langcover.txt.

// >zh-sg Chinese in Singapore 0% (2/6765) 0% (2/6765) 0% (2/6765)
// >zh-tw Chinese (traditional) (0/13063) (0/13063) (0/13063)
String lang = System.getProperty("user.language");
return lang.matches("ja|ko|zh");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is something like startsWith required for Chinese or maybe ^zh-.*$ or something?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. Because the language code and country code is different property.
For example my default java language code and country code setting are

-Duser.language=ja -Duser.country=jp

UI should follow user's property of user.language.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed my mind, there are some minor patterns (can't detect supported or not by only language code).
It looks good to detect with both language code and country code.

mn-cn  Mongolian in China                            (0/130)            (0/130)            (0/130)      
mn-mn  Mongolian in Mongolia                    100% (70/70)       100% (70/70)       100% (70/70)      

Comment thread jme3-dark-laf/src/org/jme3/netbeans/plaf/darkmonkey/DarkMonkeyLookAndFeel.java Outdated
Comment thread build.gradle
Comment thread build.gradle
@hangingman hangingman marked this pull request as ready for review August 12, 2020 14:55
@hangingman
Copy link
Copy Markdown
Author

@MeFisto94 Hi,
I updated this PR. Would you please review it again ?
If PR is OK, I will put together a commit.

Copy link
Copy Markdown
Member

@MeFisto94 MeFisto94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, looks like I forgot to submit my review :/
Regarding the files I am not sure what the best approach would be, though, e.g. instead of awk having a gradle task auto-generate the file so it does not need to be in the repo? or not have that because the source file langcover.txt would never change.
Talking of langcover, does it have any license issues we should be aware of?

Comment thread build.gradle Outdated
url "http://jcenter.bintray.com/"
}
jcenter()
mavenCentral()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there mavenCentral though?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed mavenCentral()

Comment thread copy_libs.sh Outdated
@@ -0,0 +1,18 @@
#!/bin/bash
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not sure about the existence of this, gradle's buildSdk task should take care of that in a better (version- and platform independent manner).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I update build.gradle to copy jar files from engine directory, and removed shell script.

Hiroyuki-Nagata added 2 commits August 24, 2020 23:21
* Add nifty-gui as dependencies
* Apply DejaVu font if user locale is not supported
* https://stackoverflow.com/a/18065154/2565527
* Add lang.properties to check DejaVu supporting tha language or not
* Copy dependency jme3 jar files with gradle
@hangingman
Copy link
Copy Markdown
Author

@MeFisto94 Hi,

Regarding the files I am not sure what the best approach would be, though, e.g. instead of awk having a gradle task auto-generate the file so it does not need to be in the repo? or not have that because the source file langcover.txt would never change.
Talking of langcover, does it have any license issues we should be aware of?

I think langcover.txt may not be changed in the future, so removed.

I am still not sure about the existence of this, gradle's buildSdk task should take care of that in a better (version- and platform independent manner).

I added copyEngineLibs task in the build.gradle. This task copy dependency jar files. And described detailed description on issue top.

Comment thread build.gradle
}
}

def copyEngineJarsToLib(file, extraJars=["nifty"]) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't gather why you try to copy the files from the engine folder, where the idea is to use gradles dependency resolution?

Also why do you depend on nifty-gui through jitpack while we get it transitively through

corelibs dep("org.jmonkeyengine:jme3-niftygui", false, false)
?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MeFisto94
Because, in my understanding, the buildSdk task need to have dependency jars but there are no process to download or copy these jar files. Probably since that reason, on my local environment after executing buildSdk will cause gradle error "Could not find org.jmonkeyengine:jme3-*".

I'm afraid current build script don't resolve dependency jars (jme3-.jar) correctly. build_engine.sh generates many jme3-.jar files, but it don't copy files into /lib directory or maven local repository.
So, I modified build.gradle script. Is there any misunderstanding for my side ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's definitely a misunderstanding, because CI works (https://travis-ci.org/github/jMonkeyEngine/sdk/jobs/724436054).
If that error was about a specific jme3-jogl, that's fixed in 56cdc94, essentially: Your engine folder needs to be on 3.3 (checkout or delete it and let the scripts work).

Due to

includeBuild './engine'
, engine dependencies should be there (unless jogl, which got deleted by the engine)

task copyBaseLibs(dependsOn:configurations.corelibs) {
Is the part which is copying them into a netbeans module, from which it is then available.

So all in all, it should work. Sorry for being not clear enough about this in the beginning, so you had that extra work

@hangingman
Copy link
Copy Markdown
Author

Close PR, and will reopen after fixed.

@hangingman hangingman closed this Nov 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants