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

sinst_src.zip を展開して登録する #133

Merged
merged 4 commits into from
Jun 27, 2018

Conversation

m-tmatma
Copy link
Member

sinst_src.zip を展開して登録する
macro.chm は生成できるので登録しない

@m-tmatma m-tmatma added the installer installer 関連 label Jun 17, 2018
@m-tmatma m-tmatma added the management 運営に関する話題 【ChangeLog除外】 label Jun 17, 2018
@m-tmatma
Copy link
Member Author

キーワードファイルが激しく文字化けしているが
utf8 化しても問題無いのだろうか?

ただこのファイルはユーザーもローカルで
編集できるのでその辺も考慮が必要

@KENCHjp
Copy link
Member

KENCHjp commented Jun 17, 2018

ただこのファイルはユーザーもローカルで編集できるのでその辺も考慮が必要

上書きしちゃうって意味であれば考慮不要かなと。
これまでもインストーラーでしかインストールしてないひとは上書きを逆に期待してる(内容がエンハンスされる)、または再インストールしたら戻るのを意識してる、または、exeだけを更新してるものとおもいます。
あえて書くならインストールのときに上書きしちゃう注意喚起ぐらいでいいような。

@KENCHjp
Copy link
Member

KENCHjp commented Jun 17, 2018

文字化けかぁ、検証して問題ばければばっさり変換”が”いいような。

@berryzplus
Copy link
Contributor

BOMがあったらutf-8、なければSJIS想定で読み込む仕様のようです。

// -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- //
// 強調キーワード //
// -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- //
// インポート
bool CImpExpKeyWord::Import( const wstring& sFileName, wstring& sErrMsg )
{
bool bAddError = false;
CTextInputStream in(to_tchar(sFileName.c_str()));

CTextInputStreamのコンストラクタで先頭3バイトがutf8のBOMと一致するか見てます。
とりあえず、実装的には一括変換で良さそうです。

@m-tmatma
Copy link
Member Author

http://d.hatena.ne.jp/naglfar/20051215/1134701360 によると
GetPrivateProfileInt は UTF-8 に対応しないみたいので
sakura.exe.ini は UTF-8 にはできない。

@m-tmatma
Copy link
Member Author

文字化けかぁ、検証して問題ばければばっさり変換”が”いいような。

検証方法をわかってないです。

@m-tmatma m-tmatma added this to the next release milestone Jun 18, 2018
@berryzplus
Copy link
Contributor

@m-tmatma さん

GetPrivateProfileInt は UTF-8 に対応しないみたいので
sakura.exe.ini は UTF-8 にはできない。

サクラ仕様の ini ファイルreader/writerなら utf-8 でイケます。
該当処理を CDataProfile に置換すれば変換しても大丈夫という認識です。

@berryzplus
Copy link
Contributor

文字化けかぁ、検証して問題ばければばっさり変換”が”いいような。

検証方法をわかってないです。

  1. unixシェルでバサッと変換かけて、
  2. 変換前後を diff なり WinMerge なりで比較(ここで変換の妥当性を担保)、
  3. 何件かピックアップでキーワードが読み込めるか確認する(ここで動作を担保)

設定→共通設定→強調キーワードでセット名を変えれば、読み込めてるキーワードを確認できます。
読込処理は共通なので、1ファイルでも確認できれば他も大丈夫と言えるです。
チェックはちゃんとやりたいですが、138ファイルは流石になえます・・・。

@m-tmatma
Copy link
Member Author

文字コード変換は別PR でやったほうが良さそうです。

berryzplus
berryzplus previously approved these changes Jun 18, 2018
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.

LGTMです。

元のzipを展開したフォルダと比較して、改行文字以外に差異がないこと、cpp.kwdを差し替えて強調キーワードが表示されることを確認しました。

改行に差異のあったファイル(コミットはCRLF)
AviSynth.kwd
AviSynth_Properties.kwd
AviSynth_Script.kwd
csharp.kwd
CSS2-GPL.txt
HSP.KWD
pukiwiki-readme.txt

@KageShiron
Copy link
Member

#141 を立てたのですが、古いファイルなども含まれておりmasterへのマージは中身を整理・更新してからでも良い気がします。一旦別ブランチで作業するのはどうでしょうか。

@m-tmatma
Copy link
Member Author

#141 を立てたのですが、古いファイルなども含まれておりmasterへのマージは中身を整理・更新してからでも良い気がします。一旦別ブランチで作業するのはどうでしょうか。

はい、いいです。

それならこの PRのmerge をブロックしておいてください。

既存の sinst_src.zip をもとにインストーラを作る対応をしておこうと思います。
installer は #141 が終わるまで artifacts に含めないほうがいいかもしれません。

@m-tmatma
Copy link
Member Author

installer は #141 が終わるまで artifacts に含めないほうがいいかもしれません。

既存のインストーラは keyword ファイルを含んでリリースしているので
問題ないと判断して、artifacts に含めておきます。

@kobake
Copy link
Member

kobake commented Jun 26, 2018

現時点でのブランチ運用方針としては一旦 master のみでやっていくことになりました。

@KageShiron

#141 を立てたのですが、古いファイルなども含まれておりmasterへのマージは中身を整理・更新してからでも良い気がします。一旦別ブランチで作業するのはどうでしょうか。

理想としてはそうなのですが、作業の足場として一旦 zip 解凍しただけのものを master に入れてしまい、それをもとに変更対応を入れていくのが効率的にも作業分担都合的にもやりやすいと思っていますが、いかがでしょうか。

もともと「整備されていないZIPがリポジトリに入っていた」時点で不整合はあったわけですから、それを展開したものを master に入れることが事態の悪化になるとは思ってないです。

この方針で問題ないようであればこの PR はマージしてしましたいです。自分的にもこの PR は LGTM です。

@m-tmatma
Copy link
Member Author

この PRを作ったときと状況が変わっているし
インストーラの x64 ダイアログと関連するので
このPR を取り込むには修正が必要です

@m-tmatma
Copy link
Member Author

一部間違い。x64 対応には関係ないです

@KENCHjp
Copy link
Member

KENCHjp commented Jun 26, 2018

旦 zip 解凍しただけのものを master に入れてしまい

わたしもこれがいいかなとおもいます。
zip圧縮=タイムスタンプ保持かとおもいますが、取り込んだ時点で寄贈物=改変してくものばかりなのではないかと。
そうではないものはいったん外して改めてとりこみなおすぐらいでもいいような。

@m-tmatma
Copy link
Member Author

08f813e: master をマージ
866a121: バッチファイルの変更

@m-tmatma
Copy link
Member Author

このPR を取り込むには修正が必要です

修正しました。

@kobake
Copy link
Member

kobake commented Jun 27, 2018

動作確認結果です。インストーラによるインストールで正しく keyword フォルダが含まれた構造が構築されることを確認できました。

installed

key

@kobake
Copy link
Member

kobake commented Jun 27, 2018

@berryzplus さんのほうで keyword 差異が無いことは確認していただいているので

元のzipを展開したフォルダと比較して、改行文字以外に差異がないこと、cpp.kwdを差し替えて強調キーワードが表示されることを確認しました。

 

自分のほうでは .bat 内容のみコードレビューしました。問題なさそうです。

Copy link
Member

@kobake kobake left a comment

Choose a reason for hiding this comment

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

LGTM

@kobake kobake merged commit d712a54 into sakura-editor:master Jun 27, 2018
@KENCHjp
Copy link
Member

KENCHjp commented Jun 27, 2018

マージされてから気が付く、

sakura/installer/sinst_src/

こうなんですね。

sakura/installer/

ここに展開されると思ってた(笑)

@kobake
Copy link
Member

kobake commented Jun 27, 2018

作業の記録としては
sakura/installer/sinst_src.zip
sakura/installer/sinst_src に展開されたってことで、記録としては何をやっているのか自明で良いんじゃないですかね~

フォルダ構成の調整については別件で考えましょう。
sakura/installer/ 直下に展開されることが必ずしも正解でない可能性はあるので、
この PR 内でそこの議論に時間をかけるよりは
一旦シンプルに作業土台を作ってしまってから別件で検討を進めるのが建設的に思いました。

@KENCHjp
Copy link
Member

KENCHjp commented Jun 27, 2018

はい、思ってたのと違っただけで、特にこだわりありません。

@m-tmatma m-tmatma deleted the feature/extract-sinst_src branch June 30, 2018 13:21
@ds14050 ds14050 added installer installer 関連 management 運営に関する話題 【ChangeLog除外】 labels Sep 18, 2018
@KENCHjp KENCHjp added the CI appveyor など CI 関連 【ChangeLog除外】 label Dec 5, 2018
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
…nst_src

sinst_src.zip を展開して登録する
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI appveyor など CI 関連 【ChangeLog除外】 installer installer 関連 management 運営に関する話題 【ChangeLog除外】
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants