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

Installer file update (ChineseTranslation) #1469

Merged
merged 38 commits into from
Jan 31, 2021

Conversation

EldersJavas
Copy link
Contributor

@EldersJavas EldersJavas commented Nov 22, 2020

PR の目的

カテゴリ

  • 機能追加

Simplified Chinese and Traditional Chinese installer file translate

  • ドキュメント修正

sakura-common.iss and more

PR の背景

Sakura Chinese Translation Project

PR のデメリット (トレードオフとかあれば)

可能无法在非GBK系统上正常编译。可能会出现乱码情况。(sloved)

必须在 Inno Setup 6\Languages 文件夹中添加语言文件。(sloved)

语言文件下载地址: 简体中文 繁体中文

旧版本可以在这里下载:https://jrsoftware.org/files/istrans/

It may not compile properly on non GBK systems.(sloved)
There may be garbled code.(sloved)
You must add a language file to the /inno setup 6/languages folder.(sloved)

Language file download: simplified Chinese traditional Chinese

Old versions can be downloaded here: https://jrsoftware.org/files/istrans/

テスト内容

可以在GBK编码的系统上正常编译。使用Inno Setup 6.

32位系统和64位系统均可以正常编译。

It can be compiled normally on GBK coded system. Use inno setup 6.

Both 32-bit system and 64 bit system can compile normally.

関連 issue, PR

#1369

@EldersJavas EldersJavas mentioned this pull request Nov 22, 2020
12 tasks
@AppVeyorBot
Copy link

Build sakura 1.0.3263 failed (commit 1764f8cca4 by @EldersJavas)

@AppVeyorBot
Copy link

Build sakura 1.0.3264 failed (commit 1aacc095a5 by @EldersJavas)

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

Do not remove UTF-8 byte order mark, it is required by inno setup.

@EldersJavas
Copy link
Contributor Author

Have we encountered any difficulties?

I have been compiling files in Inno Setup. I think the BOM header was removed when I copied it to GitHub and was forced to convert to UTF-8 encoding by GitHub.

@AppVeyorBot
Copy link

Build sakura 1.0.3278 failed (commit ff4a70ae15 by @EldersJavas)

@berryzplus
Copy link
Contributor

Have we encountered any difficulties?

no, thisi pull request has very simple issue.
you have remove Byte Order Mark at 81b0f4a .
i have marked reqest changes, because i think you should fix it.

I have been compiling files in Inno Setup. I think the BOM header was removed when I copied it to GitHub and was forced to convert to UTF-8 encoding by GitHub.

no. GitHub can treat UTF-8 with BOM.
How did you convert encoding?
iconv does not traat BOM.
you can use vscode, open the file, and saveas encoding UTF-8 with BOM.

Copy link
Contributor Author

@EldersJavas EldersJavas left a comment

Choose a reason for hiding this comment

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

check

@EldersJavas
Copy link
Contributor Author

OK,Thanks.
I updated the new file with BOM.Please check.

@AppVeyorBot
Copy link

Build sakura 1.0.3280 failed (commit 306ce9db3c by @EldersJavas)

@AppVeyorBot
Copy link

Build sakura 1.0.3281 failed (commit 3dd3c853bd by @EldersJavas)

Copy link
Contributor

@beru beru left a comment

Choose a reason for hiding this comment

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

I've noticed that AzurePipelines build failed.

According to the log, ChineseSimplified.isl file is missing from Inno Setup directory.

https://dev.azure.com/sakuraeditor/8a4927d0-fbf2-4d46-9d36-17af9073ab33/_apis/build/builds/2432/logs/161

2020-12-06T03:16:36.2171645Z running "C:\ProgramData\Chocolatey\bin\ISCC.exe" installer\sakura-Win32.iss
2020-12-06T03:16:36.5770672Z Error on line 51 in D:\a\1\s\installer\sakura-common.iss: Couldn't open include file "c:\program files (x86)\inno setup 6\Languages\ChineseSimplified.isl": The system cannot find the file specified.
2020-12-06T03:16:36.5772540Z Compile aborted.

It looks like Chinese language files aren't included with Inno Setup's default installer as they are not still official translations.
https://jrsoftware.org/files/istrans/

I'm not really sure how to address this problem.
Do you have any ideas?

@berryzplus berryzplus dismissed their stale review December 6, 2020 07:10

BOM has been restored.

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

see #1469 (review)

CI builds failed to make an installer.
we need to have a successful build with at least one CI.

supported CI is appveyor, azule pipelines(DevOps) and GitHubActions.

@kengoide
Copy link
Member

kengoide commented Dec 6, 2020

I was under the impression of that the PR is in need of an update on our side to install Chinese locales in the CI environment.
It is on OP what files needs to be in the environment:

You must add a language file to the /inno setup 6/languages folder.
Language file download: simplified Chinese traditional Chinese
Old versions can be downloaded here: https://jrsoftware.org/files/istrans/

I actually have no idea who is in charge of maintaining our CI infrastructure. Can anyone chime in?

@berryzplus
Copy link
Contributor

翻訳するのがややめんどうな話題なので日本語で書きます。
Google翻訳とか使って読んでください。

解決すべき課題

インストーラー生成スクリプトに中国語を含めるとビルドが失敗する。
中国語版だけでなく、インストーラー自体が生成されない。

インストーラーが生成されない原因

inno setupが現状で中国語をサポートしていない。

課題に対するおいらの見解

課題解決の目途が立つまではマージを保留せざるを得ないと思います。

この課題を誰が解決すべきか?

中国語版インストーラーを実現したい人がやるべきだと思います。

自分が中国語版インストーラーを実現したい立場なら、inno setupの開発チームに働きかけると思います。
開発チームに働きかけなくても暫定版の "c:\program files (x86)\inno setup 6\Languages\ChineseSimplified.isl" を手に入れる方法はあると思うので、問題がislファイルの欠落だけであるなら暫定版の作成は容易だと思います。そのあたりの問題解決について、協力はしますけど主体的に解決策を探っていく意思はないです。

@beru
Copy link
Contributor

beru commented Dec 8, 2020

jrsoftware/issrc#346 is related.
It seems JRSoftware has no plan incorporating Chinese translation files.

@@ -48,70 +48,164 @@ MinVersion=6.1
[Languages]
Name: "ja"; MessagesFile: "compiler:Languages\Japanese.isl"
Name: "en"; MessagesFile: "compiler:Default.isl"; InfoBeforeFile: "instmaterials\info_us.txt"
Name: "zh_hans"; MessagesFile: "compiler:Languages\ChineseSimplified.isl"; InfoBeforeFile: "instmaterials\info_zh_hans.txt"
Name: "zh_hant"; MessagesFile: "compiler:Languages\ChineseTraditional.isl"; InfoBeforeFile: "instmaterials\info_zh_hant.txt"
Copy link
Contributor

Choose a reason for hiding this comment

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

@EldersJavas

Please change these lines as specified below.

Name: "zh_hans"; MessagesFile: "Languages\ChineseSimplified.isl"; InfoBeforeFile: "instmaterials\info_zh_hans.txt"
Name: "zh_hant"; MessagesFile: "Languages\ChineseTraditional.isl"; InfoBeforeFile: "instmaterials\info_zh_hant.txt"

You need to add ChineseSimplified.isl file and ChineseTraditional.isl file into your git repository's installer/Languages folder because they don't exist inside C:\Program Files (x86)\Inno Setup 6\Languages folder (as a workaround).

Additionally, you also need to add info_zh_hans.txt file and info_zh_hant.txt file into your git repository. I think you simply forgot to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for not replying to you for so long.
I have uploaded the file, please check.

Copy link
Contributor

@beru beru Jan 4, 2021

Choose a reason for hiding this comment

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

@EldersJavas Thank you for uploading the files.

However, you would still need to rectify installer/sakura-common.iss file. Please read the comment carefully. I intentionally removed compiler: from MessageFile: value. If there's a compiler: keyword, the installer would search the specified file from InnoSetup installed directory, and otherwise the file inside the Source Directory will be archived in the installer and will be used. I guess I should have explained this behaviour as it is not very straight forward to understand.

Here's an excerpt from Inno Setup Help.

MessagesFile (Required)

Specifies the name(s) of the .isl file(s) to read the default messages from. The file(s) must be located in your installation's source directory when running the compiler, unless a fully qualified pathname is specified or the pathname is prefixed by "compiler:", in which case it looks for the file in the compiler directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/sakura-editor/sakura/pull/1469/files#r538538926 suggested below.

Suggested change
Name: "zh_hant"; MessagesFile: "compiler:Languages\ChineseTraditional.isl"; InfoBeforeFile: "instmaterials\info_zh_hant.txt"
Name: "zh_hant"; MessagesFile: "Languages\ChineseTraditional.isl"; InfoBeforeFile: "instmaterials\info_zh_hant.txt"

this should also be applied to non-traditional version.

@kengoide
Copy link
Member

kengoide commented Dec 12, 2020

I've kind of got this going on AppVeyor by following the approach @beru suggested.
https://ci.appveyor.com/project/kagari/sakura/builds/36799426

One problem in this approach: Apparently our two CIs employ different versions of Inno Setup. AppVeyor is on version 5 (we are using Visual Studio 2017 image), and Azure Pipelines is on 6. It seems between them their unofficial Chinese translation files changed encoding from MBCS to UTF-8, and I specifically added Inno Setup 5 version of translation files into the repository, it is likely to cause breaks on Azure Pipelines builds.

Possible solutions I came up with:

  • Prepare some tricks of switching files between 5 and 6 according to which one of them is on.
  • Upgrade AppVeyor image to "Visual Studio 2019".

AppVeyor のビルドイメージを Visual Studio 2019 に変更するのがこのPRについて言えば真っ当な対応策だと思いますが、どういう課題があるか伺いたいです。
(そもそも英語で書いたことが正しいかどうかも自信がありませんけども…)

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@EldersJavas
Copy link
Contributor Author

All the problems have been solved, except for the old files.

I'm very sorry, my school network is down and I cannot access GitHub. It has been resolved today.

@AppVeyorBot
Copy link

@@ -0,0 +1,3 @@
该软件被定义为“文本编辑器(共同开发版本)”。
您可以免费用于商业和非商业用途,但源代码的版权由各部分所有者保留。
无法保证该软件在分发时的完整性。
Copy link
Contributor

Choose a reason for hiding this comment

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

@EldersJavas Can you please check if this file is valid or not.
These lines are used in "Information" wizard page.

If I select Traditional Chinese on the installer, I see garbled characters.

image

On the other hand, If I select Simplified Chinese on the installer, I can see human-readable texts.

image

Copy link
Contributor Author

@EldersJavas EldersJavas Jan 29, 2021

Choose a reason for hiding this comment

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

This is indeed a problem.
When compiling locally, if I use UTF-8, select Simplified Chinese on the installer, it is still garbled.
So, I had to change its encoding to GBK.This is no way.让我再尝试下别的编码.
Moreover, it is very funny that Simplified Chinese is garbled on GitHub, but it can be displayed normally.Traditional Chinese is not garbled on GitHub, but it is garbled in it . Joke:)

OK,solved.

Copy link
Contributor

Choose a reason for hiding this comment

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

@EldersJavas Thank you for the fix. I've confirmed that the garbled Traditional Chinese texts are fixed now.

image

@sonarcloud
Copy link

sonarcloud bot commented Jan 29, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@AppVeyorBot
Copy link

@berryzplus
Copy link
Contributor

このPRについて可否判断の基準は以下だと思います。

  1. CIでビルドが通ること。
  2. 既存言語(日本語、英語)に影響を与えないこと。
    👆 @k-kagari さんの指摘がこの部分に触れたものがコレ?

インストーラの翻訳が合っているかどうかとかサクラエディタ本体が対応しているかとかは別の話だと思っています。
たぶんもう、あと少しです。

@kengoide
Copy link
Member

you are right.If use inno6, we really don't need these old files.
But if delete, it may make the whole Sakura project unable to compile in inno5.It may be necessary to modify the min compilation environment.

一人で判断することではないでしょうから皆さんに伺いますが、Inno Setup 5 と 6 の両対応は残さなければいけないものでしょうか?

今のところ、このPRがマージされれば AppVeyor と Azure Pipelines の両方で Inno Setup 6 を使うようになります。Inno Setup 5 互換のために CI で使わない古い翻訳ファイルを別に入れている状態です。6だけに対応することにできれば古いファイルを入れる必要はなくなります。

@beru
Copy link
Contributor

beru commented Jan 30, 2021

一人で判断することではないでしょうから皆さんに伺いますが、Inno Setup 5 と 6 の両対応は残さなければいけないものでしょうか?

個人的には Inno Setup 6 の対応だけで良いと思います。

https://github.com/sakura-editor/sakura/blob/fae4e2f84e54af517f29ed0f10811e889cf974df/build.md#%E3%82%A4%E3%83%B3%E3%82%B9%E3%83%88%E3%83%BC%E3%83%A9%E3%81%AE%E3%83%93%E3%83%AB%E3%83%89%E3%81%AB%E5%BF%85%E8%A6%81%E3%81%AA%E3%82%82%E3%81%AE

の説明の記載を更新する必要がありますね。

@EldersJavas
Copy link
Contributor Author

EldersJavas commented Jan 30, 2021

一人で判断することではないでしょうから皆さんに伺いますが、Inno Setup 5 と 6 の両対応は残さなければいけないものでしょうか?
@k-kagari

我的看法是:

  1. 我们使用inno5的目的是为了兼容WindowsXP。但是现在,Sakura项目已经不能运行在WindowsXP上了。因此继续使用inno5没有意义。 Inno Setup での OS の対応バージョンを Windows 7 以降に変更 #895
  2. inno是一个开源项目,无论使用哪个版本都是免费的。
  3. 如果继续使用inno5,需要耗费更多精力去维护。

缺点:我们需要更新一些Markdown :) 当然,我很乐意来更新.

所以,我认为舍弃inno5是明智的选择.


My opinion is:

  1. Our purpose of using inno5 is to be compatible with Windows XP. But now, the Sakura project(latest) can not run on Windows XP. So continuing to use inno5 does not make sense. Inno Setup での OS の対応バージョンを Windows 7 以降に変更 #895
  2. inno is an open source project, no matter which version is used, it is free.
  3. If we continue to use inno5, we need to spend more energy to maintain.

Cons: We need to update some Markdown files :) I'm very glad to update files.

Therefore, I think abandoning inno5 is a wise choice.

Copy link
Contributor

@beru beru left a comment

Choose a reason for hiding this comment

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

ビルドが通っているしもう merge して良いと思います。

下記の Inno Setup 5 向けのファイルの削除とか build.md ファイルの更新とかは別のPRで対応でも問題ないと思います。

  • installer/Languages/ChineseSimplified(old).isl
  • installer/Languages/ChineseTraditional(old).isl

@EldersJavas
Copy link
Contributor Author

いつもありがとうございます!今後ともよろしくお願いいたします。
AND...what's next?

@beru beru merged commit 455643a into sakura-editor:master Jan 31, 2021
@ghost
Copy link

ghost commented Jan 13, 2022

appveyor.ymlの履歴を確認して気が付いたんですが、このPRで変更される前のコミットが #1403 に含まれるコミットになっています。
https://github.com/sakura-editor/sakura/commits/b8bb61e8c1a2e26131b85b14affec3eca1fd434a/appveyor.yml

このPRの差分("Files changed"タブで見れるもの)は「2行追加」と表示されていますが、 #1403 とこのPRのマージコミット間で取った差分は「5行追加・31行削除」となり一致しません。
diff-appveyor_yml-9e5a78f-455643a.patch.txt

幸い差分からBuildChmジョブの廃止を行っていることがわかり、履歴から消えたのは #1509 だと特定することができました。
#1509 はちょうど「3行追加・31行削除」なので、このPRの変更と合わせれば上述の差分と合います。)

一応ご報告しておきます。

@berryzplus
Copy link
Contributor

自動マージにより欠落したコミットがありそうだ、と言ってますかね?
そんなことが、起きるんですね・・・(ただの感想。

@ghost
Copy link

ghost commented Jan 21, 2022

自動マージにより欠落したコミットがありそうだ、と言ってますかね?

たぶん……初めて見たので理由はわからないです。 5cbc57cappveyor.ymlの履歴に入っていません。

@berryzplus
Copy link
Contributor

berryzplus commented Jan 22, 2022

このPRのコミットの積み方がグチャグチャなのが原因っぽく見えます。
何を修正したかを把握するためにローカルでrebaseしてみたら、
ほぼすべてのコミットでconflictが発生したのでマージが誤動作しても仕方ないように感じました。
(じゃあなんで自動マージできたのか謎ですけど。)

「コミットの積み方が酷いので積み直してください」って指摘は、
このプロジェクトが始まった当時によく指摘されてたんですけど、
指摘を受けた本人はかなりイラッとします。
できるだけそういう指摘はしないようにしてますが、必要なんですかねぇ。

消えてる可能性のあるマージ済みPRの全量はこんだけのはずです。
https://github.com/sakura-editor/sakura/pulls?q=is%3Apr+is%3Amerged+merged%3A2021-01-19..2021-01-31

@berryzplus
Copy link
Contributor

確認してみましたが、マージコミットは全部入ってそうです。
変更内容のマージがうまくいってないだけみたい。

@ghost
Copy link

ghost commented Jan 23, 2022

EldersJavas/sakura:master のコミットログを確認しましたが、僕には派生元が c70ce54#1374 のマージコミット)であるように見えました。
このPRに含まれるコミットは38個と表示されていますが、実際は sakura-editor/sakura:master からマージしたコミットが含まれて450個になるのではないかと思います。
この450個のコミットの中に appveyor.yml ファイルを変更する 5cbc57c (#1509) が含まれているため、このPR独自のコミットと履歴が統合され消えてしまったのではないかと考えています。

@berryzplus
Copy link
Contributor

確認すべき対象コミットが3日とか2週間じゃなくて、約2か月分だってことですよね。

やってしまったものは仕方ないので、
 どうやって復旧しましょうか?
になると思っています。

消えたコミットを復活させようとすると難易度高いような・・・(笑

@berryzplus
Copy link
Contributor

結局、復元すべき差分はなさそうです。

今後は、既存ファイルをDeleteしてAddするPRブランチを見かけたら、
ちょっとめんどうでも注意するようにしたいと思います。
(「削除新規すると履歴が壊れるので、出し直してください。」→PRクローズ)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants