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

マウスドラッグ時のスクロール速度を制限する #1853

Merged
merged 4 commits into from
Jul 7, 2022

Conversation

dep5
Copy link
Contributor

@dep5 dep5 commented Jun 11, 2022

注:このPRでの「スクロール」とは、「ステータスバーを表示中」に
ドラッグ & ドロップ編集
マウスによる範囲選択
ここに書かれているドラッグ操作を行った時に起きるスクロールのことを指します。

このPRでは編集部分の「上端」および「下端から2~3行上」にあるマウスドラッグ中に越えるとスクロールが始まるラインのことを「スクロールライン」と呼びます

PR の目的

カテゴリ

  • 仕様変更
  • プログラムの動作上の問題
    • ローカルビルド版

PR の背景

 最近のmasterでステータスバーを表示していると、v2.4.1に比べスクロールがなめらかになっています。
 スクロールラインを越えた位置でマウスを静止しても、スクロールが続き、減速しなくなっています。快適なスクロールになりました。
 編集画面の行数が20行以上あると、スクロールしすぎてもスクロールライン内にマウスを戻してスクロールを停止し、選択の終点を選ぶことができます。
 しかし、慣れないマウスだったり、ウィンドウを小さくする・フォントを大きくするなど10行以内しか表示していない時はスクロールが進みすぎてしまうことも多く、スクロールがちょっと速いように思います。

PR のメリット

  • スクロールの時、微調整がしやすくなります
  • Windows 8.1でのスクロールが操作可能な速度になります。

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

  • 広いウィンドウで使用している人はスクロールを遅く感じるかもしれません。
  • スクロールラインから遠く離すと(速度制限した行を越えると)元のスピードに戻るので、スピード差が気になるかもしれません。
    (長い文書の先頭・最後に移動するのは速いですが)
  • 「ステータスバーを非表示」の場合についてテスト不足です

仕様・動作説明

スクロールラインを越えた数行に限り、スクロールスピードを制限します。

#1714 (comment)
beru@f1f0849
beruさんが書いてくださったコードをもとに条件などを追加しています。

beruさんのコードからの変更点と、変更可能な値(必要個所の抜粋)

	/sakura_core/view/CCaret.cpp
1		if( abs( (Int)nScrollRowNum ) < 7 &&
2			( m_pEditView->GetSelectionInfo().IsMouseSelecting() || m_pEditView->m_bDragMode )){
3			DWORD dwPerTiming = 80;
4			if( nScrollRowNum > 0 && m_pEditView->m_bDragMode) {
5				nScrollRowNum = 0;
	/sakura_core/view/CEditView_Mouse.cpp
6			GetCaret().MoveCursorToClientPoint( ptMouse, false, &ptNewCursor );

5-- スクロール速度を落とせるようになります。
6-- 5の影響で通常の範囲選択でスクロールがストップするので「test(カーソル移動はしない)」をfalseに変更
2-- 矢印キーのキーリピートで不正な描画になるので、マウス選択中・ドラッグアンドドロップ中に限定
4-- ドラッグアンドドロップでは上側のスクロール可能な行がが1行分しかなく増速が見込めないので速度を速めにしています

1-- 数値を変更すると速度制限をするスクロールラインからの行数を変更できます
3-- 数値を増やすと速度制限中の行のスクロール速度が遅くなります

v2.4.1に比べ
#1714 (comment)

ステータスバーの描画を抑制すると WM_MOUSEMOVE メッセージが呼ばれる回数が増えるようです。
CEditView::OnMOUSEMOVE メソッド内で呼び出す CCaret::MoveCursor メソッドで縦スクロールする処理が頻繁に呼び出されるようになります。

なぜ速度が落ちるのか細かいところまで理解できていないのですが

	if( dwTimeDiff <= dwPerTiming ){

の間スクロールをストップするのを強制的に挟み込むことでスクロールスピードを落としている…んでしょうか?

PR の影響範囲

PR のデメリットを参照

テスト内容

いろいろスクロールしてみる。

テスト1

手順

関連 issue, PR

#1714

参考資料

@AppVeyorBot
Copy link

Build sakura 1.0.4151 failed (commit 43cc4e77c4 by @dep5)

@AppVeyorBot
Copy link

Build sakura 1.0.4158 completed (commit fadfb6b2bc by @dep5)

@sanomari
Copy link
Contributor

良さそうに見えます。

1		if( abs( (Int)nScrollRowNum ) < 7 &&
2			( m_pEditView->GetSelectionInfo().IsMouseSelecting() || m_pEditView->m_bDragMode )){

5				nScrollRowNum = 0;

マウスでドラッグ中、かつ、スクロール量が7行未満の場合に
何かの条件を満たしたらスクロール量を0にする(=スクロールしない)なので、
他の操作にはよっぽど影響しなさそうです。

@dep5
Copy link
Contributor Author

dep5 commented Jun 17, 2022

sanomari さん
ほかの操作へ影響がないか気にしているので、そういっていただけるとうれしいです。

@dep5
Copy link
Contributor Author

dep5 commented Jun 17, 2022

どれくらい遅くなるかテストしてみました。
PRの適用されていないビルドと適用されているビルドを比べます。
1.0.4160 Merge pull request #1857 from sanomari/feature/fix_bad_destructor_of_CDllImp
1.0.4158 Pull request #1853 - マウスドラッグ時のスクロール速度を制限する

  • 適当な行数のファイルを用意する

  • 文書編集表示行を10行にする

  • 別のウィンドウを開き、ガイドとして行番号を表示しておく
    (行番号がabs((Int)nScrollRowNum)相当)
    tempsnip

  • スクロールラインを越えて5秒待つ

  • floor((x-10)/5/行番号)) を計算する
    結果・scroll test.csv

Windows 10では
1秒で80~90行程度のスクロールが10行程度に速度が落ちています。
細かい操作が可能になります。

Windows 8.1では
1秒で1000行程度のスクロールが35行程度に速度が落ちています。
表示行を広めにとれば操作可能です。

@dep5 dep5 marked this pull request as ready for review June 17, 2022 12:40
berryzplus
berryzplus previously approved these changes Jun 17, 2022
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.

自分もこれでいったんマージで良いと思います。

あるべき姿としては、
特定の条件を満たした場合 CCaret::MoveCursor を呼ばない、
という修正を加えるのがベターなんじゃないかと思いました。

CCaret::MoveCursor の中を変えようとするから「他に影響するかも...」と心配になるわけなので改善の余地があるんですが、このPRをマージすればとりあえず「困り事」は解決するので進める価値はあると思います。

一方、一度マージしたものは「困り事がない限り決して変更してはならない」みたいなローカルルールにしたい場合にはこの状態でマージしてはいけないことになるんですが、ぼくは「リリースしないバージョンには冒険的変更を含めても構わない」と思うので approve してしまいます。(こういう approve をするからリリースできないんですけど 😃 )

@dep5
Copy link
Contributor Author

dep5 commented Jun 19, 2022

配列の要素を増やすとWindows8.1でのさらにスピードを落とせました。
1秒当たり10行程度のスクロール速度でWindows10と同程度になりました。
Edge 102でスクロールラインを越えてすぐの速度は13行ぐらいでした。
タスクマネージャーでCPU使用率を見ている限りではmasterに比べて、重くなってはいないようです。

もう少しシンプルにできそうに思っていろいろいじってみましたが、わたしにはこれ以上何もできなさそうです。

berryzplus さん
残念ながら、わたしにはberuさんに提案していただいたコードを少し書き換えることしかできませんでした。
別アプローチの開発も楽しみにしたいと思います。

@sonarcloud
Copy link

sonarcloud bot commented Jun 19, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

5.0% 5.0% Coverage
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

Build sakura 1.0.4161 completed (commit 394250bb88 by @dep5)

@@ -300,7 +300,7 @@ CLayoutInt CCaret::MoveCursor(
Int nScrollRowNum;
DWORD dwTime;
};
static std::array<ScrollRowRecord, 128> s_records{};
static std::array<ScrollRowRecord, 512> s_records{};
Copy link
Contributor

Choose a reason for hiding this comment

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

4倍の根拠 = 試してみて値が大きいほうがいい感じだったから。

@dep5
Copy link
Contributor Author

dep5 commented Jul 3, 2022

static std::array<ScrollRowRecord, 8> s_records{};

Windows 10で要素数を変えながら5秒間のスクロール行数を確認してみました。
#1853 (comment) の条件でスクロールラインを越えて1行目のみの行数です。

4-85
5-74
6-62
7-59
8-63

要素が6以上だと増やしてもスクロール行数に影響がないようにに思います。
Windows 8.1に対応しないのなら要素数が8あれば十分でしょうか?

要素数が8,128,512の時とmasterそのまま、と4つビルドして
パフォーマンスプロファイラーでWindows 10での動作についてテストしてみました。
同じファイルをそれぞれのビルドでスクロールしてみました。
メモリ使用量は5MB程度で違いが無いようです。
CPU使用率もグラフを見ると8と512では少し増えているかなと思いましたが
どちらもmasterに比べれば減っていると思います(スクロール行数が違うから当然ですが)
マウスドラッグによるスクロールしている間だけのコードなのを考えると、許容範囲かと思いました。

22df168
fa524e8
forループを書き直してみて初めて要素数を変えることを思いついたので今回変更しています。

数値を変えるとスクロールスピードを変更できる以下のコードについて

1 static std::array<ScrollRowRecord, 512> s_records{};
2 DWORD dwPerTiming = 80;

Windows 10
1 s_recordsは6以上は変化がない?
2 dwPerTimingの増減でスクロールスピードを操作できる

Windows 8.1
1 s_recordsを512にしてようやくWindows 10ぐらいのスピードまで落とせる
2 dwPerTimingの増減では効果が無い

https://github.com/sakura-editor/sakura/pull/1694/files
IsWindows10OrGreater()を使わずに(いずれWindows8.1対応をやめるPRをしないとだめなので)
Windows 10と8.1で両対応させたい、かつ速度も揃えたいので、s_recordsを512にしています。

512が多すぎるなら256や384ではどうでしょうか?
512が理想ですが256でも半減するので十分です。
Windows8.1
256-116
384-88
512-65

ユーザーの方の感想によってはスクロール速度を少し速くするかも知れないので、その場合は512も必要ないです

@berryzplus
Copy link
Contributor

とくに反対意見もつかないのでマージじておきます。

@berryzplus berryzplus merged commit 2d51a13 into sakura-editor:master Jul 7, 2022
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.

5 participants