Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

キーボードの文字の入力の表示の間隔設定をミリ秒に変換してからタイマーの間隔に使用するように修正 #523

Merged
merged 1 commit into from
Oct 7, 2018

Conversation

beru
Copy link
Contributor

@beru beru commented Oct 6, 2018

#506 で報告した内容に対応する PR です。

SystemParametersInfo 関数に SPI_GETKEYBOARDSPEED を指定してキーボードのリピート速度の設定を取得後にそれをミリ秒に単位変換せずに SetTimer に渡している記述があるので、それへの対処です。

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTMです。

DWORD dwKeyBoardSpeed;
SystemParametersInfo( SPI_GETKEYBOARDSPEED, 0, &dwKeyBoardSpeed, 0 );
/* リピート速度の設定をミリ秒に変換 */
UINT uElapse = 400 - dwKeyBoardSpeed * (400 - 33) / 31;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

windows SDKの変数定義がおかしいのは知ってますが
変数には意味のある名前を付けるべきですよね。
https://ejje.weblio.jp/content/elapsed+time

uTimeoutとかuKeybordSpanとかが代案。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows SDKのその記述に問題があるかの判断は法曹界に委ねようかと思います。
もし記述が将来更新されたらそれに合わせる事にします。

代案のなかでは uTimeout が個人的に分かりやすいです。趣は違いますが uMilliseconds とかでも良いと思います。uInterval でも uHoge でもなんでもコンパイラがエラーを出さなければ…。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

400 >= dwKeyBoardSpeed * (400 - 33) / 31 であることは保証されるのですか?

Copy link
Contributor Author

@beru beru Oct 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SystemParametersInfo の呼び出しが失敗しなければ保証されます。

SystemParametersInfo( SPI_GETKEYBOARDSPEED, 0, &dwKeyBoardSpeed, 0 );

呼び出し後の dwKeyBoardSpeed の値の範囲は 0 ~ 31 です。

https://docs.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-systemparametersinfow
に書かれている記述を引用します。

Retrieves the keyboard repeat-speed setting, which is a value in the range from 0 (approximately 2.5 repetitions per second) through 31 (approximately 30 repetitions per second). The actual repeat rates are hardware-dependent and may vary from a linear scale by as much as 20%. The pvParam parameter must point to a DWORD variable that receives the setting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SystemParametersInfo の呼び出しが失敗しなければ保証されます。

成功することが保証されないので、SystemParametersInfo の戻り値チェックした方がいいです。
あと dwKeyBoardSpeed に対して 31 以下であることの assert を入れた方がいいです。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あと SystemParametersInfo が失敗した場合の uElapse のデフォルト値も決めておいた方がいいです。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

おっしゃる通りだと思います。しかし元のコードには SystemParametersInfo の呼び出しが失敗した場合の対処は入っていないので must かというとそうでもないと思います。

失敗した場合にデバッグビルドでは GetLastError 呼んで FormatMessage 呼んで OutputDebugStringW (DEBUG_TRACE 経由)呼んで、とかの対応まで求められないのであれば、こちらで追加の PR を作成して対処します。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

というか、

失敗した場合にデバッグビルドでは GetLastError 呼んで FormatMessage 呼んで OutputDebugStringW (DEBUG_TRACE 経由)呼んで

のマクロがあると良いですね…。。探した感じ既存では無さそうです。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

おっしゃる通りだと思います。しかし元のコードには SystemParametersInfo の呼び出しが失敗した場合の対処は入っていないので must かというとそうでもないと思います。

もともとのコードは nKeyBoardSpeed を初期化してないので、失敗したときの
nKeyBoardSpeed の値は不定になるので潜在バグですね。

失敗した場合にデバッグビルドでは GetLastError 呼んで FormatMessage 呼んで OutputDebugStringW (DEBUG_TRACE 経由)呼んで、とかの対応まで求められないのであれば、こちらで追加の PR を作成して対処します。

失敗したときに、dwKeyBoardSpeed あるいは uElapse の妥当なデフォルト値を
設定するようにしとけばいいと思います。

失敗した原因をわかるようにするより、失敗したときに予期しない動作を
しないようにすればいいと思います。

PR お願いします。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#527 を作成しました。

@beru
Copy link
Contributor Author

beru commented Oct 7, 2018

Merge します。もし問題が見つかったら別の PR で修正する事にしましょう。

@beru beru merged commit 463a5b1 into sakura-editor:master Oct 7, 2018
@m-tmatma m-tmatma added this to the next release milestone Oct 21, 2018
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
キーボードの文字の入力の表示の間隔設定をミリ秒に変換してからタイマーの間隔に使用するように修正
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants