補完候補一覧ダイアログの不具合修正 #1750

ghost · 2022-01-03T04:16:16Z

PR の目的

#1749 で指摘された補完候補一覧ダイアログに関する2件の不具合を修正します。

補完候補一覧ダイアログがやけに小さく表示される
補完候補一覧ダイアログで選択した候補が入力されない

カテゴリ

不具合修正

PR の背景

PR のメリット

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

仕様・動作説明

前者の不具合について

補完候補一覧ダイアログの表示サイズは基底クラスから継承したメンバ変数に格納した値を利用しています。

sakura/sakura_core/CHokanMgr.cpp

Lines 241 to 244 in 0dd4391

    
           nX = m_poWin.x - m_nColumnWidth; 
        
           nY = m_poWin.y + m_nWinHeight + 4; 
        
           nCX = m_nWidth; 
        
           nCY = m_nHeight;

これらの変数の値は、モニターワークエリアに収まるように調整された上でAPI関数::MoveWindow()に渡されます。
このためにメンバ変数にダイアログサイズが記憶されていなければなりません。

以前はCHokanMgr::DoModeless()からCHokanMgr::OnSize()を通じて呼び出されるCDialog::OnSize()に当該処理が記述されていましたが、 #1711 で削除されたため、メンバ変数の値が基底クラスのコンストラクタで初期化されたままになっています。

幸い、CHokanMgr::DispatchEvent()に記述されたWM_GETMINMAXINFOメッセージに対する処理によって下限値が設定されているため、それより小さい表示になることはありませんでした。

対策として、CDialog::OnSize()から失われた当該処理をCHokanMgr::OnSize()に追加します。
なお、メンバ変数名と既存のローカル変数名が似ているうえ、それぞれスクリーン座標とクライアント座標で内容が異なることから、念のためローカル変数の名前を変更しました。

後者の不具合について

ApiWrap::List_GetText()にて、格納先であるバッファのサイズを取得した文字列が収まるように調整する際、ヌル終端を含んだ値が必要な所に含まれない値を渡していたため、文字列が格納できていませんでした。
渡している値はリストボックスのアイテムから文字列を取得するLB_GETTEXTメッセージの戻り値ですが、これは成功時にコピーできた文字列のヌル終端を含まない長さを返します。

対策として、ヌル終端を文字列の長さに加えたうえでサイズ調整を行うようにします。
また、単体テストを追加してList_GetTextの動作確認を実施しています。

PR の影響範囲

補完候補一覧ダイアログ (CHokanMgr)

テスト内容

手順

サクラエディタを起動し、次のように入力して改行しておきます。

STR_BACKUP_CONFORM_MSG1
STR_BACKUP_CONFORM_MSG2

「STR」と入力し、Ctrl + Spaceを押して候補を表示させます。
表示された候補一覧のサイズが小さすぎないか確認してください。
Enterで候補の選択を確定します。
選択した候補が入力されることを確認してください。

参考資料

ghost · 2022-01-03T04:21:10Z

補足：

長すぎる候補はダイアログに収まらず見切れてしまうようですが、以前からそうなっているようなのでスルーします。

AppVeyorBot · 2022-01-03T04:44:28Z

✅ Build sakura 1.0.3968 completed (commit b45144f8f2 by @kazasaku)

sakura_core/CHokanMgr.cpp

sakura_core/apiwrap/StdControl.cpp

beru · 2022-01-03T11:36:22Z

このPRとは全く関係無いんですが、久しぶりにブランチをpullしてローカルで sakura.sln をVS2019で開いてビルドしたら sakura_core/githash.h の記述が古くて（BUILD_ENV_NAME が定義されていない）エラーが出ました。

sakura/githash.targets の記述で sakura_core/githash.h ファイルが存在しなければ sakura/githash.bat を呼び出して生成するので問題は起きないんですが、古いファイルが残っている環境ではファイルをいったん削除しないと問題が解決しません。

いつもここらへんを気にしていなかったのでびっくりしました。

sonarqubecloud · 2022-01-03T14:28:50Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

12.5% Coverage
0.0% Duplication

AppVeyorBot · 2022-01-03T14:33:42Z

✅ Build sakura 1.0.3969 completed (commit 7a953bf713 by @kazasaku)

beru

#1749 で報告された問題が解消された事を確認しました。

berryzplus · 2022-01-03T14:40:05Z

sakura_core/CHokanMgr.cpp

-				nWidth - rc.left * 2,
-				nHeight - rc.top * 2/* - 20*/,
+				nClientWidth - rc.left * 2,
+				nClientHeight - rc.top * 2/* - 20*/,


「/* - 20*/」ってなんですかね？

「これって何ですか？食べられますか？」がムダな質問なら、
削ってしまうのが環境にやさしいと思います。（対応不要）

自分も不思議に思ったので調べてみました。そもそも * 2 しているのもどうしてだかよくわかりません。gitのlogを見ると、CHokanMgr::OnSize の処理内容は 2000/11/07 の genta さんのコミット b5555ab の時点で既にこうなっていました。

クラス名の CHokanMgr からこのクラスがダイアログだという事が想像しにくいですが、ダイアログリソース IDD_HOKAN をリソースエディタで見てみると、リストボックス IDC_LIST_WORDS が存在しました。

ここではforループで配列の要素数分回していますが、配列の要素数は固定で1要素なのでループにする意味は無いように思えます。将来的にダイアログのコントロールの数が増えた場合に対応がしやすいコードにしていたのかもしれないですね。

ループの中の処理は

GetWindowRect でウィンドウ領域のスクリーン座標を取得

ScreenToClient を2回呼び出して左上と右下の位置をクライアント座標に変換

SetWindowPos を呼んでリストボックスの位置をクライアント座標で設定し、サイズは親ダイアログのクライアント領域のサイズに設定

です。デバッグ実行して確認すると、リストボックスの左上のクライアント座標は 0,0 になっています。なので、ここで * 2 をする意味も無いように思えます。左上が 0,0 でコントロールの横幅と高さをクライアント領域の横幅と高さにするというのは、クライアント領域めいっぱいに広げるという事でしょうか。それが目的ならわざわざこういう記述にする必要は無い気もしますが、書いた人は何か理由があってこういう書き方にしたのかもしれないですね。

あと余談ですが + 4, - 3, - 4 している記述もあります。どういう目的での調整なのか自分には読み取れません。

しかし補完候補の一覧が機能しないというのは結構重大な問題ですね。自分が不具合を入れてしまいましたが、報告してくれたusagishitaさんと修正してくれたkazasakuさんのお陰で問題が解消されそうです。ありがとうございます。

berryzplus · 2022-01-03T14:59:28Z

sakura_core/CHokanMgr.cpp

-	nWidth = rcDlg.right - rcDlg.left;  // width of client area
-	nHeight = rcDlg.bottom - rcDlg.top; // height of client area
+	int nClientWidth = rcDlg.right - rcDlg.left;  // width of client area
+	int nClientHeight = rcDlg.bottom - rcDlg.top; // height of client area


WM_SIZEメッセージは本来、第2パラメータにMAKELPARAM(cx,cy)を受け取るので、
自前でGetClientRectを呼び出すのは無駄です。
https://docs.microsoft.com/en-us/windows/win32/winmsg/wm-size
ウインドウ矩形とクライアント矩形の両方を使って処理したい場合には、 WM_WINDOWPOSCHANGEDメッセージのハンドラを書くんですが・・・この話はPRの内容と関係ないのでスルーしてよいかと思います。

ここの処理は SetWindowSubclass で「表示位置を保存するウインドウのためのSubclassProc（≒WndProc）」を定義してあげると整理しやすくなるかもしれないです。いわゆるアスペクト指向っす。

berryzplus · 2022-01-03T15:16:57Z

sakura/githash.targets の記述で sakura_core/githash.h ファイルが存在しなければ sakura/githash.bat を呼び出して生成するので問題は起きないんですが、古いファイルが残っている環境ではファイルをいったん削除しないと問題が解決しません。

リビルドしたら消えるように修正したほうがいいのかもです。

beru · 2022-01-03T23:33:40Z

sakura/githash.targets の記述で sakura_core/githash.h ファイルが存在しなければ sakura/githash.bat を呼び出して生成するので問題は起きないんですが、古いファイルが残っている環境ではファイルをいったん削除しないと問題が解決しません。

リビルドしたら消えるように修正したほうがいいのかもです。

試しに sakura/githash.targets ファイルに下記の記述を追加してみたところ期待通りの動作をしました。

  <Target Name="DeleteGitHash"
      Condition="Exists('$(GeneratedGitHash)')"
      BeforeTargets="Clean">
    <Exec Command="del $(GeneratedGitHash)" />
  </Target>

ただこの指定が適切なのかどうかはMSBuildを知らないのでよくわかりません。

https://docs.microsoft.com/en-us/visualstudio/msbuild/how-to-extend-the-visual-studio-build-process?view=vs-2022

ghost · 2022-01-04T08:56:28Z

レビューありがとうございました。マージします。
何かありましたらメンションでお知らせください。

Kohki Akikaze added 4 commits January 2, 2022 10:22

ダイアログの位置とサイズを記憶する処理を補完候補ダイアログに限って復元する

8212d55

似通った変数名を変更

62d06ad

リストボックスからのテキスト取得処理で取りこぼしが生じているのを修正

c899648

ApiWrap::List_GetText()のテストを追加

978a9a5

ghost mentioned this pull request Jan 3, 2022

ダイアログの位置が表示するたびにずれていく問題を修正 #1711

Merged

ghost added the 🐛bug🦋 ■バグ修正(Something isn't working) label Jan 3, 2022

beru reviewed Jan 3, 2022

View reviewed changes

sakura_core/CHokanMgr.cpp Outdated Show resolved Hide resolved

beru reviewed Jan 3, 2022

View reviewed changes

sakura_core/apiwrap/StdControl.cpp Show resolved Hide resolved

タイプミスを修正

7508075

beru approved these changes Jan 3, 2022

View reviewed changes

berryzplus approved these changes Jan 3, 2022

View reviewed changes

ghost merged commit 42568b8 into sakura-editor:master Jan 4, 2022

ghost deleted the feature/fix_word_completion branch January 4, 2022 08:57

berryzplus mentioned this pull request Jan 5, 2022

リビルド時、githash.hが再生成されるように修正 #1757

Closed

berryzplus mentioned this pull request Jun 18, 2023

リビルド時、githash.hが再生成されるように修正 #1923

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

補完候補一覧ダイアログの不具合修正 #1750

補完候補一覧ダイアログの不具合修正 #1750

ghost commented Jan 3, 2022

ghost commented Jan 3, 2022

AppVeyorBot commented Jan 3, 2022

beru commented Jan 3, 2022

sonarqubecloud bot commented Jan 3, 2022

AppVeyorBot commented Jan 3, 2022

beru left a comment

berryzplus Jan 3, 2022

beru Jan 3, 2022

berryzplus Jan 3, 2022

berryzplus commented Jan 3, 2022

beru commented Jan 3, 2022

ghost commented Jan 4, 2022

	nX = m_poWin.x - m_nColumnWidth;
	nY = m_poWin.y + m_nWinHeight + 4;
	nCX = m_nWidth;
	nCY = m_nHeight;

補完候補一覧ダイアログの不具合修正 #1750

補完候補一覧ダイアログの不具合修正 #1750

Conversation

ghost commented Jan 3, 2022

PR の目的

カテゴリ

PR の背景

PR のメリット

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

仕様・動作説明

前者の不具合について

後者の不具合について

PR の影響範囲

テスト内容

関連 issue, PR

参考資料

ghost commented Jan 3, 2022

AppVeyorBot commented Jan 3, 2022

beru commented Jan 3, 2022

sonarqubecloud bot commented Jan 3, 2022

AppVeyorBot commented Jan 3, 2022

beru left a comment

Choose a reason for hiding this comment

berryzplus Jan 3, 2022

Choose a reason for hiding this comment

beru Jan 3, 2022

Choose a reason for hiding this comment

berryzplus Jan 3, 2022

Choose a reason for hiding this comment

berryzplus commented Jan 3, 2022

beru commented Jan 3, 2022

ghost commented Jan 4, 2022