Window テキストをCNativeT で取得/設定するユーティリティ関数を追加 #776

m-tmatma · 2019-01-20T13:16:23Z

Window テキストをCNativeT で取得/設定するユーティリティ関数を追加

段取り

~~CNative::Clear() → CNative::shrink_to_empty() に名前変更する #778 (説明欄を参照) をマージをする~~ (CNative::Clear() → CNative::shrink_to_empty() に名前変更する #778 (comment))
CNative::Clear の実装を改善 #777 を rebase してマージする (完了)
Window テキストをCNativeT で取得/設定するユーティリティ関数を追加 #776 を rebase してマージする (この PR)

berryzplus · 2019-01-20T13:20:45Z

目的に反対します。

ds14050 · 2019-01-20T13:37:15Z

お互いに何が嬉しいのか、どこに差し障りがあるのか、説明するつもりはないのですか。

berryzplus · 2019-01-20T13:39:44Z

お互いに何が嬉しいのか、どこに差し障りがあるのか、説明するつもりはないのですか。

もっともな意見ですね。
しかし、何が嬉しいかを説明する責任はまずPRを出した人にあると思ってます。
こちらとしては何が嬉しいかを予測した上で「やー」と駄々をこねております。

m-tmatma · 2019-01-20T21:07:47Z

以下のようにメモリを確保してタイトルを取得するようなコードがありますが、
CNativeT の変数を確保したら、関数呼び出し一つで取得できて楽だからです。

	const ULONG cchText = ::GetWindowTextLength( hWnd );
	auto textBuf = std::make_unique<WCHAR[]>( cchText + 1 );
	WCHAR* pchText = textBuf.get();
	::GetWindowText( hWnd, pchText, cchText + 1 );

berryzplus · 2019-01-21T00:08:26Z

しまった。変な状態で送信してしまった...orz

一応、Java でいう @Deprecated な関数として新設するなら賛成します。
リファクタリングを行うための一手としてアリな手法だと思います。
new/delete や vector.resize() などを使った意図を読み取りにくいコードをどう片すかを問題ととらえるなら「アリ」、CNativeTを使ったコーディングを便利にするためにと考えるなら「ナシ」です。

CNativeTはアロケータなのかホルダーなのかコンバータなのか役割があいまいです。
この型を使うことでコードの解読難易度が上がると思っています。

m-tmatma · 2019-01-21T11:26:54Z

この型を使うことでコードの解読難易度が上がると思っています。

それはなぜですか?
使う側では特に内部は意識する必要はないと思います。

berryzplus · 2019-01-21T12:29:09Z

使う側では特に内部は意識する必要はないと思います。

何でもできるクラスを使うと、外見から何をしたいコードなのか分かりにくくなります。何でもできる≒何がしたいか読み取りづらい、です。

std::wstringには文字コード変換の機能がありせんよね？変換する時は専用のクラスが出てきます。あー変換しとるねと分かります。

std::wstringには文字列置換の機能がありませんよね？文字列を置換する時は専用のクラスが出てきます。あー置換しとるねと分かります。

CNativeTの場合、getter / setterが文字コード変換機能を兼ねています。Replaceメソッドが文字列置換を実行します。最近ではフォーマット機能も追加されました。

何がしたいのか把握するためにはCNativeTに精通する必要が出てきます。解読のためにCNativeTを知らないといけないとしたら、それは読み解くのを難しくしてるんだと思います。

m-tmatma · 2019-01-21T12:37:59Z

何でもできるクラスを使うと、外見から何をしたいコードなのか分かりにくくなります。何でもできる≒何がしたいか読み取りづらい、です。

具体にどうわかりにくいのですか? それは単に一般論ですか?
今回のコードに関する限り、それを使っているだけです。

何がしたいのか把握するためにはCNativeTに精通する必要が出てきます。解読のためにCNativeTを知らないといけないとしたら、それは読み解くのを難しくしてるんだと思います。

なんか、CNativeT　が出てきたら条件反射的に反対しているように思います。
具体的にどういう点で精通しないといけないか説明していただけますか?

berryzplus · 2019-01-21T13:51:57Z

何でもできるクラスを使うと、外見から何をしたいコードなのか分かりにくくなります。何でもできる≒何がしたいか読み取りづらい、です。

具体にどうわかりにくいのですか? それは単に一般論ですか?
今回のコードに関する限り、それを使っているだけです。

何がしたいのか把握するためにはCNativeTに精通する必要が出てきます。解読のためにCNativeTを知らないといけないとしたら、それは読み解くのを難しくしてるんだと思います。

なんか、CNativeT　が出てきたら条件反射的に反対しているように思います。
具体的にどういう点で精通しないといけないか説明していただけますか?

やーです。メリットもデメリットもほぼ全部自分で説明してしまいました。
メリットを語れるってのがどういうことか、ご理解はいただけると思います。
とりあえず、本件について @m_tmatma さんからの説明を聞けなかったことには困っています。

今回PRで入れる新規関数群を今後どうしていくかについて、ビジョンはありますか？

最初に「目的に反対します」と書いた理由は、先について何も考えてなさそうな雰囲気を感じたからです。「こういう風にしていきたいから必要なんだ」という話ができるなら反対は撤回します。

「なんもねっす」と言われたら close しちゃうかも知れませんが。

m-tmatma · 2019-01-22T10:13:41Z

ウィンドウタイトルを取得する際あるいは、設定すら際に、いちいちバッファを用意しなくても良くなります。

また、ウィンドウタイトルを取得した後、加工してウィンドウタイトルを再設定する際に、cnativet の機能を利用して設定することができます。

使う側の観点からすると、内部構造は全く気にする必要はないはずです。

データを加工する際の使い方と生データにアクセスする方法さえ知っていたらいいはずです。

なんで精通する必要があるのですか？

beru

apiwrap/StdControl.h ファイルは、あくまで単純なWindowsAPIのラッパーに留めておいてユーザー定義型への依存は混ぜない方が良いかなと少し思います。あくまで私見なので「絶対に混ぜてほしくない、混ぜるな危険！！うぎゃあああ」とまでは言いませんが…。

ユーティリティクラス的な物、つまりユーティリティ関数を用意する事自体には反対しません（OOP原理主義者の哲学にはそぐわないとしても）。関数の数が5桁とかになって内容が重複したのばっかりになったら嫌な汗をかくことになりそうですが多分そこに行く前にリファクタは入るでしょう。。きっと。。

immutable な文字列型を用意した方が良いんじゃないかとかまぁ有りますが、バッファ管理が表面化したCっぽい香ばしいコードを許容するのもC++では有りでは無いかと思います。

beru · 2019-01-22T19:15:12Z

使う側では特に内部は意識する必要はないと思います。

何でもできるクラスを使うと、外見から何をしたいコードなのか分かりにくくなります。何でもできる≒何がしたいか読み取りづらい、です。

std::wstringには文字コード変換の機能がありせんよね？変換する時は専用のクラスが出てきます。あー変換しとるねと分かります。

std::wstringには文字列置換の機能がありませんよね？文字列を置換する時は専用のクラスが出てきます。あー置換しとるねと分かります。

CNativeTの場合、getter / setterが文字コード変換機能を兼ねています。Replaceメソッドが文字列置換を実行します。最近ではフォーマット機能も追加されました。

何がしたいのか把握するためにはCNativeTに精通する必要が出てきます。解読のためにCNativeTを知らないといけないとしたら、それは読み解くのを難しくしてるんだと思います。

berryzplus さんのこのコメントは説得力がありますね。CNativeW をこれ以上肥大化させたくないって事ですね。ただこのPRって CNativeW のメソッドを追加するわけではなくてユーティリティ関数を追加するという事なので、その新規に追加した関数が色々な箇所で使えてコードがすっきりするのであれば価値は有ると思います。ただ惜しむらくは関数が追加されただけで実使用しているわけじゃないんですよね…。

なおOOP原理主義者はユーティリティクラスを禁止したがるようです。
https://www.arksystems.co.jp/closeupit/object_oriented/0401.html
https://www.kaitoy.xyz/2016/01/03/oop-alternative-to-utility-classes/

が…、そもそも現状のサクラエディタのコード的にその考え方はマッチしない気がします。あとC++でそれを追い求めるのはヘルメット付けたままコンビニに入って食事をするようなものではないかと…。

berryzplus

目的に納得がいったのでレビューに入ります。

~~まず1点、こっちのレビューを優先でお願いしたいです（着順優先で。~~
5b1be0a

2点目、いつもの「プチ文句」で済まされない雰囲気の問題を見付けたので、調査＆対応をお願いしたいです。（Clear()メソッドの件です。

sakura_core/apiwrap/StdControl.h

berryzplus · 2019-01-23T16:08:47Z

sakura_core/apiwrap/StdControl.h

+ */
+ inline BOOL Wnd_SetText(HWND hwnd, const CNativeT& str)
+ {
+ return SetWindowText(hwnd, str.GetStringPtr());


このコメントは修正要求じゃないです。

CNativeW::operator LPCWSTR( void ) const を追加する選択もあると思います。

この関数の追加と両立できないわけではありませんが、変換演算子を定義すれば他の用途でも GetStringPtr() しなくて済むようになります。

暗黙の型変換演算子でしょうか？それとも explicit を付けた明示的な型変換演算子でしょうか？
std::basic_string も c_str メソッド経由じゃないと内部データのアドレス取れないようにしてるのでカプセル化を考えたら暗黙にはしない方が良いと思います。そんなの関係ねぇと言ってローレベルな記述をして処理効率を上げるのもそれはそれで乙なものだと思いますが…。

意図してるのは暗黙のほうです。
シグニチャ通り、constな内部ポインタを返すconst暗黙変換演算子があると便利かな、です。

マズい場合もありえることを初めて知りました...orz

この提案は完全にNGでした。

むしろ、既存コードにあるimplicit なwchar_t*変換演算子をexplicitに変えた方がいいくらいでした。

X64対応の障害になっちゃうから:smile:

昔懐かし CString だと暗黙的な型変換があってお手軽だった気がします。
しかし暗黙的な型変換は不具合に繋がりやすいのか何やら敵視されているっぽいです。
面倒くさいからといってバケツでウラン溶液を扱ってはいけないという事でしょうか？

sakura_core/apiwrap/StdControl.h

m-tmatma · 2019-01-24T11:45:58Z

まず1点、こっちのレビューを優先でお願いしたいです（着順優先で。
5b1be0a

これは、#769 の一部だと思いますが、別 PR にするのがいいと思います。

berryzplus · 2019-01-25T16:01:31Z

これは、#769 の一部だと思いますが、別 PR にするのがいいと思います。

了解です。関数追加と適用を別 PR にするメリットとして、修正済みコードへの影響を気にせずに関数仕様を検討できる、ということを挙げられると思います。使い始める前に関数の中身をちゃんと考えておく機会を作る意味で有用だと思うのでそうすることにします。（レビューもそのつもりで見直します

sakura_core/apiwrap/StdControl.h

berryzplus · 2019-02-02T12:09:08Z

sakura_core/apiwrap/StdControl.h

 }
- return ret;
+ return actualCount;


何故戻り値boolがよいかというと、文字列長では成功・失敗を判別できないからです。

テキストボックスやコンボボックスの入力値を取るケースでは、未入力があり得ます。
CNativeTを出力変数とするならlengthはCNativeTに埋め込まれるので、成功・失敗の情報が欲しいんです。

1点目、着順優先の件は破棄。2点目Clear()の件は勘違いだったため。

berryzplus

if文の分岐条件で少し気になることがありました。
好みの問題かも知れないので意見だけコメントします。

sakura_core/apiwrap/StdControl.h

berryzplus · 2019-02-03T13:57:05Z

ふーむ。

MinGW-w64のデバッグ版だけテストが失敗。
https://ci.appveyor.com/project/sakuraeditor/sakura/builds/22084197

結果表示を見る感じ、環境依存でうまくいかない、ではなくて、~~なんか違うものをリンクして~~おかしな結果を吐くプログラムにしてしまってる雰囲気を感じています。

MinGW-w64のテスト結果は当面スルーでよいと思ってますが、なんとかしたいです。
もちろん、このPRとは関係ありませんけどｗ

ds14050 · 2019-02-03T14:48:31Z

MinGW-w64のデバッグ版だけテストが失敗。

ローカルでも fixedPatternLen の値は違いますが再現しました。

変数の並びを以下のようにするとテストにパスしましたが、理由はわかりません。

	CNativeW stringW;
	constexpr const WCHAR*	fixedPatternStr = L"abc";
	constexpr const int		fixedPatternLen = 3;

ds14050 · 2019-02-03T14:57:10Z

これもパスしました。

	constexpr const WCHAR*	fixedPatternStr = L"abc";
	constexpr const int		fixedPatternLen = 3;
	int buffer[2];
	CNativeW stringW;

m-tmatma · 2019-02-03T22:02:36Z

ふーむ。

MinGW-w64のデバッグ版だけテストが失敗。
https://ci.appveyor.com/project/sakuraeditor/sakura/builds/22084197

結果表示を見る感じ、環境依存でうまくいかない、ではなくて、なんか違うものをリンクしておかしな結果を吐くプログラムにしてしまってる雰囲気を感じています。

MinGW-w64のテスト結果は当面スルーでよいと思ってますが、なんとかしたいです。
もちろん、このPRとは関係ありませんけどｗ

別件ですが、テストに失敗しても appeyor が失敗しないのは問題なので調べます。

ds14050 · 2019-02-03T23:52:29Z

これが PR #780 の件だったのかと(一晩経って)思いましたが、ちがうのかな？

berryzplus · 2019-02-04T00:48:13Z

これが PR #780 の件だったのかと(一晩経って)思いましたが、ちがうのかな？

違います。対処したい問題が別にあってそれは直りました。テストが通らない事象は想定外で、見落としです。

@ds14050 さんの指摘通り、宣言を触るとテスト通るようになるみたいです。
https://ci.appveyor.com/project/berryzplus/sakura/builds/22094975/job/ho49ao95lvfme0n4

定義した定数値がコーティングと異なっていた不思議現象(=MinGW-w64-g++のバグ?)なので、深入りせず対症療法しとくのがいいかなと思います。

ds14050 · 2019-02-04T10:30:57Z

これが PR #780 の件だったのかと(一晩経って)思いましたが、ちがうのかな？

違います。

たしかに違うみたいです。ところで、これもテストが通るようになるパターンです。

	constexpr static const WCHAR*	fixedPatternStr = L"abc";
	constexpr static const int		fixedPatternLen = 3;
	
	CNativeW stringW;

CNativeW が自身の置かれたメモリ領域から外れて int １つ分以上先まで書き込むことでスタックを破壊していると疑っています。

ds14050 · 2019-02-04T13:19:09Z

おそらく

*_DEBUG が未定義で評価されたヘッダ

_DEBUG が定義されてコンパイルされた .o ファイル

の組み合わせが原因だと思われます。原因のファイルは tests/unittests/CMakeLists.txt で、_DEBUG の定義を漏らしたのは私です。

言い訳

_DEBUG は _ で始まっていることからわかるように、処理系(VC++)固有の定義なんですね。だから CMake で Debug 版の makefile をビルドしても g++ に向けては _DEBUG を定義してくれない。サクラエディタでは Debug/Release の場合分けに _DEBUG を利用しているから、g++ でコンパイルする場合でも _DEBUG を定義しなければいけません。

コンパイル時に _DEBUG を定義しているのは sakura_core/Makefile ではなく build-gnu.bat です。見落としました。

わかってたはずなんですけどね……>https://github.com/ds14050/sakura-clone/blob/1fb806d1847e9bd6e68d97d2765a5e397d0383a9/CMakeLists.txt#L42-L43

ds14050 · 2019-02-04T13:27:20Z

修正するならこんな感じです。>master...ds14050:fix_mingwtest_compileoption

m-tmatma · 2019-02-04T14:04:02Z

別件ですが、テストに失敗しても appeyor が失敗しないのは問題なので調べます。

#781 で登録しました。

m-tmatma · 2019-02-04T14:13:14Z

MinGW-w64のデバッグ版だけテストが失敗。
https://ci.appveyor.com/project/sakuraeditor/sakura/builds/22084197

#782 で登録しました。

berryzplus · 2019-02-09T07:49:43Z

このPRのステータスはどうなっています？

ぼくにとっては、最後の指摘「if文の構成分かりづらくね？」に対するレス待ちです。

修正も、これで行きたいんだ、という意思表示もないので放置してます。

berryzplus

LGTMです。

tests/unittests/test-StdControl.cpp

berryzplus

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

…wText-CNativeT Window テキストをCNativeT で取得/設定するユーティリティ関数を追加

beru reviewed Jan 22, 2019

View reviewed changes

berryzplus previously requested changes Jan 23, 2019

View reviewed changes

m-tmatma mentioned this pull request Jan 24, 2019

CNative::Clear の実装を改善 #777

Merged

berryzplus reviewed Jan 25, 2019

View reviewed changes

sakura_core/apiwrap/StdControl.h Outdated Show resolved Hide resolved

m-tmatma mentioned this pull request Jan 26, 2019

CNative::Clear() → CNative::shrink_to_empty() に名前変更する #778

Closed

berryzplus reviewed Feb 2, 2019

View reviewed changes

sakura_core/apiwrap/StdControl.h Show resolved Hide resolved

berryzplus reviewed Feb 2, 2019

View reviewed changes

m-tmatma force-pushed the feature/util-WindowText-CNativeT branch 2 times, most recently from 2b8ebee to 7dd913b Compare February 3, 2019 05:32

berryzplus reviewed Feb 3, 2019

View reviewed changes

sakura_core/apiwrap/StdControl.h Show resolved Hide resolved

sakura_core/apiwrap/StdControl.h Outdated Show resolved Hide resolved

sakura_core/apiwrap/StdControl.h Show resolved Hide resolved

m-tmatma mentioned this pull request Feb 4, 2019

googletest でテストに失敗してもビルド結果が失敗にならない #781

Closed

m-tmatma mentioned this pull request Feb 4, 2019

MinGW の Debug で CNativeW.Clear のテストに失敗する #782

Closed

m-tmatma added this to the next release milestone Feb 5, 2019

m-tmatma added 7 commits February 9, 2019 17:42

Window テキストをCNativeT で取得/設定するユーティリティ関数を追加

19ab73a

インクルードを追加

323edd4

コメント追加

aabf31f

GetWindowText() の戻り値の仕様を間違っていたのを修正

97e4b7d

コンパイルに必要なヘッダを追加

f12eed5

エラーチェックを修正

4b21935

Wnd_GetText のテストを追加

9585343

m-tmatma force-pushed the feature/util-WindowText-CNativeT branch from 7dd913b to 9585343 Compare February 9, 2019 08:42

m-tmatma added 2 commits February 9, 2019 18:21

条件式を逆にする

12594f2

コメント修正

ed9e1fb

berryzplus previously approved these changes Feb 9, 2019

View reviewed changes

tests/unittests/test-StdControl.cpp Outdated Show resolved Hide resolved

ASSERT_FALSE を使う

195add5

m-tmatma dismissed berryzplus’s stale review via 195add5 February 9, 2019 21:57

berryzplus approved these changes Feb 10, 2019

View reviewed changes

m-tmatma merged commit 78f04c3 into sakura-editor:master Feb 10, 2019

m-tmatma added the enhancement ■機能追加 label Feb 10, 2019

HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019

Merge pull request sakura-editor#776 from m-tmatma/feature/util-Windo…

d6c6596

…wText-CNativeT Window テキストをCNativeT で取得/設定するユーティリティ関数を追加

m-tmatma deleted the feature/util-WindowText-CNativeT branch July 5, 2019 21:54

Window テキストをCNativeT で取得/設定するユーティリティ関数を追加 #776

Window テキストをCNativeT で取得/設定するユーティリティ関数を追加 #776

Conversation

m-tmatma commented Jan 20, 2019 • edited Loading

段取り

berryzplus commented Jan 20, 2019

ds14050 commented Jan 20, 2019

berryzplus commented Jan 20, 2019

m-tmatma commented Jan 20, 2019

berryzplus commented Jan 21, 2019

m-tmatma commented Jan 21, 2019

berryzplus commented Jan 21, 2019

m-tmatma commented Jan 21, 2019

berryzplus commented Jan 21, 2019

m-tmatma commented Jan 22, 2019

beru left a comment

Choose a reason for hiding this comment

beru commented Jan 22, 2019

berryzplus left a comment • edited Loading

Choose a reason for hiding this comment

berryzplus Jan 23, 2019

Choose a reason for hiding this comment

beru Jan 23, 2019

Choose a reason for hiding this comment

berryzplus Jan 23, 2019

Choose a reason for hiding this comment

berryzplus Feb 12, 2019

Choose a reason for hiding this comment

beru Feb 12, 2019

Choose a reason for hiding this comment

m-tmatma commented Jan 24, 2019

berryzplus commented Jan 25, 2019

berryzplus Feb 2, 2019

Choose a reason for hiding this comment

berryzplus left a comment

Choose a reason for hiding this comment

berryzplus commented Feb 3, 2019 • edited Loading

ds14050 commented Feb 3, 2019

ds14050 commented Feb 3, 2019

m-tmatma commented Feb 3, 2019

ds14050 commented Feb 3, 2019

berryzplus commented Feb 4, 2019 • edited Loading

ds14050 commented Feb 4, 2019

ds14050 commented Feb 4, 2019

言い訳

ds14050 commented Feb 4, 2019

m-tmatma commented Feb 4, 2019

m-tmatma commented Feb 4, 2019

berryzplus commented Feb 9, 2019

berryzplus left a comment

Choose a reason for hiding this comment

berryzplus left a comment

Choose a reason for hiding this comment

m-tmatma commented Jan 20, 2019 •

edited

Loading

berryzplus left a comment •

edited

Loading

berryzplus commented Feb 3, 2019 •

edited

Loading

berryzplus commented Feb 4, 2019 •

edited

Loading