Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

next: The custom theme allow to use external image, set font & color by config #378

Merged
merged 5 commits into from
Oct 18, 2018

Conversation

zsalch
Copy link
Contributor

@zsalch zsalch commented Oct 3, 2018

Base on #365

Add a new way to customize the theme by external images, setup font & color of the comment.

The configuration method is as following:
1) Change the 'theme' value to specify your theme name:
"theme": "",
for example:
"theme": "mylike",
3) Create subfolders in the lizzie folder
/theme/
for example:
/theme/mylike
4) The custom-theme folder must include the config file 'theme.txt', the format as followings(use default when option not be specified):
{
"background-image" : "background.png",
"board-image" : "board.png",
"black-stone-image" : "black.png",
"white-stone-image" : "white.png",
"font-name": "Wawati SC",
"solid-stone-indicator": false,
"comment-background-color": [0, 0, 0, 200],
"comment-font-color": [255, 255, 255, 255],
"winrate-line-color": [0, 255, 0, 255],
"winrate-miss-line-color": [0, 0, 178, 255],
"blunder-bar-color": [255, 0, 0, 150]
}
e) Will use default theme when not found related config item.
f) Run the Lizzie.

OlivierBlanvillain added a commit to OlivierBlanvillain/lizzie that referenced this pull request Oct 3, 2018
OlivierBlanvillain added a commit to OlivierBlanvillain/lizzie that referenced this pull request Oct 3, 2018
@ParmuzinAlexander
Copy link
Contributor

It is hard make random stone "white-stone-image" : "1.png,10.png" ?

@zsalch
Copy link
Contributor Author

zsalch commented Oct 5, 2018

@ParmuzinAlexander
The random stone is not particularly difficult to implement, but I want to know more about the scenario, which is to randomly select a stone at each move, or choose one at the beginning of the game.
If each move is randomly chosen, it needs to cache more types of stones, performance may have a little impact and require more logic.

@ParmuzinAlexander
Copy link
Contributor

@zsalch
Each move. Or maybe not random but in sequence 1.png,2.png,....10.png,1.png

@OlivierBlanvillain
Copy link
Contributor

@zsalch Can you rebase this one now that #365 is merged?

@zsalch
Copy link
Contributor Author

zsalch commented Oct 5, 2018

@OlivierBlanvillain
No problem. But now ITheme&DefaultTheme has been removed, does this mean that CustomTheme needs to be adjusted accordingly, after all, it is based on ITheme & DefaultTheme.

@OlivierBlanvillain
Copy link
Contributor

Yes. My suggestion would be to always have custom theme on. Basically add a few options to the config JSON to change colors and the paths to the various images we use.

@zsalch
Copy link
Contributor Author

zsalch commented Oct 5, 2018

Ok, I'll remove some redundant paths and use 'theme' instead of 'theme'+'custom-theme' in the config.txt file.

@zsalch
Copy link
Contributor Author

zsalch commented Oct 5, 2018

@OlivierBlanvillain
I have updated the code and commit again.

private String path = null;
private JSONObject config = new JSONObject();

public Theme(String themeName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove themeName altogether, from this class and from the JSON config, it's never used for anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

FileInputStream fp;
try {
fp = new FileInputStream(file);
config = new JSONObject(new JSONTokener(fp));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think having a theme.txt adds a lot of complexity in term for configuration for users. I would simply keep everything in the main config file. To have multiple themed Lizzie I would simply recommend having multiple config files with variations on the theme related configs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As conversation, keep two ways.

fp = new FileInputStream(file);
config = new JSONObject(new JSONTokener(fp));
fp.close();
} catch (FileNotFoundException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

FileNotFoundException extends IOException, so you can remove that line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@@ -137,6 +141,13 @@ public LizzieFrame() {
JSONArray windowSize = Lizzie.config.uiConfig.getJSONArray("window-size");
setSize(windowSize.getInt(0), windowSize.getInt(1)); // use config file window size

// Allow change font in the config
if (theme.fontName() != null) {
systemDefaultFontName = theme.fontName();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these name are obsolete if you start changing them. Instead I would use

  • fontName
  • regularFont
  • boldFont

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rename to fontName, uiFontName, winrateFontName.

if (theme.fontName() != null) {
systemDefaultFontName = theme.fontName();
OpenSansRegularBase = new Font(systemDefaultFontName, Font.PLAIN, 12);
OpenSansSemiboldBase = new Font(systemDefaultFontName, Font.BOLD, 12);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the front size also be configurable? I it often the case than it needs adjustment depending on the font...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Different areas have different font sizes. If need to configure them, you may need to add several different configurations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@featurecat @OlivierBlanvillain

Currently the font usage is now as follows:

OpenSansSemiboldBase: winrate text on the stone
OpenSansRegularBase: some UI text
systemDefaultFontName: some prompt text

And now the font size is basically responsive with the window, if you want to use the configuration file to change, I think we need to take some time to design the font usage.

So now only three font configurations are defined. The font size option only supports comment.

OpenSansSemiboldBase: winrate-font-name
OpenSansRegularBase: ui-font-name
systemDefaultFontName: font-name

blackStoneCached =
ImageIO.read(
new File(
this.path
Copy link
Contributor

Choose a reason for hiding this comment

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

String p = this.path + /* File. */separator + config.optString("black-stone-image", "black.png");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

whiteStoneCached =
ImageIO.read(
new File(
this.path
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, just to make it fit in the 100 char limit...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


/** Use custom font */
public String fontName() {
return config.optString("font-name", null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Default to null? Why not return the actual default font name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change fontName logic.

public Color commentBackgroundColor() {
JSONArray color = config.optJSONArray("comment-background-color");
if (color != null) {
return new Color(color.getInt(0), color.getInt(1), color.getInt(2), color.getInt(3));
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should give a nice error message in case somebody messes up the config file and color.length != 4. Same comment for the methods below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use new logic to process color.

@zsalch
Copy link
Contributor Author

zsalch commented Oct 12, 2018

@OlivierBlanvillain
I still prefer to use the independent theme settings, so if you need to share a theme, just copy the corresponding directory file, and simply specify the theme name in the config.txt file.

However, I also agree that you can set theme-related options directly in the config.txt file, so that users can modify them without enabling the theme.

@zsalch
Copy link
Contributor Author

zsalch commented Oct 13, 2018

@featurecat @OlivierBlanvillain
We've added some new configuration options, and we need to determine which ones are enabled by default.
Such as: show-blunder-bar, show-comment etc.

@OlivierBlanvillain
Copy link
Contributor

@zsalch Alright, if you think the split in config.txt/theme.txt makes more sense, I'm OK with it.

I think we should turn both of these on by default for better discoverability.

Copy link
Contributor

@OlivierBlanvillain OlivierBlanvillain left a comment

Choose a reason for hiding this comment

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

LGTM!

@OlivierBlanvillain OlivierBlanvillain merged commit 3eb8ec7 into featurecat:next Oct 18, 2018
kaorahi pushed a commit to kaorahi/lizzie that referenced this pull request Nov 12, 2018
featurecat added a commit that referenced this pull request Nov 12, 2018
fix: #290 (modify alpha) was broken after #378 (custom theme)
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.

3 participants