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

BSキーでカーソル前を削除時に画面が余計に再描画される事を防ぐ #1754

Merged
merged 1 commit into from
Jan 14, 2022

Conversation

beru
Copy link
Contributor

@beru beru commented Jan 4, 2022

PR の目的

BSキー押しでカーソル前を削除時に、余計な画面の再描画が行われないようにするのが目的です。

カテゴリ

  • 速度向上

PR の背景

ミニマップを表示するとBSキーで文字を連続で削除するのが明らかに遅いです。
DELキーでの連続削除だとそこまで遅くは無いです。

PR のメリット

BSキーで文字を削除する際に余計な再描画が行われなくなる事で操作の応答性が良くなります。

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

余計だと思っていた再描画がもし必要だった場合に、再描画されない問題が発生する可能性があります。

仕様・動作説明

CViewCommander::Command_DELETE_BACKCDocEditor::m_nOpeBlkRedawCount をインクリメントする処理を追加する事で後で画面全体の再描画が行われる事を防ぎます。キャレット移動の CMoveCaretOpe 追加では再描画は必要無いはずです。

全てのペインを再描画する CEditWnd::RedrawAllViews の呼び出しは CEditView::SetUndoBuffer で行われてます。その呼び出しは CViewCommander::HandleCommand の最後で行われます。

PR の影響範囲

BSキーでカーソル前削除時の画面描画の挙動

テスト内容

テスト1

手順

  • 行数が大きいファイルを開く(例えば sakura_core\sakura_rc.rc
  • ミニマップを表示する設定にする
  • DELキーを押し続けて連続で文字を削除する
  • DELキーを離す
  • BSキーを押し続けて連続で文字を削除する
  • BSキーを離す
  • DELキーの場合とBSキーの場合で応答性が変わらない事を確認

関連 issue, PR

参考資料

https://egg.5ch.net/test/read.cgi/software/1587603412/423

…BlkRedawCount をインクリメントする処理を追加

キャレット移動の CMoveCaretOpe 追加では再描画は必要無いはず
@beru beru added the 🚅 speed up 🚀 高速化 label Jan 4, 2022
@sonarcloud
Copy link

sonarcloud bot commented Jan 4, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

Build sakura 1.0.3973 completed (commit c151ac2640 by @beru)

@@ -857,6 +857,7 @@ void CViewCommander::Command_DELETE_BACK( void )
GetCaret().GetCaretLogicPos()
)
);
GetDocument()->m_cDocEditor.m_nOpeBlkRedawCount++;
Copy link
Contributor

Choose a reason for hiding this comment

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

修正内容はここだけ(1行追加)ですよね?
 ↓
他の処理から「本来必要な処理」を抜いてきて貼り付けた、ってことですよね?

【このPRの修正の成り立ち(推定)】
再描画が遅くならないようにする機構がもともと存在していた。
BSキー押下時の処理にはこの機構を有効にするための1文が抜けている(ように見えた)。
・・・ので、「~したときのコード」から必要な1文をコピってきた。

PR本文の説明とまるで違うので、そうではないんだろうな、と思いました。
成り立ちが上記想定に一致するなら入れてしまってもいいと思います。

現状、自分はこの件で困ってないです。
BSだとエディタによっては処理遅延が発生するので、
削除範囲を選択してから削除するクセがついてるためかと思います。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

既存の記述を参考というかコピってきたのはその通りです。DELキーだとビューの全再描画はされていなかったのでその仕組みを真似ています。

PR本文の説明にそれが書かれていないのは、多分書いた人に親切心が足りてないからだと思われます。

なおミニマップを表示していない時でもBSキー時に全ビューを再描画する挙動は同じなのですがそこまで重くないです。気づかない間に密かに無駄遣いしていたんですね。

@berryzplus
Copy link
Contributor

動作確認して問題なければ入れていいんじゃないかと思います。

@beru
Copy link
Contributor Author

beru commented Jan 12, 2022

動作確認して問題なければ入れていいんじゃないかと思います。

「動作確認しました!」と申告したら「よし通れ!」と Approve つけて頂けるのでしょうか?
PR出した本人の申告では駄目で、だれか違うレビューアーの確認が必要になるのかな。。

@berryzplus
Copy link
Contributor

他の方にレビューを任せます、という意味です。

@beru
Copy link
Contributor Author

beru commented Jan 13, 2022

他の方にレビューを任せます、という意味です。

このPRのレビューや動作確認をberryzplusさんにして頂けないのは残念ですが、今まで数多くのPRを作成したりレビューしてくれて多大な貢献を行ってくれたので非常に感謝してます。きっと非効率なマウス操作を行う世界から卒業してしまったんですね。

@berryzplus
Copy link
Contributor

このPRの修正内容は、表面的には妥当だと思います。
同様の処理で問題がおきてないコードから適切なコードを持ってきました、は正しいと思います。
あとは動作確認で「なんとなく大丈夫」なら入れてよいと思ってます。

レビュアー独自の観点で深堀すると異なる結論になりそうなことと、
おそらくソレは要らないだろう、と考えたことからパスを宣言しました。
ニーズがあって解決策が妥当なら入れればよいです。
メンバーは10人くらいいた気がするので・・・。

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

だいぶスムーズになったと思います。

@beru
Copy link
Contributor Author

beru commented Jan 14, 2022

レビュ=ありがとうございました。Mergeします。
もし問題が見つかったら別のMRで修正します。

@beru beru merged commit 12d5c59 into master Jan 14, 2022
@beru beru deleted the minimap_backspace_redraw_all_views branch January 14, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants