ListViewで複数選択してからの削除について

[戻る]
かれー 2010/05/25(火) 15:12:45
開発環境
Win XP Pro SP3
VC++ 2005 MFC

お世話になっております。
現在、リストビューに表示したファイル一覧から、
複数選択して削除できる機能を作成しています。

SHFileOperation のAPIを使用して一つずつ選択して
削除することは出来たのですが複数選択での削除が出来ず悩んでいます。

以下の二点を教えてください。

1)
http://www.crimson-systems.com/tips/t001r.htm
このサイトを参考に、ファイルパスの最後に'\1'を入れて
後から'\0'に置き換える・・・と言う手段をとっているのですが
_tcscat_s で結合したあとに pszFrom の中身を見ても'\1'が入っておらず
置き換えができません。何かやり方が悪いのでしょうか?

※
C:\a.txt\1C:\b.txt となってほしいのが
C:\a.txtC:\b.txt   となってしまいます。


2)
コンパイルは通っているのですが、ビルドしてこの関数を抜けると、
「Windows によって xxxx.exe でブレークポイントが発生しました〜」の
警告が出て終了してしまいます。
_tcscat_s を抜けば警告が出ずに上手く通るようになるのですが、
_tcscat_s 自体はブレークポイントでも止まらず、正常に動いてるように見えます。
何か記述が間違っているのでしょうか?



以下、作成したコードになります。


TCHAR* pTemp;
TCHAR pszFrom[] = _T("");

// 選択されているアイテムを取得
int index = m_List.GetNextItem(-1, LVNI_ALL | LVNI_SELECTED);
if(index == -1) return;

while (index != -1)
{
    // パスを取得する自作関数
    CString sFilePath = m_List.GetPathName(index);

    size_t len = sFilePath.GetLength() + 1;
    pTemp = new TCHAR[len];
    _tcscpy_s( pTemp, len, sFilePath );
    pTemp[sFilePath.GetLength() + 1] = '\1';

    size_t len2 = _tcslen(pszFrom) + _tcslen(pTemp) + 10;
    _tcscat_s(pszFrom, len2, pTemp);

    index = m_List.GetNextItem(index, LVNI_ALL | LVNI_SELECTED);
}

int nLen = (int)_tcslen(pszFrom);
for (int i=0; i<nLen; i++) {
    if ( pszFrom[i] == '\1' ) pszFrom[i]= '\0';
}

// 構造体設定
SHFILEOPSTRUCT FileOp;
::ZeroMemory(&FileOp, sizeof(SHFILEOPSTRUCT));
FileOp.fFlags = FOF_ALLOWUNDO;
FileOp.wFunc = FO_DELETE;
FileOp.pFrom = pszFrom;

// 削除処理
::SHFileOperation(&FileOp);

delete []pszFrom;

仲澤@失業者 2010/05/25(火) 15:53:33
1.pszFromの入れ物のサイズが0はまずいです。
  pTempと同じような方法をとるべきでしょう。

2.size_t len2 = _tcslen(pszFrom) + _tcslen(pTemp) + 10;
  って何を計算しているのかさっぱり分かりません。

3._tcscat_s(pszFrom, len2, pTemp);
  は常に先頭に追加ってことでしょうか。しかも、pTempの長さはlenしか
  ないのに(直前でpTemp = new TCHAR[len]; ってしましたよねぇ)len2を
  追加コピーしようとしてます。意味不明です。

4.CStringを使ってよいなら、メンバ関数で実装し、完了してから
  SHFILEOPSTRUCTにコピーしたほうがエレガントかも。
  その後01->00変換ですかね。

仲澤@失業者 2010/05/25(火) 15:57:30
ごめん。3.は勘違い、n付きかと思ったorz。

maru 2010/05/25(火) 16:07:00
> 何か記述が間違っているのでしょうか?
突っ込みどころが一杯あって楽しいコードですね。

> このサイトを参考に、ファイルパスの最後に'\1'を入れて
'\1'は文字では無いのに文字列処理用関数を使うはいいの?
まあここで使われている関数は'\0'で終端を判断しているから問題は無いだろうけど。

> TCHAR pszFrom[] = _T("");
これは固定の空文字列へのポインタになりますね。
このポインタに書込みを行ってはいけません。警告はこれが原因です。

>     size_t len = sFilePath.GetLength() + 1;
+1は終端文字用ですよね。まさか'\1'を入れるためではありませんよね。

>     pTemp[sFilePath.GetLength() + 1] = '\1';
確保した領域の一つ先に書いているようですが...

>     size_t len2 = _tcslen(pszFrom) + _tcslen(pTemp) + 10;
この10は何?

> delete []pszFrom;
newで確保した領域へのポインタではないのでdeleteって?

かれー 2010/05/25(火) 16:53:16
>仲澤@失業者さん
> 1.pszFromの入れ物のサイズが0はまずいです。
入れ物も動的にサイズを増やすということでしょうか?
whileで回しているため、最初が0ではないといけないのではないんでしょうか?

> 2.何を計算しているのかさっぱり分かりません。
結合文字列 + 追加したい文字列 + 少し余裕を持たせた10
と言うことでした。この分の場所を確保したかったのですが、
確保の仕方が間違っているみたいですね・・・

> 4.CStringを使ってよいなら、メンバ関数で実装し、完了してから
CString だと末端に\0をつける事が出来ず、
相性が悪いとのことで、使うのが難しいと思っています。


>maruさん
> '\1'は文字では無いのに文字列処理用関数を使うはいいの?
後から'\0'に変換して使っているので問題ないと思っていましたが・・・
そもそもこの文字を追加すること自体が間違っているのでしょうか?

> このポインタに書込みを行ってはいけません。警告はこれが原因です。
なるほど。上記でも書きましたが、動的にサイズをとる変数にすれば良いのでしょうか?

> +1は終端文字用ですよね。まさか'\1'を入れるためではありませんよね。
'\1'を入れるためにとっていました・・・

> 確保した領域の一つ先に書いているようですが...
sFilePath を pTemp に入れたことによって、
c:\a.txt → c:\a.txt\0
となり、sFilePath.GetLength() + 1 は
\0の部分を指していると思っていましたが違っているんでしょうか?

> この10は何?
少し余裕を持たせた幅をとろうと思っていたのですが・・・
コード上を見るだけだと意味がわからない記述ですね。

> newで確保した領域へのポインタではないのでdeleteって?
すいません、コピペミスです。delete []pTemp; の間違いでした。

maru 2010/05/25(火) 16:54:10
ちょっとした間違い修正。
> newで確保した領域へのポインタではないのでdeleteって?
newで確保した領域へのポインタではないのにdeleteって?

逆にpTempはdeleteされていない!

maru 2010/05/25(火) 17:10:17
> なるほど。上記でも書きましたが、動的にサイズをとる変数にすれば良いのでしょう
か?
その通りです。ただし、サイズはすべて文字列を調べないと分からないので、
一回目のループで文字列の長さを調べてから領域を確保し、2回目のループで
結合文字列を作成します。

> '\1'を入れるためにとっていました・・・
'\1'が入った後も文字列として扱うので、さらに終端コード分の+1が必要です。

> c:\a.txt → c:\a.txt\0
> となり、sFilePath.GetLength() + 1 は
> \0の部分を指していると思っていましたが違っているんでしょうか?
文字列ポインタを配列に読み変えて検証してみましょう。
sFilePath.GetLength() -> 8
pTemp[0] -> 'c'
pTemp[1] -> ':'
中略
pTemp[7] -> 't'
pTemp[8] -> '\0'
pTemp[sFilePath.GetLength() + 1] -> pTemp[9] 領域のひとつ先!間違い!
以下にしないと目的に動きにならない。
pTemp[sFilePath.GetLength()] = '\1';
pTemp[sFilePath.GetLength()+1] = '\0';

> すいません、コピペミスです。delete []pTemp; の間違いでした。
だとしてもforループ中でnewしてその外でdeleteしてはだめ!

maru 2010/05/25(火) 17:31:34
というか、複数の文字列がリストボックスにあるのであれば、'\1'->'\0'の
置き換えなんて必要ないじゃん。

// 選択されているアイテムを取得
int index = m_List.GetNextItem(-1, LVNI_ALL | LVNI_SELECTED);
if(index == -1) return;

size_t len = 0;
while (index != -1)
{
    len += m_List.GetPathName(index).GetLength() + 1;
    index = m_List.GetNextItem(index, LVNI_ALL | LVNI_SELECTED);
}
TCHAR* pBuffer = new TCHAR [len+1];
TCHAR* pTemp = pBuffer;
index = m_List.GetNextItem(-1, LVNI_ALL | LVNI_SELECTED);
while (index != -1)
{
    CString sFilePath = m_List.GetPathName(index);
    _tcscpy_s(pTemp, sFilePath);
    pTemp += sFilePath.GetLength();
    ++pTemp;    // 次書込み位置を終端の次へ移動
    index = m_List.GetNextItem(index, LVNI_ALL | LVNI_SELECTED);
}
*pTemp = '\0';    // 最後の2つ目の終端

// 構造体設定
SHFILEOPSTRUCT FileOp = {0};
FileOp.fFlags = FOF_ALLOWUNDO;
FileOp.wFunc = FO_DELETE;
FileOp.pFrom = pBuffer;

// 削除処理
::SHFileOperation(&FileOp);

delete [] pBuffer;

コンパイル/実行していないので細かな間違いがあるかも知れませんが。

かれー 2010/05/26(水) 10:51:46
[[解決]]
>maruさん
> 一回目のループで文字列の長さを調べてから領域を確保し、2回目のループで
> 結合文字列を作成します。
なるほど、ループを2回すると分かりやすいですね。
一度のループで全て詰め込もうと考えていたので、おかしな作りになっていました。
頭が固かったようです・・・

> 領域のひとつ先!間違い!
分かりやすい解説ありがとうございます。
配列の宣言ではないので、0から始まる事を忘れて勘違いしておりました。


> サンプルコード
わざわざご提示して頂いてありがとうございます。
_tcscpy_s(pTemp, sFilePath);
第二引数に結合先のサイズを指定しなければならないので、
_tcscpy_s(pTemp, sFilePath.GetLength() + 1, sFilePath);
としました。

あと、さりげなく変更されていますが
> SHFILEOPSTRUCT FileOp = {0};
この初期化方法、楽でいいですね。


ご教授頂いた内容をもとに作り変えて、上手く動かすことができました。
ありがとうございました!

maru 2010/05/27(木) 11:09:07
> 一度のループで全て詰め込もうと考えていたので、おかしな作りになっていました。
> 頭が固かったようです・・・
whileループが1->2と増えていますが、かれーさんの方は後に文字列をチェックする
forループがありますね。そのループ回数は全体の文字列長であり、最初のwhileループ
の回数よりずっと多いはずです。ループそのものの数より、そこで処理する回数に
ついて考える必要があります。
ループの数を減らそうと考えるのは良いことですが、その為に変な処理にしてしまう
より素直なコードを書いた方が良い場合もあります。最適化はコンパイラに任してし
まうというのも手です。

> > SHFILEOPSTRUCT FileOp = {0};
> この初期化方法、楽でいいですね。
そこもなんですが、_tcscat_sを使用していないことににも気づいていただけると
嬉しかったです。
strcatは連結元の文字列の先頭から終端文字列を探すため、一つの文字列に次々に
文字列を連結していくとだんだん遅くなってしまうんです(大した時間ではないで
しょうが)。かれーさんの元のコードだと、whileループの中で毎回文字列の先頭
を探す処理が行われてしまいます。ループの中で文字列の終端位置を把握していけ
ばstrcatを使用せず、strcpyだけで文字列の連結が出来ます。

最初に「最適化はコンパイラに任してしまうというのも手です。」と書きましたが、
上記のような処理の変更は現在のコンパイラでは不可能で、当分の間実現しないで
しょう。つまり人間が考える必要があります。
最適化も場合によってってことですね。

maru 2010/05/27(木) 11:43:44
細かな間違い修正。
> whileループの中で毎回文字列の先頭を探す処理が行われてしまいます。
「先頭を探す処理」->「先頭から終端を探す処理」

かれー 2010/05/28(金) 09:28:25
> strcatは連結元の文字列の先頭から終端文字列を探すため、一つの文字列に次々に
> 文字列を連結していくとだんだん遅くなってしまうんです
そういえば、以前VBAを学んでいた時、strcatのような関数を使って
処理時間が異常に遅くなってしまうと言う問題が発生して苦労した事がありました。
今回の場合も、大量にファイルを選択したりするとstrcatを使った場合は重くなってしま
いそうです。
strcatは出来る限り使わないほうが良さそうですね・・・

終端位置からstrcpyで連結していくという方法は思いつかなかったので、非常に参考にな
りました。
今後、似たような処理を行う場合でも応用で使うことができそうです。

古いスレッドには追加発言できません。