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

sakura.iniがない状態で起動した時にマウスホイールでフォントサイズが変更できない問題を修正 #1536

Merged

Conversation

suconbu
Copy link
Member

@suconbu suconbu commented Feb 9, 2021

PR の目的

sakura.ini が置かれていない状態でサクラエディタを起動した後、Ctrl + マウスホイール操作をしてもフォントサイズが 100% のまま変更できない問題を修正します。

※この問題は #1513 の反映後 (73d4fe7 以降) から発生

カテゴリ

  • 不具合修正

PR の背景

#1513 (comment) にて報告頂いた問題の修正です。

PR のメリット

不具合が解消されます。

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

特にありません。

仕様・動作説明

Ctrl + マウスホイール操作時の処理概要

CViewCommander::Command_SETFONTSIZE にてマウスホイールの回転方向に応じた相対指定によるフォントサイズ設定が行われます。
この相対指定時にはその時点の共通設定またはタイプ別設定のフォントサイズを基準値とし、それにズーム倍率を乗算したものを変更後のフォントサイズ候補として算出します。
算出されたフォントサイズが変更前と異なる場合に限り、そのサイズを一時適用フォントサイズとしてして設定、エディタに反映します。

フォントサイズ変更ができない理由について

sakura.ini が置かれていない状態でサクラエディタを起動した時には、LOGFONT 構造体のフォントサイズとして 10pt 相当の値が設定される一方、ポイント単位のサイズを保持する変数 CommonSetting_View::m_nPointSize には 0 (無効値) が設定されます。

lf.lfHeight = DpiPointsToPixels(-10); // 2009.10.01 ryoji 高DPI対応(ポイント数から算出)

sView.m_nPointSize = 0; // フォントサイズ(1/10ポイント単位) ※古いバージョンからの移行を考慮して無効値で初期化 // 2009.10.01 ryoji

相対指定時における基準値となる CommonSetting_View::m_nPointSize が 0 の場合、ズーム倍率を適用した後のフォントサイズも必ず 0 となるため、サイズ変更なしとしてフォントサイズの反映がスキップされます。
結果、マウスホイール操作に対して何も反応がない状態となります。

対策方法

CViewCommander::Command_SETFONTSIZE において、相対指定時の基準値が 0 (無効値) の場合には、LOGFONT 構造体の lfHeight の値から pt 単位のフォントサイズを算出してそれを基準値として使うようにします。
また、もし lfHeight から得たフォントサイズも 0 となる場合には、設定可能なフォントサイズの下限値 (=1pt) を基準値とします。

PR の影響範囲

  • マウスホイールによるフォントサイズ変更動作
  • マクロコマンド SetFontSize の振る舞い

テスト内容

sakura.ini がない状態で起動させた直後の動作を確認します。

  • (起動直後) Ctrl + ホイール操作でフォントサイズの拡大縮小ができること。
  • (起動直後) 以下のマクロ (.js) を「名前を指定してマクロ実行」で実行した時にフォントサイズの倍率が 200% に変わること。
SetFontSize( 0, 5, 2 );

関連 issue, PR

#1513

参考資料

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 9, 2021

@AppVeyorBot
Copy link

Build sakura 1.0.3442 completed (commit 662bc8b689 by @suconbu)

@beru beru added the 🐛bug🦋 ■バグ修正(Something isn't working) label Feb 9, 2021
@sanomari
Copy link
Contributor

CViewCommander::Command_SETFONTSIZE において、相対指定時の基準値が 0 (無効値) の場合には、LOGFONT 構造体の lfHeight の値から pt 単位のフォントサイズを算出してそれを基準値として使うようにします。
また、もし lfHeight から得たフォントサイズも 0 となる場合には、設定可能なフォントサイズの下限値 (=1pt) を基準値とします。

結局、挿入したコードのバグだと言ってます?

ちゃんと読めていませんが CommonSetting_View::m_nPointSize が 0 でなければ問題ない、と言ってるように見えます。
で、無効値(=0)だった場合計算して適切な値を入れて対策するって言ってるように見えます。

無効値だったら値を入れるってのは初期化ですよね?
つまり「変数の初期化漏れ」を発見したと言ってる気がします。

だとすると、問題あるのは拡大縮小のコードじゃないと思います。

@suconbu
Copy link
Member Author

suconbu commented Feb 10, 2021

この件に関して以下二つの事柄があると考えています。

  1. sakura.ini なしで起動した場合 sView.m_nPointSize が 0 になる
  2. sView.m_nPointSize が 0 の場合に拡大縮小の処理が正しく動作しない

#1513 の反映前には「1」の状態であっても (挙動は少し変ですが) できていた拡大縮小操作が、反映後にはできなくなりました。
この点は #1513 反映前から見て劣化してしまったと考えていますので、(「1」が正しい状態かどうかは一旦置いておき) まずここ (=「2」) を修正しようとしています。

@suconbu
Copy link
Member Author

suconbu commented Feb 10, 2021

おかしな状態に合わせて修正する必要はなく、もし「1」がおかしいのであればまずこちらを是正、という進め方も浮かびましたが、今回は劣化させてしまった状態で置いておきたくないという思いから、先に「2」の修正 PR 出させてもらいました。

@berryzplus
Copy link
Contributor

うーむ・・・。このまま進めていいんじゃないでしょうか。

困っていること

拡大・縮小が動かないことがある。

原因

基準サイズが無効であるために、サイズ変更を検出できない。
 👇なぜ?
既存コードから選定した基準サイズ変数の初期化が漏れている。
 👇なぜ?
フォントサイズの特定に使える変数が別にあるため、基準サイズ変数の初期化は必須ではない。

対策

基準サイズは lfHeight から算出するように改める。(このPRの提案内容。)
紛らわしい変数 CommonSetting_View::m_nPointSize を削除する。(これは別件で良さそうです。)

その他考慮事項

設定で lfHeight を変更した直後の動きがスムーズかチェックしとくべきなのかも知れません。

@berryzplus
Copy link
Contributor

一応ハッキリさせときます。
このPRをapproveにしない理由は👇です。

テスト内容

後で書きます。

原因分析と対策内容から確認すべき内容は自明なので「省略」でも構わない気がします。
「後で書きます。」なので書かれるのを待っています。

@suconbu
Copy link
Member Author

suconbu commented Feb 11, 2021

遅くなりましたがテスト内容を書きました。二項目とも OK です。

Code Smell B 1 Code Smell

分岐が増えたことで「複雑度高い」と指摘が出ていますが、
以下が対応される時には追加した分岐もおそらく不要にできると思いますので、今回は放置としておきたいです。

紛らわしい変数 CommonSetting_View::m_nPointSize を削除する。(これは別件で良さそうです。)

@berryzplus
Copy link
Contributor

遅くなりましたがテスト内容を書きました。二項目とも OK です。

PR本文にテスト内容が反映されていないみたいです。

@suconbu
Copy link
Member Author

suconbu commented Feb 13, 2021

PR本文にテスト内容が反映されていないみたいです。

すみません💧更新しました。

@suconbu
Copy link
Member Author

suconbu commented Feb 13, 2021

ありがとうございました。マージいたします。

@suconbu suconbu merged commit 503d619 into sakura-editor:master Feb 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛bug🦋 ■バグ修正(Something isn't working)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants