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

ノート線描画を少し分かりやすくする #1066

Merged
merged 5 commits into from
Oct 6, 2019

Conversation

berryzplus
Copy link
Contributor

PR の目的

ノート線描画に関わるコードを整理して、メンテナンスしやすくします。

カテゴリ

  • リファクタリング

PR の背景

#1065 (comment) で指摘した内容を実装してみました。

  1. 指定した範囲内にノート線を描くメソッドに単数形の名前が付いている
    ⇒ 元々複数の横線を描く目的のメソッドだから改名しよう!
  2. 引数の型が int になので、ピクセル指定か桁指定か紛らわしい(と思った)
    ※ 最近の Windows GDI では、ピクセル位置を指定するのに LONG 型の変数を使います。
    ⇒ 引数がピクセル指定であることを明示するために LONG 型に訂正しよう!
  3. 領域を受け取る関数は left, top, right, bottom の順に指定するのが標準(だと思います)
    標準 に合わせて指定順を変更しよう!
  4. 複数の線分を描く目的の関数なのに、線分を1本ずつ描いているのが効率悪い(気がする)
    ⇒ 描画に必要なデータを作る処理とAPI呼出を分離して一撃で描画するように改善しよう!

PR のメリット

  • ノート線描画の処理が少し分かりやすくなります。

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

  • メソッドの引数変更を伴うので、影響範囲が広いです。
  • この対応を行うことにより、ノート線描画にもDPI対応が必要であることに気付きましたが、放置しています。

PR の影響範囲

  • アプリ(=サクラエディタ)の機能に影響がある変更です。
    • ただし、見た目も体感速度も変化してない気がします。
  • ノート線描画を行う処理に影響します。
    • 通常の描画処理(行番号を含む行データの描画)
    • 通常の描画処理(行番号のみの描画)
    • 対括弧の強調表示により上書き描画を行う際のノート線の描画処理
  • ノート線描画の処理内容が変更になります。
    • 変更前) 描画するノート線の本数分だけ MoveToEx + LineTo 繰り返し。
    • 変更後) 描画するノート線の本数に関わらず PolyPolyLine 一発。
  • 変更による影響として大きいのは、今後のメンテナンスがしやすくなる(主観)ことだと思っています。

関連チケット

#1065 ノート線描画、指定桁縦線描画、折り返し桁縦線描画、を行毎ではなく一括で行うように変更

参考資料

https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-polypolyline
https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-movetoex
https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-lineto

変更前) CTextDrawer::DispNoteLine
変更後) CTextDrawer::DispNoteLines
領域内に等間隔の横線を引くメソッドなので複数形が正しい
GDI関数に渡す幅と高さはintでなくLONGとするのが通例なので合わせる。
SetRectの引数に合わせ、left,top,right,bottomにする。
変更前) MoveToEx + LineTo を繰り返し呼び出して実現
変更後) PolyPolyline で一発描画
@AppVeyorBot
Copy link

beru
beru previously approved these changes Oct 6, 2019
Copy link
Contributor

@beru beru 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

beru commented Oct 6, 2019

処理時間の観点からのコメントですが、ノート線描画を有効にした場合 CTextDrawer::DispLineNumber から呼び出される CTextDrawer::DispNoteLines の処理に 5% 程度時間が掛かっているようです(Performance Profiler で確認)。

共通設定のウィンドウの「行番号とテキストの隙間」の値が 0 ドットだと、leftright の値は同じになる事が多いので呼び出す意味が無いような気がします。

高速化の為に CTextDrawer::DispLineNumber 内で呼び出さずに CEditView::OnPaint2 内で呼び出せると良いですが、ちょっとその変更は行いにくいかもしれません。

ところでノート線の存在って今まで意識してませんでした。使ってる人いるのかな…。

@berryzplus
Copy link
Contributor Author

レビューありがとうございます。
#1067 のレビューで、入れといたほうがよいキャストがあることに気付いたので後で追加コミットつみます。

処理時間の観点からのコメントですが、ノート線描画を有効にした場合 CTextDrawer::DispLineNumber から呼び出される CTextDrawer::DispNoteLines の処理に 5% 程度時間が掛かっているようです(Performance Profiler で確認)。

共通設定のウィンドウの「行番号とテキストの隙間」の値が 0 ドットだと、left と right の値は同じになる事が多いので呼び出す意味が無いような気がします。

高速化の為に CTextDrawer::DispLineNumber 内で呼び出さずに CEditView::OnPaint2 内で呼び出せると良いですが、ちょっとその変更は行いにくいかもしれません。

いけるんじゃないかな?と 😃

  • CTextDrawer::DispLineNumber 行単位で行番号を描画する関数
  • CTextDrawer::DispNoteLines 範囲指定で複数のノート線を描画する関数

役割を明確にしたことによって、外に出したほうが良いと分かったから外に出します、で問題ない気がします。

ところでノート線の存在って今まで意識してませんでした。使ってる人いるのかな…。

サクラエディタのユーザー層には「文筆家」という職業の人がいるらしいです。
twitterのつぶやき数でもってそう思ってるだけなので本当かどうかは知りません。

モノ書きの人にとってみれば、横書きテキストにノート線がはいっていると落ち着くのかも。
デフォルトの色指定だと薄い青紫になってて、ちょうど本物の紙のノートみたいですし。

64bitビルドでの警告を抑制するため。
@AppVeyorBot
Copy link

@berryzplus
Copy link
Contributor Author

レビューありがとうございます。
何か問題が見つかったら別PRで対処しようと思います。

@berryzplus berryzplus merged commit d2a8294 into sakura-editor:master Oct 6, 2019
@berryzplus berryzplus deleted the feature/DispNoteLines branch October 6, 2019 12:41
@m-tmatma m-tmatma added this to the v2.4.0 milestone Dec 29, 2019
@KENCHjp KENCHjp added the refactoring リファクタリング 【ChangeLog除外】 label Jan 7, 2020
@beru beru added no-changelog 【ChangeLog除外】 reverted labels Jan 16, 2020
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 16, 2020
…eLines

ノート線描画を少し分かりやすくする
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog 【ChangeLog除外】 refactoring リファクタリング 【ChangeLog除外】 reverted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants