フォルダ選択ダイアログを SHBrowseForFolder() でなく IFileDialog を使用するように変更 #1609

Ocelot1210 · 2021-03-27T10:38:15Z

PR の目的

フォルダ選択ダイアログの操作性を向上させる事が目的です

カテゴリ

仕様変更

PR の背景

SHBrowseForFolder はツリー形式の UI のため操作性に難があります(特に深い階層のフォルダを選択する場合)
Windows Vista 以降では SHBrowseForFolder ではなく、 FOS_PICKFOLDERS オプションを指定した IFileDialog を使用する事が推奨されています(詳細は参考資料 No.1 をご参照下さい)

PR のメリット

フォルダ選択ダイアログの操作性が向上します

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

Vista 以前の OS (2000/XP) でフォルダ選択ダイアログが動作しなくなります

仕様・動作説明

変更前

SHBrowseForFolder を使用してフォルダ選択ダイアログを表示します

変更後

IFileDialog に FOS_PICKFOLDERS オプションを指定し、フォルダ選択ダイアログを表示します

PR の影響範囲

フォルダ選択ダイアログを表示する以下の項目に影響があります

メニュー→設定→共通設定→バックアップ→フォルダ名
メニュー→設定→共通設定→プラグイン→設定→値
- 確認用に使用したプラグインは参考資料 No.2 をご参照下さい
メニュー→設定→共通設定→マクロ→マクロ一覧
メニュー→設定→共通設定→支援→migemo設定→辞書
メニュー→設定→共通設定→編集→ファイルダイアログの初期位置→指定フォルダ
メニュー→ツール→外部コマンド実行→カレントディレクトリ
メニュー→検索→ファイルツリー→設定→パス
メニュー→検索→Grep→検索場所
メニュー→検索→タグファイルの作成→タグ作成フォルダ
マクロ専用関数 FolderDialog() が呼ばれた際

テスト内容

テスト1

手順

PR の影響範囲の 1～9 にあるフォルダ選択ダイアログが表示されるボタンを押す
フォルダ選択ダイアログのタイトルが変更前後で差異が無い事を確認する
フォルダ選択ダイアログにて「キャンセル」ボタンを押した際の動作が変更前後で差異が無い事を確認する
フォルダ選択ダイアログにて「フォルダーの選択」ボタンを押した際の動作が変更前後で差異が無い事を確認する

テスト2

手順

以下の内容のファイル(拡張子は.js)を作成し、メニュー→ツール→名前を指定してマクロ実行で開く

result = Editor.FolderDialog("フォルダを選択してください", "C:\\Windows")
Editor.MessageBox(result, 0x40)

フォルダ選択ダイアログのタイトルが変更前後で差異が無い事を確認する
フォルダ選択ダイアログにて「キャンセル」ボタンを押した際の動作が変更前後で差異が無い事を確認する
フォルダ選択ダイアログにて「フォルダーの選択」ボタンを押した際の動作が変更前後で差異が無い事を確認する

参考資料

AppVeyorBot · 2021-03-27T11:10:29Z

✅ Build sakura 1.0.3610 completed (commit ac8181694c by @Ocelot1210)

berryzplus · 2021-03-27T12:22:13Z

@Ocelot1210 さん　はじめましてでしょうか。PR趣旨は了解です。

1 Security Hotspot

いまのところ、レビューして「問題ありません」で解決した実績はないです。
wcscpy_s を使うように修正するだけで良いので対応をお願いしたいです。

4 Code Smells

C++ではポインタに NULL(=数値0のこと) を使うべきでないそうです。
ネストしたコードブロックには、通常「名前」を付けられます。
関数化して処理の「名前」を明示したほうがメンテナンスしやすくなるそうです。
関数の戻り値が使用されないパスがあるようです。
戻り値は処理の成否を表すことが多いので、取得したら何らかの判定を行うべきと思います。

SonarCloud指摘に対する対処は「絶対必要」というわけではないです。

対処することによって修正を誤る危険がある。
対処するのがすっごくめんどい。
明らかに Fault Positive （＝擬陽性、解析ツールのバグじゃね？）。

「対応しない理由」が（普通に考えて）納得できるようなら「スルー」で良いと思っています。

Ocelot1210 · 2021-03-27T14:27:48Z

@berryzplus さん、はじめまして
PR の内容のご確認及びアドバイスを頂きありがとうございます
普段 C++ を触らないので勉強になりました
GitHub / SonarCloud をあまり理解していないためお手数をおかけして申し訳ございません
ご指摘頂いた内容を対応いたしました

wcscpy_s を使うように修正するだけで良いので対応をお願いしたいです。

本件、格納先の要素数を渡すため SelectDir 関数の引数が増えましたがご容赦ください

AppVeyorBot · 2021-03-27T14:57:15Z

❌ Build sakura 1.0.3612 failed (commit 6d2c2a9783 by @Ocelot1210)

berryzplus · 2021-03-28T11:40:10Z

appveyorのビルド失敗が意味不明なのでリビルドかけときました。

AppVeyorBot · 2021-03-28T12:02:56Z

✅ Build sakura 1.0.3612 completed (commit 6d2c2a9783 by @Ocelot1210)

berryzplus

判断できずに放置してしまっていました。
対応したい変更はできてそうなので結論を出しておきます。

「分かんなかったらapprove」で良いこともあると思うんですよね。
人間、一人で全領域のスペシャリストにはなれないのだから。
それで問題を起こさないようにするためのCIで、SonarCloudだと思っています。

一応、今回は動かしたっす 😃

beru

テンプレート引数で固定長配列の要素数を取る SelectDir のオーバーロード関数を追加する事で、呼び出し側で最後の引数指定（例えば _countof(szPath)）を省略出来るので便利かもと思いました。

Ocelot1210 · 2021-04-02T10:47:39Z

@beru さん、PR のご確認ありがとうございます
ご指摘頂いた点を対応いたしました

AppVeyorBot · 2021-04-02T11:19:43Z

✅ Build sakura 1.0.3631 completed (commit 32e6321cdd by @Ocelot1210)

sakura_core/outline/CDlgFileTree.cpp

AppVeyorBot · 2021-04-03T07:16:12Z

✅ Build sakura 1.0.3636 completed (commit 63b0339342 by @Ocelot1210)

berryzplus

細かいところは後まわしでもいいような気がするんですよね・・・。

berryzplus · 2021-04-04T07:20:24Z

sakura_core/util/shell.h

+BOOL SelectDir(HWND hWnd, const WCHAR* pszTitle, const WCHAR* pszInitFolder, WCHAR* strFolderName, size_t nMaxCount );	/* フォルダ選択ダイアログ */
+
+template <size_t nMaxCount>
+BOOL SelectDir(HWND hWnd, const WCHAR* pszTitle, const WCHAR* pszInitFolder, WCHAR(&strFolderName)[nMaxCount])


ここ、SonarCloudの警告がでました。
対応は必須でないと思いますが、あるべき姿で考えると「戻り値BOOL」がマズいのかもです。

std::wstring SelectDir( HWND hWnd, std::wstring_view title, std::wstring_view initFolder);

berryzplus · 2021-04-04T07:27:03Z

sakura_core/dlg/CDlgExec.cpp

@@ -259,7 +259,7 @@ BOOL CDlgExec::OnBnClicked( int wID )

 	case IDC_BUTTON_REFERENCE2:
 		{
-			if( SelectDir( GetHwnd(), LS(STR_DLGEXEC_SELECT_CURDIR), &m_szCurDir[0], &m_szCurDir[0] ) ){
+			if( SelectDir( GetHwnd(), LS(STR_DLGEXEC_SELECT_CURDIR), &m_szCurDir[0], &m_szCurDir[0], m_szCurDir.GetBufferCount() ) ){


戻り値BOOLをやめた場合、ここはこんな感じになります。

if( auto selectedDir = SelectDir( GetHwnd(), LS(STR_DLGEXEC_SELECT_CURDIR), &m_szCurDir[0] ); selectedDir.length() > 0 ){ m_szCurDir = selectedDir.c_str();

m_szCurDir の宣言型がおかしいのが原因で、ロジックが書きづらくなっています。
対応はしなくてもいいんじゃないかなぁ、と思います。

berryzplus · 2021-04-04T07:28:47Z

sakura_core/util/shell.cpp

+	@param [in] pDialog			設定対象のダイアログ
+	@param [in] pszInitFolder	初期フォルダに設定したいパス
+*/
+static void SetInitialDir( Microsoft::WRL::ComPtr<IFileDialog> pDialog, const WCHAR* pszInitFolder )


別に正しく書く必要はありませんが、使い方が間違っています。

Suggested change

static void SetInitialDir( Microsoft::WRL::ComPtr<IFileDialog> pDialog, const WCHAR* pszInitFolder )

static void SetInitialDir( IFileDialog* pDialog, const WCHAR* pszInitFolder )

あとは好みですが、第二引数は std::wstring_view がいいっす。

対応いたしました
ご指摘頂きありがとうございます

berryzplus · 2021-04-04T07:33:29Z

sakura_core/util/shell.cpp

+	wcscpy_s( szInitFolder, _countof(szInitFolder), pszInitFolder );
+
+	// フォルダの最後が半角かつ'\\'の場合は、取り除く "c:\\"等のルートは取り除かない
+	CutLastYenFromDirectoryPath( szInitFolder );


stdafx.hのマージでコンフリクトが発生したため。

berryzplus · 2021-04-04T07:57:59Z

あるべき姿で考えるとSelectDirのシグニチャはこうなんですよね。

std::filesystem::path SelectDir(
  HWND hWnd,
  std::wstring_view title,
  std::filesystem::path initFolder);

このグローバル関数が扱う対象は何でしょう？

パス文字列 = std::filesystem::path
文字列参照 = std::wstring_view
出力文字列 = std::wstring&
共有メモリ内の文字列 = SFilePath = StaticString<wchar_t, 260>

AppVeyorBot · 2021-04-04T13:47:41Z

✅ Build sakura 1.0.3640 completed (commit 8e1ad6126c by @Ocelot1210)

sakura_core/util/shell.cpp

AppVeyorBot · 2021-04-07T13:45:31Z

✅ Build sakura 1.0.3644 completed (commit d0f4746e1d by @Ocelot1210)

sakura_core/util/shell.cpp

AppVeyorBot · 2021-04-08T12:43:21Z

✅ Build sakura 1.0.3645 completed (commit 3c49f909ff by @Ocelot1210)

berryzplus · 2021-04-09T04:32:35Z

sakura_core/util/shell.cpp

+		return FALSE;
+	}
+
+	if ( nMaxCount < wcslen(pszResult) ) {


この行でSecurity Hotspotが検出されとります。

1 Security Hotspot

Suggested change

if ( nMaxCount < wcslen(pszResult) ) {

if ( pszResult != nullptr && nMaxCount < wcsnlen( pszResult, nMaxCount ) ) {

になるのかなぁ・・・。

IShellItem::GetDisplayName が成功時に pszResult の値が nullptr になる事があるのかどうかは疑問ですが、仮にそうなる事があるとしてその場合に wcsnlen を呼ばないようにしたいという事でしょうか？

ただこの記述だと仮に pszResult が nullptr の時に wcsnlen は呼ばれないですが、wcscpy_s は呼ばれますよね。

それならばそもそも pszResult が nullptr の場合は return FALSE; した方が良いと思いますね。まぁ無いと思いますが…。

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/strnlen-strnlen-s?view=msvc-160

の説明に下記のように書かれていました。

strnlen, wcsnlen, and _mbsnlen do not validate their parameters. If str is NULL, an access violation occurs.

strnlen_s and wcsnlen_s validate their parameters. If str is NULL, the functions return 0.

実際に pszResult が NULL 値になるとは思いませんが使うなら wcsnlen より wcsnlen_s が良いと思います。

あと wcsnlen はC標準規格には無いのか下記のページには載っていませんでした。

https://en.cppreference.com/w/c/string/wide/wcslen

元の wcslen 関数を呼び出す記述だと SonarCloud が Security Hotspot を検出してきますが、実際には問題は無いと思うんですよね。GetDisplayName 成功時に取得した文字列は有効領域と見なせると思います。

なお wcslen の代わりに wcsnlen_s を使う事による利点としては、SonarCloud が Security Hotspot を検出しなくなるのと、あと文字列長が出力先バッファ長を超える場合にそこでループを中断させる事が出来るので無駄が少ないですね。

というか IShellItem が DisplayName のバッファ長を返すメソッドを用意してくれていたら良かったんですが探しても見つかりませんでした。

sakura_core/util/shell.cpp

berryzplus · 2021-04-09T13:11:30Z

util/shell.cpp 104行目からの修正案です。

	BOOL bRet = TRUE;
	if ( STRUNCATE == wcsncpy_s( strFolderName, nMaxCount, pszResult, _TRUNCATE ) ) {
		wcscpy_s( strFolderName, nMaxCount, L"" );
		bRet = FALSE;
	}

	CoTaskMemFree( pszResult );

	return bRet;

これなら pszResult の長さを取得する必要も、初期値を設定する必要もないです。

beru · 2021-04-09T13:30:53Z

util/shell.cpp 104行目からの修正案です。
	BOOL bRet = TRUE;
	if ( STRUNCATE == wcsncpy_s( strFolderName, nMaxCount, pszResult, _TRUNCATE ) ) {
		wcscpy_s( strFolderName, nMaxCount, L"" );
		bRet = FALSE;
	}

	CoTaskMemFree( pszResult );

	return bRet;
これなら pszResult の長さを取得する必要も、初期値を設定する必要もないです。

wcsncpy_s を使う方法は #1609 (comment) でもコメントしましたが戻り値を確認して FALSE を返す事を思いつきませんでした。その方法は良いですね。

なお戻り値の判定は STRUNCATE ではなくて 0 以外かどうかの方が良い気がします。

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/strncpy-s-strncpy-s-l-wcsncpy-s-wcsncpy-s-l-mbsncpy-s-mbsncpy-s-l?view=msvc-160#return-value

Zero if successful, STRUNCATE if truncation occurred, otherwise an error code.

どういう時にエラーコードが返るのか？については
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/strncpy-s-strncpy-s-l-wcsncpy-s-wcsncpy-s-l-mbsncpy-s-mbsncpy-s-l?view=msvc-160#error-conditions

に書かれているので転載します。

errno_t wcsncpy_s(
   wchar_t *strDest,
   size_t numberOfElements,
   const wchar_t *strSource,
   size_t count
);

strDest	numberOfElements	strSource	Return value	Contents of strDest
NULL	any	any	EINVAL	not modified
any	any	NULL	EINVAL	strDest[0] set to 0
any	0	any	EINVAL	not modified
not NULL	too small	any	ERANGE	strDest[0] set to 0

例えば SelectDir の nMaxCount 引数にゼロを渡したら（ひどい利用方法）、その値が wcsncpy_s 関数の第2引数の numberOfElements に渡されるのでこの場合は EINVAL が返るんですかね？動作確認はしていませんが…。

なお空文字を設定するのに

wcscpy_s( strFolderName, nMaxCount, L"" );

を呼び出すのではなく strFolderName[0] = 0; でも良いのではないかと思ったんですが、strFolderName が nullptr の場合がありそうですね。なお wcscpy_s だと NULL を渡すとエラーハンドラが呼ばれるので wcsncpy_s にした方が良いと思います。

というか nullptr 引数のチェックは SelectDir 関数の冒頭で行うようにした方が良いのかもしれないですね。IFileDialog が働き損になるので。

beru

問題無いと思います。

PRの説明の影響範囲に列挙されていた箇所で、フォルダ選択ダイアログにVista以降のCommon Item Dialogが使われるようになった事の動作確認をしました。

SonarCloud が wcslen の呼び出しを Security Hotspot として検出していますが、実際に今のコードでどういうケースでどういう問題が起きるのか（起こせるのか）は分かりません。

自分が分かっている範囲では SelectDir 関数の引数に NULL を渡したら問題が起きると思いますが、そのように呼び出している箇所は無いと思います。

という事で、それについては対応してもしなくてもどちらでも良いとの判断で Approve します。

berryzplus

Security Hotspotが検出されないように修正を試みてほしいです。

Ocelot1210 · 2021-04-10T06:06:04Z

ご指摘ありがとうございます
wcslen が Security Hotspot として検出されるとは思わず、お手数をおかけして申し訳ございません...

以下対応いたしました

ご提示頂いた方法で Security Hotspot が検出されないように修正を試みた
SelectDir 関数冒頭で出力先バッファの nullptr チェックを追加

sonarcloud · 2021-04-10T06:15:00Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

AppVeyorBot · 2021-04-10T06:34:53Z

✅ Build sakura 1.0.3653 completed (commit 39b969ebcb by @Ocelot1210)

Security Hotspotの解消を確認したので。

berryzplus

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

beru

動作確認して特に問題は見つかりませんでした。

フォルダ選択ダイアログを SHBrowseForFolder() でなく IFileDialog を使用するように変更

58ceffe

SonarCloud指摘の対応

32e6f05

berryzplus previously approved these changes Mar 31, 2021

View reviewed changes

beru reviewed Mar 31, 2021

View reviewed changes

beru added the specification change ■仕様変更 label Mar 31, 2021

SelectDir() にテンプレート引数で固定長配列の要素数を取るオーバーロード関数を追加

5dd13bf

Ocelot1210 dismissed berryzplus’s stale review via 5dd13bf April 2, 2021 10:44

beru reviewed Apr 2, 2021

View reviewed changes

sakura_core/outline/CDlgFileTree.cpp Outdated Show resolved Hide resolved

ファイルの差分の原因を解消

127b8b4

berryzplus previously approved these changes Apr 4, 2021

View reviewed changes

Ocelot1210 added 2 commits April 4, 2021 21:21

コンフリクト解消

531b648

SetInitialDir() の引数を修正

5c5cdef

beru reviewed Apr 6, 2021

View reviewed changes

sakura_core/util/shell.cpp Outdated Show resolved Hide resolved

文字列長が MAX_PATH を超える場合、実行時エラーが発生するのを対応

1b6628e

beru reviewed Apr 7, 2021

View reviewed changes

sakura_core/util/shell.cpp Outdated Show resolved Hide resolved

格納先のバッファサイズが足りない場合、実行時エラーが発生するのを対応

54bc397

berryzplus requested changes Apr 9, 2021

View reviewed changes

beru previously approved these changes Apr 9, 2021

View reviewed changes

berryzplus previously requested changes Apr 10, 2021

View reviewed changes

Security Hotspot が検出されないように試みた

38e017a

Ocelot1210 dismissed beru’s stale review via 38e017a April 10, 2021 06:02

berryzplus approved these changes Apr 10, 2021

View reviewed changes

beru approved these changes Apr 10, 2021

View reviewed changes

beru merged commit ab89e26 into sakura-editor:master Apr 10, 2021

Ocelot1210 deleted the use_ifiledialog branch April 12, 2021 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

フォルダ選択ダイアログを SHBrowseForFolder() でなく IFileDialog を使用するように変更 #1609

フォルダ選択ダイアログを SHBrowseForFolder() でなく IFileDialog を使用するように変更 #1609

Ocelot1210 commented Mar 27, 2021

AppVeyorBot commented Mar 27, 2021

berryzplus commented Mar 27, 2021

Ocelot1210 commented Mar 27, 2021

AppVeyorBot commented Mar 27, 2021

berryzplus commented Mar 28, 2021

AppVeyorBot commented Mar 28, 2021

berryzplus left a comment

beru left a comment

Ocelot1210 commented Apr 2, 2021

AppVeyorBot commented Apr 2, 2021

AppVeyorBot commented Apr 3, 2021

berryzplus left a comment

berryzplus Apr 4, 2021

berryzplus Apr 4, 2021

berryzplus Apr 4, 2021

Ocelot1210 Apr 4, 2021

berryzplus Apr 4, 2021

berryzplus commented Apr 4, 2021 •

edited

AppVeyorBot commented Apr 4, 2021

AppVeyorBot commented Apr 7, 2021

AppVeyorBot commented Apr 8, 2021

berryzplus Apr 9, 2021

beru Apr 9, 2021

beru Apr 9, 2021 •

edited

berryzplus commented Apr 9, 2021

beru commented Apr 9, 2021 •

edited

beru left a comment •

edited

berryzplus left a comment

Ocelot1210 commented Apr 10, 2021

sonarcloud bot commented Apr 10, 2021 •

edited

AppVeyorBot commented Apr 10, 2021

berryzplus left a comment

beru left a comment

	static void SetInitialDir( Microsoft::WRL::ComPtr<IFileDialog> pDialog, const WCHAR* pszInitFolder )
	static void SetInitialDir( IFileDialog* pDialog, const WCHAR* pszInitFolder )

	if ( nMaxCount < wcslen(pszResult) ) {
	if ( pszResult != nullptr && nMaxCount < wcsnlen( pszResult, nMaxCount ) ) {

フォルダ選択ダイアログを SHBrowseForFolder() でなく IFileDialog を使用するように変更 #1609

フォルダ選択ダイアログを SHBrowseForFolder() でなく IFileDialog を使用するように変更 #1609

Conversation

Ocelot1210 commented Mar 27, 2021

PR の目的

カテゴリ

PR の背景

PR のメリット

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

仕様・動作説明

変更前

変更後

PR の影響範囲

テスト内容

テスト1

テスト2

関連 issue, PR

参考資料

AppVeyorBot commented Mar 27, 2021

berryzplus commented Mar 27, 2021

Ocelot1210 commented Mar 27, 2021

AppVeyorBot commented Mar 27, 2021

berryzplus commented Mar 28, 2021

AppVeyorBot commented Mar 28, 2021

berryzplus left a comment

Choose a reason for hiding this comment

beru left a comment

Choose a reason for hiding this comment

Ocelot1210 commented Apr 2, 2021

AppVeyorBot commented Apr 2, 2021

AppVeyorBot commented Apr 3, 2021

berryzplus left a comment

Choose a reason for hiding this comment

berryzplus Apr 4, 2021

Choose a reason for hiding this comment

berryzplus Apr 4, 2021

Choose a reason for hiding this comment

berryzplus Apr 4, 2021

Choose a reason for hiding this comment

Ocelot1210 Apr 4, 2021

Choose a reason for hiding this comment

berryzplus Apr 4, 2021

Choose a reason for hiding this comment

berryzplus commented Apr 4, 2021 • edited

AppVeyorBot commented Apr 4, 2021

AppVeyorBot commented Apr 7, 2021

AppVeyorBot commented Apr 8, 2021

berryzplus Apr 9, 2021

Choose a reason for hiding this comment

beru Apr 9, 2021

Choose a reason for hiding this comment

beru Apr 9, 2021 • edited

Choose a reason for hiding this comment

berryzplus commented Apr 9, 2021

beru commented Apr 9, 2021 • edited

beru left a comment • edited

Choose a reason for hiding this comment

berryzplus left a comment

Choose a reason for hiding this comment

Ocelot1210 commented Apr 10, 2021

sonarcloud bot commented Apr 10, 2021 • edited

AppVeyorBot commented Apr 10, 2021

berryzplus left a comment

Choose a reason for hiding this comment

beru left a comment

Choose a reason for hiding this comment

berryzplus commented Apr 4, 2021 •

edited

beru Apr 9, 2021 •

edited

beru commented Apr 9, 2021 •

edited

beru left a comment •

edited

sonarcloud bot commented Apr 10, 2021 •

edited