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.

Sign up for GitHub

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

Jump to bottom

PageUp, PageDown 時に描画する必要が無い場合は描画しないようにする判定を追加 #1320

Merged

beru merged 2 commits into sakura-editor:master from beru:PageUp_PageDown

Sep 6, 2020

Contributor

beru commented Jun 13, 2020 •

edited

Loading

PR の目的

キャレットが画面の最上行にいる時に PageUp キーを押し続けた場合、もう画面の変化が無い場合にも描画を繰り返し行ってしまいます。キャレットが画面の最下行にいる時に PageDown キーを押し続けた場合も同様です。

このPRの目的は、画面の描画を行う必要が無い場合は行わないように判定する事で、プロセッサが無駄に動かないようにして消費電力を抑える事です。

カテゴリ

速度向上？

PR のメリット

描画を行う必要が無い場合に描画を省く事でプロセッサの使用率を下げる事が出来ます。

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

描画を行う必要が有るかどうかの判定に不具合があると、本来は描画しなければいけないケースで描画がされない事になります。
判定処理が入る分コードが複雑になって読みづらくなります。

テスト内容

何かのテキストファイル（自分は sakura_core\sakura_rc.rc を使いました）を開いて、エディタのスクロール操作を行いました。

テスト1

手順

キャレットを最上行に移動して PageUp キーを押し続けても CPU 使用率が上がらない事を確認（変更前は上がる）
キャレットを最下行に移動して PageDown キーを押し続けても CPU 使用率が上がらない事を確認（変更前は上がる）
PageUp, PageDown キー押しによるスクロールの挙動が以前と同じかどうか目視で確認、Shiftキー押しのPageUp, PageDown についても確認

PR の影響範囲

PageUp キー、PageDown キー押し時の処理

beru added the 🚅 speed up 🚀 label

AppVeyorBot commented Jun 13, 2020

❌ Build sakura 1.0.2856 failed (commit be9cd3a04e by @beru)

AppVeyorBot commented Jun 13, 2020

✅ Build sakura 1.0.2857 completed (commit be9cd3a04e by @beru)

Contributor

berryzplus commented Sep 5, 2020

/azp run

azure-pipelines bot commented Sep 5, 2020

Azure Pipelines successfully started running 1 pipeline(s).

Contributor

berryzplus commented Sep 5, 2020

なんかビルドが完了してなかったみたいなので放置していました。
PR元ブランチ名にfeatureって付いてないから正しくビルドできてなかったのかな？
ビルド成否については一旦気にせずにレビューを始めてみたいと思います。

berryzplus reviewed

View reviewed changes

Contributor

berryzplus left a comment

空の Azure Pipelines ビルドが登録されてしまってるようなのでビルド完了は無理そうです。
ざっと見た感じ、とくに問題はなさそうに見えます。

おそらく本質的な問題は RedrawAll を呼び出す実装になってることなので、SetDrawSwitch(false) をやめる方向の修正にしたら分岐とか必要なくなる気がしました。

質問1点と修正提案2件がありますので対応よろしくです。

sakura_core/cmd/CViewCommander_Cursor.cpp Outdated

		CLayoutInt nViewTopLine = m_pCommanderView->GetTextArea().GetViewTopLine();
		const bool bDrawSwitchOld = m_pCommanderView->SetDrawSwitch(false);

Contributor

berryzplus Sep 5, 2020

質問です。
この実行時定数の宣言位置を移動したことについて、何か理由がありますか？

変更前739行目が描画と関係ない処理だから、という理由だとすると、
もっと下まで描画には関係ない処理なので、移動先が違うような気がします。
（変更前742行目までは描画と関係ないっす。）

Contributor Author

beru Sep 5, 2020

理由は覚えていませんが今見返すと特に移動する必要性が無いように見えますね。
元の位置に戻しておきます。

sakura_core/cmd/CViewCommander_Cursor.cpp Outdated

		CLayoutInt nViewTopLine = m_pCommanderView->GetTextArea().GetViewTopLine();
		const bool bDrawSwitchOld = m_pCommanderView->SetDrawSwitch(false);

Contributor

berryzplus Sep 5, 2020

同上です。

Contributor Author

beru Sep 5, 2020

元の位置に戻しておきます。

sakura_core/cmd/CViewCommander_Cursor.cpp Outdated

+              		auto& caret = GetCaret();
+              		auto prevCaretPos = caret.GetCaretLayoutPos();
+              		caret.Cursor_UPDOWN( -nScrollNum, bSelect );
+              		auto currCaretPos = caret.GetCaretLayoutPos();
               		//	Sep. 11, 2004 genta 同期スクロール処理のため
               		//	m_pCommanderView->RedrawAllではなくScrollAtを使うように

Contributor

berryzplus Sep 5, 2020

既に実装とかけ離れた古いコメントは混乱の元になるので削るべきだと思います。

Contributor Author

beru Sep 5, 2020

なんかよくわかりませんが（このコメントとコードの関係性とか）削っておきます。

sakura_core/cmd/CViewCommander_Cursor.cpp

               		//	Sep. 11, 2004 genta 同期スクロール処理のため
               		//	m_pCommanderView->RedrawAllではなくScrollAtを使うように
-              		m_pCommanderView->SyncScrollV( m_pCommanderView->ScrollAtV( nViewTopLine - nScrollNum ));
+              		CLayoutInt nScrolled = m_pCommanderView->ScrollAtV( nViewTopLine - nScrollNum );

Contributor

berryzplus Sep 5, 2020

やや好みの問題ですが、正確には CLayoutYInt nScrolled と思います。
めんどうなら auto nScrolled でもよいです。

Contributor Author

beru Sep 5, 2020 •

edited

Loading

そうなんですか？ CEditView::ScrollAtV の戻り値の型は CLayoutInt ですが、どうして CLayoutYInt にする方が正確なんでしょうか？

Contributor

berryzplus Sep 6, 2020

ええと・・、「べき論」ではないんですが・・・。
CLayoutInt型は、現在3つの意味で使われています。

例	正確な型	備考
`CLayoutInt nLines`	`CLayoutYInt`	垂直方向のレイアウト単位（≒行）です
`CLayoutInt nPos`	`CLayoutXInt`	水平方向のレイアウト単位（≒桁）です
`CLayoutInt tabPadding`	`LONG`	水平方向の幅・ピクセル単位で、誤用です

そんなに遠くない未来に CLayoutInt の X,Y を区別するようにしたいと考えています。

「好みの問題」と言ってるように、いまその基準に合わせる必然はないっす。

sakura_core/cmd/CViewCommander_Cursor.cpp

		m_pCommanderView->SetDrawSwitch(bDrawSwitchOld);
		if (prevCaretPos == currCaretPos && nScrolled == 0) {

Contributor

berryzplus Sep 5, 2020

メモ：カーソル位置が変化しなかった、かつ、スクロール行数が0だった場合

Contributor Author

beru Sep 5, 2020

せっかくなのでコメントを入れる事にします。

sakura_core/cmd/CViewCommander_Cursor.cpp

               		m_pCommanderView->SetDrawSwitch(bDrawSwitchOld);
+              		if (prevCaretPos == currCaretPos && nScrolled == 0) {
+              			return;
+              		}
               		m_pCommanderView->RedrawAll();

Contributor

berryzplus Sep 5, 2020

メモ：これがCPU使用率を上げてしまう、問題のあるコードです。

スクロール操作で再描画する必要がある領域はスクロール前に表示されていなかった被スクロール領域とスクロールバーだけなので一般的に描画領域の一部ですが、このコードではあえて全域を再描画しています。

Contributor Author

beru Sep 5, 2020 •

edited

Loading

描画処理の最適化は本当は出来たら良いんですが、もう今の実装ごちゃごちゃしてて読み進めて理解して整理したり書き換えるの大変ですね。

大きく変更して改善（描画のパフォーマンスを劇的に上げるとか）しないとテキストエディタとして使い物にならないかっていうとそんな事も無いと思うので、今のままでも良いかなとか個人的に思ってます。

beru added 2 commits

September 6, 2020 03:36


          PageUp, PageDown 時に描画する必要が無い場合は描画しないようにする判定を追加

685c9e6

CViewCommander::Command_1PageUp と CViewCommander::Command_1PageDown で、キャレットの更新前後の位置が変化が無く、スクロール量も 0 の場合は CEditView::RedrawAll の呼び出しを行わないようにする


          レビュー指摘対応

60e290f

beru force-pushed the PageUp_PageDown branch from 0b5f218 to 60e290f Compare

September 5, 2020 19:10

AppVeyorBot commented Sep 5, 2020

✅ Build sakura 1.0.3068 completed (commit 7f1713be36 by @beru)

berryzplus approved these changes

View reviewed changes

Contributor

berryzplus left a comment

問題ないと思います。

動作確認結果

sakura_rc.rcを開いて、PageDown,PageUpキーを押しっぱなしにしてCPU使用率をチェック。

--	PageDown	PageDown(下端到達後)	PageUp	PageUp(上端到達後)
変更前	20%	20%	20%	20%
変更後	20%	6%	20%	6%

Contributor Author

beru commented Sep 6, 2020

レビュー有難うございました。Mergeします。

beru merged commit 2d2f3cb into sakura-editor:master

beru deleted the PageUp_PageDown branch

September 6, 2020 05:17

beru mentioned this pull request

カーソルが先頭行にある状態からさらに PageUp キーを押すと縦線が残る #1617

Closed

beru added the 💩degradation🧻🚽 label

beru mentioned this pull request

タイプ別設定の「カーソル位置縦線」を有効時に、カーソルが先頭行にある状態で PageUp キーを押した後にカーソル左右移動するとカーソル位置縦線が残る問題を修正 #1619

Merged

This was referenced Apr 29, 2021

先頭行でPageUp・末尾行でPageDownした後のカーソル移動で下線が残る #1650

Closed

先頭行でPageUp・末尾行でPageDownした後のカーソル移動で下線が残る不具合に対処 #1652

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🚅 speed up 🚀 💩degradation🧻🚽