ファイルを開く、で _MAX_PATH (260) より長いパスのファイルの場合はエラーメッセージを出して処理を中断 #409

beru · 2018-09-04T22:55:31Z

#401 で検討中ですが、アプリが落ちないようにする対策を入れました。

本当は扱えるようにした方が良いですがすぐには直せないので暫定的な対応です。

berryzplus

対応ありがとうございます。LGTMです。

例によって、主題と関係ないツッコミがいくつかあるので、適宜課題化するなりスルーするなりで進めていきたい感じです。

sakura_core/String_define.h

berryzplus · 2018-09-06T11:12:47Z

sakura_core/cmd/CViewCommander_File.cpp

@@ -136,6 +136,16 @@ void CViewCommander::Command_FILEOPEN( const WCHAR* filename, ECodeType nCharCod
 		);
 		if(!bDlgResult)return;

+		for(size_t i = 0; i < files.size(); i++ ){


std::for_eachで書くのとfor文で書くのとどちらが見やすいですかねぇ。

他と合わせるなら
for(int I =0; I<file.size(); I++)
と書いてsignedとunsignedの比較で怒られるパターンだと思います。

サイズ値にはintで十分なものとそうでないものがあると思っていますが、これの場合前者だと思います。
「ファイルを開く」ダイアログで2億ファイルを選択できるか？ってことです。

std::for_eachで書くのとfor文で書くのとどちらが見やすいですかねぇ。

std::for_eachで書く場合はFunctorとかLambda functionと組み合わせる事になると思うので読みにくくなると思います。
Range-based for loop が見た目が一番すっきりすると思いますが、自分がRange-based for loopを書きなれていないので読みにくいです。

再起とかで無理やり処理するのは書きにくいし読みにくいです。数を限定してFall-throughなswitch文で書く方法も出来そうですが…。

あまりc++やらない人のためにc++のforeachの例を書くとこんなんです。

std::for_each(files.cbegin(), files.cend(), [] (const auto& file) { // ←繰り返す処理をラムダ式で指定している ... });

たしかにちょっと見辛いかも知れませんね。
「いちばんすっきりする」という C++11の範囲 forだとこんなんになります。

for (const auto& file : files) { ... // ← 普通のfor文と同じに書ける }

すっきりしすぎてて違う言語みたくなってますが、
実はこれ、昔から使える定石パターンを生成するだけの糖衣構文らしいです。

for (const auto &iter = files.cbegin(); iter != files.cend(); ++iter) { const auto &file = *iter; ... }

ちなみに、自分はこの書き方が好きです。
「それ、std::for_eachじゃないじゃんｗ」ってツッコミはスルーします。

for (const auto &iter = files.cbegin(); iter != files.cend(); ++iter) { const auto &file = *iter; ... }

この書き方ですが実際には、

for (auto it = files.cbegin(); it != files.cend(); ++it) { auto &file = *it; ... }

になると思います。

便利な Iterator ですが自分はstaticおじさん指向なので出来るだけ使用を回避しています。

この書き方ですが実際には、

実用コードになると色々変形すると思ってます。

auto beg = files.begin(); // 開始位置を記憶 auto end = files.end(); // 終了位置を記憶 for (auto it = beg; it != end; ++it) { auto file = *it; // 変更できないのは嫌なのでconst外しちゃう auto idx = it - beg; // 「staticおじさん」はおおむね配列思考 ... }

こんなんするくらいなら
for (int i = 0; i < files.size(); i++) でよくね？
って見方は分からんでもないです。

きっとループの書き方が将来のC++では何十種類もバリエーションが増えて、いつかシンプルなgotoが評価される日が来ると思います。

berryzplus · 2018-09-06T11:14:24Z

sakura_core/cmd/CViewCommander_File.cpp

+		for(size_t i = 0; i < files.size(); i++ ){
+			if (files[i].length() >= _MAX_PATH){
+				ErrorMessage(
+					CEditWnd::getInstance()->GetHwnd(),


このカスタム関数の第一引数は省略可能です。
省略した場合は CEditWnd::getinstance()->GetWnd() になります。

省略可能というのは、オーバーロードが有るわけでは無くて NULL 指定したら、って事のようですね。

そうです。省略（＝NULLを指定）しても同じ動きになります。

ここに CEditWnd::getinstance()->GetWnd() を書いてしまうと、
この関数を単体テストするときに CEditWnd のインスタンスが必須になります。
NULL 指定にしておけば ErrorMessage 自体をマクロで置き換えたりして、
コード単体での動作検証がラクになります。

現状ではGUI絡むところは単体テスト書かれてないみたいですね。
単体テスト記述する時になってから手直しすれば良いかなぁと思います。

berryzplus · 2018-09-06T11:16:11Z

sakura_core/cmd/CViewCommander_File.cpp

+				ErrorMessage(
+					CEditWnd::getInstance()->GetHwnd(),
+					LS(STR_ERR_FILEPATH_TOO_LONG),
+					files[i].c_str()


複数のファイルを開こうとしてうち1つが制限にかかった場合、制限にかかったファイル名のみが表示されそうな書き方です。いったんはこれで問題ないと考えますが、他のファイルは処理しなくていいんでしたっけ？という細かいツッコミ。

ファイルダイアログで同じフォルダにあるファイルを複数選択して開こうとすると下記のメッセージが表示されました。

[Window Title] ファイルを開く [Content] 複数の項目の選択は、項目がすべて同じフォルダーにある場合にのみできます。 [OK]

適切なエラーメッセージじゃないと思います。OS側の対応が不十分ですし、とりあえず１つでも制限に引っかかったらまとめて却下という対応でも良いかなと判断しました。

適切なエラーメッセージじゃないと思います。

Documents みたいな仮想フォルダでフォルダを跨いだファイル選択すると再現可能ですね・・・。
これはこれで正しい気がしています。
OS側として「ファイルを開く」で選択される複数ファイルは、同じディレクトリに存在している前提なので、どのファイルが他と違う、というより、選択方法に誤りがある、を指摘したいんだと思うからです。

同じフォルダにあるファイルを複数選択した場合（そしてその中の1つ以上のファイル名が MAX_PATH を超えて長い特殊例）なのに、そのエラーメッセージが出てしまうので不適切だと思っています。天に唾してもしょうがないのですが…。

＞> どのファイルが他と違う、というより、選択方法に誤りがある、を指摘したいんだと思うからです。

複数の項目は全て同じフォルダーにあるのに、そうなってないよ、というエラーメッセージが出るので、おかしい、と言っています。なんか話が噛み合ってない気がしますが、限りなく黒に近いグレーに細かいツッコミをしてはいけないという事でしょうか…。

何の話してたかようやく把握です。
それはおかしいｗ
（複数ファイル選択のうち１つがMAX_PATHを超えたときに「フォルダが違う」と言われる）

なんか話が噛み合ってない気がしますが、限りなく黒に近いグレーに細かいツッコミをしてはいけないという事でしょうか…。

理解できてませんでした。
ぼく基本スペックはアフォなので。

そもそも MAX_PATH を超えるようなファイルを扱うユーザーが悪いんですよね。そんな事言ったらこのPRの意味 Nothing ですが…。

berryzplus · 2018-09-06T11:17:19Z

sakura_core/cmd/CViewCommander_File.cpp

+				);
+				return;
+			}
+		}
 		sLoadInfo.cFilePath = files[0].c_str();


本質的にはここの左辺の型の問題なんですよね。

左様です。

berryzplus · 2018-09-06T11:21:00Z

sakura_core/sakura_rc.rc

@@ -4121,6 +4121,8 @@ GDI リソース\t残り  %d%%\n\n"
 	STR_FILETREE_REPLACE_PATH_FROM	"置換元パスを指定してください"
 	STR_FILETREE_REPLACE_PATH_TO	"置換先文字列を指定してください"

+	STR_ERR_FILEPATH_TOO_LONG		"%ls\nというファイルを開けません。\nファイルのパスが長すぎます。"


%ls なんですね。
プロジェクト標準的には %ts だと思っています。
個人的には %s でよいと考えております。

ちらっと思ったことですが、このメッセージってwindows標準でありませんでしたっけ？（ちゃんと調べてないですが）

%ls とか %ts とか良く分からなくてコピペしてました。

こんなページに遭遇しました。
https://sourceforge.net/p/sakura-editor/wiki/%E4%B8%8D%E5%AE%89%E7%AE%87%E6%89%80/

sourceforge側のWikiに結構色々なページがあるのでgithub側に移行した方が良さそうですね。
https://sourceforge.net/p/sakura-editor/wiki/browse_pages/?limit=250

ちらっと思ったことですが、このメッセージってwindows標準でありませんでしたっけ？（ちゃんと調べてないですが）

FormatMessage に ERROR_FILENAME_EXCED_RANGE を渡して取ってくると良いかも知れないですね。

独自記法 %ts の発生経緯については把握しています。
UNICODE 一本化により存在意義が薄れた認識です。

今後、auto_sprintf(char*, char*, ...) をwchar_t*版に置き換える活動を画策していて、
それが終わったら s/%ts/%s/g の全置換を行いたいと考えています。
その後、auto_sprintf(wchar_t*, wchar_t*, ...) の内部で使っているCRT関数を
sprintf_s → sprintf_p として言語によって埋め文字の位置が変わるケースに対応したいです。

FormatMessage に ERROR_FILENAME_EXCED_RANGE を渡して取ってくると良いかも知れないですね。

あとで検証してみまっす 😄

auto_sprintf を整備する場合は、出力先が固定長配列の場合のオーバーロードを用意して、配列要素数を引数に記述しないでも良くなると良いですね。

ソースコードの規模が大きいのでリファクタ色々しても焼け石に水的な感じはありますが…。

berryzplus · 2018-09-06T11:23:20Z

sakura_lang_en_US/sakura_lang_rc.rc

@@ -4132,6 +4132,9 @@ GDI Resource\tRemaining  %d%%\n\n"
 	STR_FILETREE_REPLACE_PATH_FROM	"Please Input Replace 'From' Path"
 	STR_FILETREE_REPLACE_PATH_TO	"Please Input Replace 'To' Path"

+	STR_ERR_FILEPATH_TOO_LONG		"'%ts'\nCannot open the file.\nThe filepath is too long."


%ts なんですね・・・。

英語メッセージではファイルパスをシングルクォートで囲っているけれど、日本語メッセージでは囲っていないという違いもありますね。

berryzplus · 2018-09-11T11:07:18Z

これってなんでマージしないんでしたっけ。

beru · 2018-09-11T12:52:43Z

これってなんでマージしないんでしたっけ。

PR出した当人が Merge はどうかなと思うので自分ではやってません。
もし問題が無いようでしたら Merge お願いします。
もし後で問題が見つかった場合には Revert 掛ければ良いかなと。

beru · 2018-09-12T18:33:44Z

う、反応が無いですね。

このまま放置しててもしょうがないので自分で Merge してしまいます。
問題があった時は別PRで対応する事にしましょう。

berryzplus · 2018-09-14T14:32:06Z

う、反応が無いですね。

ここ最近忙しかったので、流してしまいました。
書き込んだ意図は、マージどうぞ、です。

このまま放置しててもしょうがないので自分で Merge してしまいます。

ぼくは GitHub の文化にはあまり詳しくないんですけど、
別の人にレビューしてもらえれば自分でマージしてよいみたいです。

他プロジェクトもちょこちょこ見てるんですが、
自分でPR出してマージしてるところもあったりしてよく分らんです。

ファイルを開く、で _MAX_PATH (260) より長いパスのファイルの場合はエラーメッセージを出して処理を中断

ファイルを開く、で _MAX_PATH (260) より長いパスのファイルの場合はエラーメッセージを出して処理を中断

ec13b08

berryzplus approved these changes Sep 6, 2018

View reviewed changes

beru merged commit 3e61097 into sakura-editor:master Sep 12, 2018

beru deleted the reject_LFN branch September 15, 2018 00:20

m-tmatma added this to the next release milestone Oct 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ファイルを開く、で _MAX_PATH (260) より長いパスのファイルの場合はエラーメッセージを出して処理を中断 #409

ファイルを開く、で _MAX_PATH (260) より長いパスのファイルの場合はエラーメッセージを出して処理を中断 #409

beru commented Sep 4, 2018

berryzplus left a comment

berryzplus Sep 6, 2018

beru Sep 6, 2018

berryzplus Sep 6, 2018

beru Sep 7, 2018 •

edited

Loading

berryzplus Sep 8, 2018

beru Sep 10, 2018

berryzplus Sep 6, 2018

beru Sep 7, 2018

berryzplus Sep 8, 2018

beru Sep 10, 2018

berryzplus Sep 6, 2018

beru Sep 6, 2018

berryzplus Sep 8, 2018

beru Sep 8, 2018

berryzplus Sep 8, 2018

beru Sep 8, 2018

berryzplus Sep 6, 2018

beru Sep 7, 2018

berryzplus Sep 6, 2018

beru Sep 7, 2018

berryzplus Sep 8, 2018

beru Sep 8, 2018

berryzplus Sep 6, 2018

beru Sep 10, 2018

berryzplus commented Sep 11, 2018 •

edited

Loading

beru commented Sep 11, 2018

beru commented Sep 12, 2018

berryzplus commented Sep 14, 2018

ファイルを開く、で _MAX_PATH (260) より長いパスのファイルの場合はエラーメッセージを出して処理を中断 #409

ファイルを開く、で _MAX_PATH (260) より長いパスのファイルの場合はエラーメッセージを出して処理を中断 #409

Conversation

beru commented Sep 4, 2018

berryzplus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beru Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berryzplus commented Sep 11, 2018 • edited Loading

beru commented Sep 11, 2018

beru commented Sep 12, 2018

berryzplus commented Sep 14, 2018

beru Sep 7, 2018 •

edited

Loading

berryzplus commented Sep 11, 2018 •

edited

Loading