外部コマンド実行でキャンセルした時のメッセージが異なる問題を修正 #1721

beru · 2021-08-29T11:03:52Z

PR の目的

#1720 で報告された、外部コマンド実行でキャンセルした時のメッセージが異なる問題を修正するのが目的です。

カテゴリ

不具合修正

テスト内容

テスト1

手順

外部コマンド実行で名前「notepad」「標準出力を得る」「アウトプットウィンドウ」でコマンドを実行します。
アウトプット画面が表示されたら閉じます。
notepadの終了待ちのダイアログをキャンセルします。
2個目の新しくできたアウトプットウィンドウのメッセージが正しいか確認します。

関連 issue, PR

#1720

AppVeyorBot · 2021-08-29T11:32:05Z

✅ Build sakura 1.0.3903 completed (commit dcc6e4666c by @beru)

usagisita · 2021-08-29T18:03:45Z

sakura_core/view/CEditView_ExecCmd.cpp

@@ -570,7 +570,7 @@ bool CEditView::ExecCmd( const WCHAR* pszCmd, int nFlgOpt, const WCHAR* pszCurDi
 		if( bCancelEnd && bOutputExtInfo ){
 			//	2006.12.03 maru アウトプットウィンドウにのみ出力
 			//最後にテキストを追加
-			oa.OutputW( LS(STR_EDITVIEW_EXECCMD_STOP) );
+			oa.OutputW( CLoadString().LoadStringW(STR_EDITVIEW_EXECCMD_STOP) );


CSelectLang.h/cppのコード上のメソッド名定義はLoadStringですね。
WIN32APIのマクロによりLoadStringWに置換されるのだと思いますが、サクラ上の定義とは食い違います。
たぶんここはWなしのLoadStringのほうにしたほうがいいと思います。

指摘ありがとうございます。 d0d63b7 で修正しました。

AppVeyorBot · 2021-08-29T23:27:11Z

✅ Build sakura 1.0.3904 completed (commit 90f20ba942 by @beru)

berryzplus · 2021-09-03T03:43:29Z

何がいけなかったんですかね？

berryzplus · 2021-09-03T03:48:56Z

sakura_core/view/CEditView_ExecCmd.cpp

@@ -570,7 +570,7 @@ bool CEditView::ExecCmd( const WCHAR* pszCmd, int nFlgOpt, const WCHAR* pszCurDi
 		if( bCancelEnd && bOutputExtInfo ){
 			//	2006.12.03 maru アウトプットウィンドウにのみ出力
 			//最後にテキストを追加
-			oa.OutputW( LS(STR_EDITVIEW_EXECCMD_STOP) );
+			oa.OutputW( CLoadString().LoadString(STR_EDITVIEW_EXECCMD_STOP) );


全く分からんです。

この直し方で修正するってことは、他にも「直さないといけない箇所」がたくさん出てきそうで、影響調査とかめんどいです。

oa.OutputW()の呼出し箇所すべてで考慮が必要ですよね？

渡された引数の内容を処理完了までキープしておく責任があるのは呼ばれた側のような気がします。

oa.OutputW()の呼出し箇所すべてで考慮が必要ですよね？

呼び出し箇所で問題が起きそうかどうかの見極めは必要だと思います。

文字列リテラル等の定数値を渡している箇所については特に問題無いと思います。
スタック上の文字列領域もリエントラントなので多分大丈夫だと思います。
LSマクロはstatic記憶領域の変数を使う CLoadString::LoadStringSt の呼び出しになりリエントラントではなく問題が発生しました。

OutputW で Grep したところ他にLSマクロと組み合わせて使用している記述は見当たりませんでした。
なので多分大丈夫な気もしますが、考慮漏れで同様の問題が起きる可能性は否定出来ません。

まぁワニワニパニック大好きなのでその時はまたなんとかすれば良いと思います。変更をする事で不具合がたくさん追加されるとかだとさすがにまずいと思いますが。。

渡された引数の内容を処理完了までキープしておく責任があるのは呼ばれた側のような気がします。

それは一概に言い切れないケースバイケースだと思います。

bool COutputAdapterDefault::OutputW(const WCHAR* pBuf, int size)

こういうプロトタイプのメソッドの呼び出しですが、第1引数のポインタが指す領域の内容が変わる事は、少なくとも利用する呼ぶ側では意識しないと思います（なのでLSマクロのままにしたいんだと言われたら納得しちゃいそう）。呼ばれる側の実装でもなかなか意識しにくいので問題が生じうる実装になっているのではないかと。

影響確認実施済みなのは了解しました。
また、oa.OutputW()の呼ばれた側で対策するのがつらいのもなんとなく分かりました。

beru · 2021-09-03T20:25:57Z

何がいけなかったんですかね？

問題発生のメカニズムについては、#1720 (comment) に書かれている通り、下記の流れで問題が起きているんじゃないかと思います。

=> COutputAdapterDefault::OutputW
=> COutputAdapterDefault::OutputBuf
=> m_pCShareData->TraceOutString
=> CShareData::OpenDebugWindow
=> ActivateFrameWindow
=> SendMessageTimeout

~~~メッセージループ~~~

=> CEditWndProc
=> CEditWnd::DispatchEvent
  case WM_ACTIVATEAPP:
=> CEditWnd::UpdateCaption
=> CSakuraEnvironment::ExpandParameter
  const wstring	PRINT_PREVIEW_ONLY		= LS( STR_PREVIEW_ONLY );	//L"(印刷プレビューでのみ使用できます)";
=> CLoadString::LoadStringSt

LSマクロがstatic記憶領域の変数を使う CLoadString::LoadStringSt の呼び出しになりリエントラントでは無いのが問題の根本的な原因です。ただLSマクロは使用頻度が高いのでメモリの動的確保を避ける為にstatic記憶領域を使う実装にした判断はなかなか否定出来ないです。

他に考えられる対策としては CShareData::TraceOutString で OpenDebugWindow を呼ぶ前に引数の文字列を別の領域にコピーする事でしょうか？ただ文字列リテラル等の定数値を渡している場合にはそれは本来は不要な事だと思うので少しもったいないですね。

GUIのデザインが問題が起きうる操作を誘発している気がしますが、まぁ古いソフトなのでしょうがないですね。

berryzplus · 2021-09-07T23:03:13Z

妥協できる直し方を書いてみます

			//最後にテキストを追加
			oa.OutputW( LS(STR_EDITVIEW_EXECCMD_STOP) );

　👇

			// L"\r\n中断しました。\r\n"
                        std::wstring msg( LS(STR_EDITVIEW_EXECCMD_STOP) );
			oa.OutputW( msg.c_str(), (int)msg.length() );

・OutputWの処理内容を説明する無駄なコメントを削除。
・リソースの内容が分かるようコメントを付加。
・リソース文字列をコピーするよう変更。

こういう書き方にするメリットは、普通の人でも「ちょっと待て！」と言えるようになることです。

beru · 2021-09-08T14:43:15Z

妥協できる直し方を書いてみます
			//最後にテキストを追加
			oa.OutputW( LS(STR_EDITVIEW_EXECCMD_STOP) );
　👇
			// L"\r\n中断しました。\r\n"
                        std::wstring msg( LS(STR_EDITVIEW_EXECCMD_STOP) );
			oa.OutputW( msg.c_str(), (int)msg.length() );
・OutputWの処理内容を説明する無駄なコメントを削除。
・リソースの内容が分かるようコメントを付加。
・リソース文字列をコピーするよう変更。

こういう書き方にするメリットは、普通の人でも「ちょっと待て！」と言えるようになることです。

berryzplusさんがその書き方にしたいならそうします。ちなみに大文字の単語 LS でGrepすると900件近くヒットします。文字列ロード簡易化マクロ LS がサクラエディタのコードでは標準的に使われているので、リソース文字列を読み込む記述で変な例外的な記述をしたくはないという考えでしょうか？

個人的には CLoadString クラスの LoadString メソッドを使うと異常者と見做されるのが解せないのですが…。

berryzplus · 2021-09-20T15:08:53Z

berryzplusさんがその書き方にしたいならそうします。

「その書き方にしたいかどうか」は自明だと思いますが 😃
長いこと回答を付けてなかったカタチになってるのであえて回答します・・・「はい」です。

ちなみに大文字の単語 LS でGrepすると900件近くヒットします。
文字列ロード簡易化マクロ LS がサクラエディタのコードでは標準的に使われているので、リソース文字列を読み込む記述で変な例外的な記述をしたくはないという考えでしょうか？

ほぼほぼお察しの通りです。

「標準的な記述プラスアルファ」にしておけば、
「なんか特殊なことしてる」が分かりやすいんじゃないかなと思います。

個人的には CLoadString クラスの LoadString メソッドを使うと異常者と見做されるのが解せないのですが…。

「異常者」と看做したつもりはないです。
が、その方式はなんか気に食わんです・・・。

CLoadString クラスの LoadString メソッドを使う方式が気に食わない理由を挙げてみます。
（理由を挙げて妥協できるか検討します。）

クラス名がWindows SDKのマクロと酷似していて紛らわしい（目視による誤認を誘発する気がします
メソッド名がWindows SDKのマクロと衝突していて「超」紛らわしい（IDEのコード参照機能が使えなくて不便です
既存コードで利用実績がほとんどない

　👇

クラス名が～はなんかもう、存在否定ですよね。ただ、マクロなどを使ってクラス名がコード中に現れないようにしてやれば、クラス名の問題は回避できます。マクロを使った回避策だと「クラス名を直接書かないといけないケース」に遭遇したときに「なんだこの珍クラス名は？」ってなりますね、今回のように。まぁ、誤認による被害（？）は起きない気がするので、この点は妥協できそうです。
メソッド名が～はLoadStringWと書くことで回避できます。外部コマンド実行でキャンセルした時のメッセージが異なる問題を修正 #1721 (comment) で指摘されて「Wなし」に修正してますね。問題の本質はIDEがショボ・・・じゃなかった、windows SDKの「マクロ」と同名の関数を「誤って」定義していることなので、指摘も修正も「悪いってわけじゃない」と思います。このメソッドは、利用箇所が少ないうちに適切な名前（≒Windows SDKマクロと衝突しない名前）にリネームすべきだと思います。この点は、できれば妥協したくないです。
既存コードは、１箇所だけですがCLoadString::LoadStringWメソッドを利用しています。たった1箇所でも利用実績なので、これをもって「流用できる」と判断するのは妥当と思います。なので、この点は妥協できます。

というわけで、総合的に見て結論は「妥協できる」になりました。

beru · 2021-09-20T15:47:53Z

「その書き方にしたいかどうか」は自明だと思いますが 😃
長いこと回答を付けてなかったカタチになってるのであえて回答します・・・「はい」です。

回答ありがとうございます。特にこの変更に関してこだわりがあるわけではないのでberryzplusさんが書いてくれた内容で更新しておきました。

「標準的な記述プラスアルファ」にしておけば、
「なんか特殊なことしてる」が分かりやすいんじゃないかなと思います。

事情を知らない人が見ると文字列型の一時変数にどうしてコピーしてるんだろう？と思うかもしれないですね。
とはいえ読み手に分かるようにコメントで解説を書くのもなんか辛いです。

CLoadString クラスの LoadString メソッドを使う方式が気に食わない理由を挙げてみます。

というわけで、総合的に見て結論は「妥協できる」になりました。

命名はなんだか悩ましいですね。CLoadString::LoadString という名前が微妙ですが、これを今更変える気は自分はしないです。LSマクロは名前が2文字で衝突が怖いですが、お手軽に使えるのは良いとこですね。

sonarqubecloud · 2021-09-20T15:48:41Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

AppVeyorBot · 2021-09-20T16:12:51Z

✅ Build sakura 1.0.3910 completed (commit 2acbd09a5d by @beru)

berryzplus

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

コードの意図を1行コメントで追加したほうがいいかも・・・と思ったので気が向いたら別件で追加修正の提案を出すかも知れません。

beru · 2021-09-20T22:35:47Z

レビューありがとうございました。Mergeします。

外部コマンド実行でキャンセルした時のメッセージが異なる問題を修正

242f785

beru added the 🐛bug🦋 ■バグ修正(Something isn't working) label Aug 29, 2021

usagisita reviewed Aug 29, 2021

View reviewed changes

レビュー指摘対応

d0d63b7

berryzplus reviewed Sep 3, 2021

View reviewed changes

レビュー指摘に対応

634008b

berryzplus approved these changes Sep 20, 2021

View reviewed changes

beru merged commit f706330 into sakura-editor:master Sep 20, 2021

beru deleted the ExecCmd_LS_overwritten_STR_EDITVIEW_EXECCMD_STOP branch September 20, 2021 22:35

beru mentioned this pull request Sep 21, 2021

外部コマンド実行でキャンセルした時のメッセージが異なる #1720

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

外部コマンド実行でキャンセルした時のメッセージが異なる問題を修正 #1721

外部コマンド実行でキャンセルした時のメッセージが異なる問題を修正 #1721

beru commented Aug 29, 2021

AppVeyorBot commented Aug 29, 2021

usagisita Aug 29, 2021

beru Aug 29, 2021 •

edited

Loading

AppVeyorBot commented Aug 29, 2021

berryzplus commented Sep 3, 2021

berryzplus Sep 3, 2021

beru Sep 3, 2021

berryzplus Sep 7, 2021

beru commented Sep 3, 2021

berryzplus commented Sep 7, 2021

beru commented Sep 8, 2021 •

edited

Loading

berryzplus commented Sep 20, 2021

beru commented Sep 20, 2021

sonarqubecloud bot commented Sep 20, 2021

AppVeyorBot commented Sep 20, 2021

berryzplus left a comment

beru commented Sep 20, 2021

外部コマンド実行でキャンセルした時のメッセージが異なる問題を修正 #1721

外部コマンド実行でキャンセルした時のメッセージが異なる問題を修正 #1721

Conversation

beru commented Aug 29, 2021

PR の目的

カテゴリ

テスト内容

テスト1

関連 issue, PR

AppVeyorBot commented Aug 29, 2021

usagisita Aug 29, 2021

Choose a reason for hiding this comment

beru Aug 29, 2021 • edited Loading

Choose a reason for hiding this comment

AppVeyorBot commented Aug 29, 2021

berryzplus commented Sep 3, 2021

berryzplus Sep 3, 2021

Choose a reason for hiding this comment

beru Sep 3, 2021

Choose a reason for hiding this comment

berryzplus Sep 7, 2021

Choose a reason for hiding this comment

beru commented Sep 3, 2021

berryzplus commented Sep 7, 2021

beru commented Sep 8, 2021 • edited Loading

berryzplus commented Sep 20, 2021

beru commented Sep 20, 2021

sonarqubecloud bot commented Sep 20, 2021

AppVeyorBot commented Sep 20, 2021

berryzplus left a comment

Choose a reason for hiding this comment

beru commented Sep 20, 2021

beru Aug 29, 2021 •

edited

Loading

beru commented Sep 8, 2021 •

edited

Loading