-
Notifications
You must be signed in to change notification settings - Fork 163
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
設定データ読み込み処理において言語設定切り替え後にMRUエントリが無い場合は新規インストール後とみなし false を返すように変更 #620
設定データ読み込み処理において言語設定切り替え後にMRUエントリが無い場合は新規インストール後とみなし false を返すように変更 #620
Conversation
管理プロセスから呼び出ししている設定データ読み込み処理(CShareData_IO::LoadShareData)の戻り値を使うように変更 後続する設定データ保存処理(CShareData_IO::SaveShareData)の呼び出しは設定ファイルが存在しないという判断ではなくて、設定データ読み込みに処理に失敗したらという判断に変更
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTMです。
とくに問題ないと思います。
MRU(最近使ったファイル)=0件をインストール直後と看做す、で文句ないように思います。
懸念は、インストーラから初期設定を触るアプローチが今後とりにくくなることかな?
思い当たる悪い影響・・・初回起動が遅くなる。
設定壊れるより全然いいとのでOKと思ってます。
いわゆる「デフォルト設定」を作っているのは、CControlProcess.cppの82行目の呼出です。 なんかの条件で「新規」と判断できたら設定を読むのをやめる、というこの変更は適切だと思っとります。 |
厳密にはMRUが0件という判定ではないです。MRUのエントリが存在するかどうかの判定にしています。
インストーラ側で初期設定を触る場合は設定データ読み込み処理周りもチェックしないといけないですね。大変だ。。
初回起動時に |
なんか対策した方がいいですかねぇ。
仕組み自体はこれでいいと思ってます。 終了時、何も問題なければ管理プロセスが落ちるときに設定ファイルが保存されます。 このPRについて、とくに問題はないと思ってますが、他の人の反応をみたい感じなので可能なら少しマージを遅らせてほしいです。ま、ぼくならマージされてても気になったことはコメントしますけどw |
インストーラーが配置した初期設定ファイルかどうかを厳密に確認するなら、特定のセクションとエントリ(言語DLLの)しか存在しないかを判定する事になると思いますがその記述がすんなり書けそうになかったので MRU のエントリの非存在で判定しています。 |
設定ファイルの保存に関してですが、MRUとか頻繁に変更されるものは別のファイルに分けるみたいな事を @moca_skr さんが紅桜でやっていたような気がします。使った事無いですが…。まぁでも設定ファイルを分けたり設定ファイルのフォーマットをINIから変えるのが正解かというと、変更によるデメリットもある筈なので今更あえて変更するのも良くないかもしれないですね。 設定を全然変更していない場合でも終了時に設定を保存しているようなので、それはしないようにした方が良いと思います。 |
紅桜はMocaさんじゃなかったような・・・ MRUをiniから切り離す試みは、変更量そんなでもないはずです。 ini初期化用のエントリを用意して、インストーラで書いてもらうのが無難なのかな・・・ 「設定リセット」ってボタンを共通設定のどっかに用意して、それを押すとサクラエディタが再起動してまっさらな初期設定に戻るって新機能を妄想。 |
あ、ほんとですね。適当に記憶してました。
過去のMRU記録の引き継ぎも出来るようにしたりとか、従来の記録方法も選択できるようにしたりとか考えるとややこしくなりそうですね。。一方通行で良いのかもしれませんが。 win10のMRU機能というのは、Windows7から追加された Jump List の事を指していると考えて良いですか?
そっちの方が良いかも知れないですね。MRUのエントリの非存在で設定リセットというのが後になって牙を剥かないか心配なので。。
Visual Studio だとメニューのツールの設定のインポートとエクスポート(Import and Export Settings Wizard)という画面で設定の読み書きやリセットが行えますね。サクラエディタにインポートとエクスポートを付けないままリセット機能だけを追加すると、「間違ってリセット押しちゃったけどバックアップ取ってなかったよ、どうしてくれるんだ」と怖い人達に詰め寄られる可能性が有るとかないとか。 |
appveyor で rebuild かけたけどだめですね。 |
ダメですね。 エラー原因は hhc.exe のエラーだけで、他のビルドは通っているからPR自体に問題はないと思っています。「×」マークが気になるっちゃ気になりますが。 |
そうです。 サクラエディタはジャンプリストには何故か対応しています。 設定のインポート・エクスポート機能は既にあると思います。 |
#622 で chm のビルドに失敗したときにリトライするように実装してみました。 |
#622 をマージ後にこの PR のリビルドをかけました。 |
https://ci.appveyor.com/project/sakuraeditor/sakura/builds/20382916/job/she82xg2avnnq6el 以下で失敗している。
|
何が起きているか分かりませんな・・・w |
ローカルでは再現しないの? |
通ったみたい |
これはこのままマージでいいように思っていますがどうでしょうか。 設定読み込みをやめて続行するかどうかの判断条件を改善する話が出ていて、ほぼほぼ内容決まってる感じですが、それは別件でPRいれたほうがよい気がしています。 タイミングは任せます > @beru さん |
それではいったんこれで merge してしまいます。 インストーラー側で
|
@berryzplus さん
下記の設定は個別にインポート・エクスポート操作が出来るようですが、 sakura.ini の設定全体のインポート・エクスポート機能は見当たりませんでした。そんな機能が要るのかと言われたら、
|
りょ。
設定リセットエントリは ある or なし の二値で考えています。 具体的なことは今考えてる感じですけど、こんなん。 ; sakura.ini テキストエディタ設定ファイル
[Common]
szLanguageDll=
bResetAllSettings=1 bool型なので 一般の設定項目と同列に扱うと面倒なことが起きそうなので、 更新インストールについては、なんも考えとらんです。 新機能追加時の設定追随をどう考えるかでissueがあった気がします。 windowsの通知機構を活用したいなぁ、と思ってますけど、そこよりもエディタ機能の強化の方を前に進めたいです 😭 |
うーむ。確かに。 |
追加の実装箇所として、「exe 側に インストーラーについてInnoSetup の しかし (今も既に |
設定リセット用エントリを通常の設定項目と同じ入出力フローに含めない設計がよいと思っています。サクラエディタのiniファイルは、毎回全設定項目を書きだして上書きする仕様なので、通常の入出力フローに含めないということは「普段は絶対書かない」ということです。設定リセットを行う場合は、その場でデフォルト設定を書きだすようになっていたと思いますが、設定リセットエントリは書き出しが行われた時点で設定ファイルからいなくなります。 インストーラについては、ちゃんと考えられてないです 😢 |
設定リセットボタンを押した時に初期設定ファイルの書き込みを行うのだからその場合は設定リセットエントリの存在と起動時の初期化(設定ファイルから読んだ内容で設定情報を更新しない)動作に頼る必要も無いという事ですね。それは理解できます。 設定リセット用エントリの話が出てきたのはインストーラーが書いた中途半端な設定ファイルの判断用がそもそもですが、そのエントリを読み込み時に更新するとなると(なんかやってる事が強引ですが)読み込み時には 設定リセットエントリを使わない方法を考えました。 設定ファイルが存在しない状態で起動した場合の表示言語DLLを、OSの表示言語が日本語で無かったらプログラム側で |
正直にいうとそんな感じにしたいです。 言語設定のエントリがない場合はOSの言語選択に追随くらいの勢いで自動判定させたいです。 実現するためには英語化というか、国際化対応ってなんだっけ?から考え直す必要があると思っていて、他の対応に優先して進める決断をできないでいます。#610 のissueの件もあるので、どっかで手は入れないといけないんですけど。
それでいいと思ってます。 もちろん、別件として「中途半端な設定ファイルを正しく読めない」は解決せにゃならん問題だとおもっていますが、当座の対応としてはそれで十分だと思います。 |
それでいいと思います。 |
転載です。 リンクされている EXE は Build 1247 を指していました。Build 1240 では落ちませんでした。 これがちょっと関係してるかなーと思いましたが調べていません>「SAKURA Editor / PatchUnicode / #1116 CFileNameManager::GetIniFileName の問題含みの仕様を正します。」 |
追いかけれてませんが、結局のところ、インストーラーは、iniファイルの szLanguageDll を生成していることそのものをやめてもよいんでしたっけ? |
インストーラでini書くのはやめたほうがいいかも、なレベルで認識はあってますが このPRは単体で「MRUリスト(最近使ったファイル)を全クリアした場合に設定を初期化する」という変更なので、どっかに注意書きしとかないと「バグだ」と騒がれる懸念はあります。書いてあっても騒ぐ人は騒ぐんでしょうけど・・・。 その点についても「どうする」までの合意は取れてなかったはず。 |
了解です。 |
インストーラーでINIファイル作るようにしたのはもっと国際化対応したいからだと伺ってます。
的な対応をするのが良いと思います。が、手は付けていません(必要に応じてカイジの利根川の名言を再生して下さい)。 |
補足しておきますと、設定ファイルの 例えばサクラエディタの
なお自分のPRを弁護しましたが、思いもよらぬ操作で設定が消失するバグが存在し無い事の証明は出来ません。 |
そういう変更だと勘違いしてました。 ← |
InnoSetupのソースコード を調べてみたところ、WindowsAPI の という事で起動時に設定ファイルが無くて、 言語識別子の一覧は下記のURLに載っていました。 |
…e_settings 設定データ読み込み処理において言語設定切り替え後にMRUエントリが無い場合は新規インストール後とみなし false を返すように変更
このPRの変更の目的は、#616 で報告した問題に対処する為です。
%APPDATA%/sakura.ini
ファイルの内容が新規インストール後のものかどうかの判断は、[MRU]
セクションの_MRU_Counts
エントリが存在しないかどうかで判定しています。ちょっとこの判定方法だと単純すぎて危ないかも知れませんが…。。
新規インストールと判断した場合は
return false;
で処理を中断する事で後続する各設定読み込み処理を実行しないようにしました。ほぼ空な設定ファイルから設定情報を呼んでしまうとおかしな事になってしまうので。それに加えて下記の変更も加えました。
管理プロセスから呼び出ししている設定データ読み込み処理(CShareData_IO::LoadShareData)の戻り値を使うように変更
後続する設定データ保存処理(CShareData_IO::SaveShareData)の呼び出しは設定ファイルが存在しないという判断ではなくて、設定データ読み込みに処理に失敗したらという判断に変更