表示スケールに合わせて拡大する処理を色々と追加 #1762

beru · 2022-01-10T13:28:22Z

ミニマップのウィンドウ横幅とフォントサイズを表示スケールに合わせて拡大する処理を追加
ミニマップのフォントサイズのデフォルト値を -1 から -2 に変更
ルーラーの高さ、ルーラーとテキストの隙間、行番号とテキストの隙間を表示スケールに合わせて拡大する処理を追加
文字の間隔と行の間隔を表示スケールに合わせて拡大する処理を追加

PR の目的

ディスプレイの表示スケールに合わせて見た目がスケールするようにする。

カテゴリ

仕様変更

PR の背景

PR のメリット

表示スケールに変更した際に見た目がスケールするので設定値を変更する必要がなくなる。

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

dot by dot のピクセル単位では設定出来なくなるので、表示スケールが大きい数値の時にピクセル単位で細かく調整する事が出来なくなります。

仕様・動作説明

変更前はディスプレイの表示スケールを掛けないピクセル単位でした。

PR の影響範囲

項目に関連する画面の見た目

テスト内容

様々な表示スケールで画面の見た目を確認

参考資料

ミニマップのフォントサイズのデフォルト値を -1 から -2 に変更ルーラーの高さ、ルーラーとテキストの隙間、行番号とテキストの隙間を表示スケールに合わせて拡大する処理を追加文字の間隔と行の間隔を表示スケールに合わせて拡大する処理を追加

sonarcloud · 2022-01-10T13:39:30Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

94.4% Coverage
0.0% Duplication

AppVeyorBot · 2022-01-10T13:39:32Z

❌ Build sakura 1.0.3988 failed (commit 1bc3d3425b by @beru)

berryzplus · 2022-01-10T14:23:41Z

この制限外します？

sakura/sakura/sakura.vcxproj

Line 87 in 138ec8e

    
           <PreprocessorDefinitions>_WIN32_WINNT=_WIN32_WINNT_WIN7;%(PreprocessorDefinitions)</PreprocessorDefinitions>

_WIN32_WINNTを_WIN32_WINNT_WIN7にしたままだと、
Windows 10のGetSystemMetricsForDpiを利用できず、
HighDPI対応を2回やらナイトいけなくなるような気がします。

berryzplus · 2022-01-10T14:24:42Z

CIのビルドエラーに見えているものは、LeProcのインストール失敗によるものなのでスルーしてよいと思います。

beru · 2022-01-10T15:05:27Z

この制限外します？

sakura/sakura/sakura.vcxproj

Line 87 in 138ec8e

<PreprocessorDefinitions>_WIN32_WINNT=_WIN32_WINNT_WIN7;%(PreprocessorDefinitions)</PreprocessorDefinitions>

_WIN32_WINNTを_WIN32_WINNT_WIN7にしたままだと、 Windows 10のGetSystemMetricsForDpiを利用できず、 HighDPI対応を2回やらナイトいけなくなるような気がします。

定かでは無いんですが、サクラエディタの対応OSにWindows 8.1を今はまだ入れていたような気がします。検索してみたところWindows 8.1の延長サポートは2023年1月10日に終了となるようなので、それを過ぎたらさすがに切り捨ててWindows10以降に限定して良いと思います。

高DPI対応では GetDpiForWindow を使うとPer-Monitor DPI対応も行いやすいですね。Per-monitor DPI対応しようとするとソースコードの色々な箇所を書き換えないといけなくて大変なので行われるか不明ですが…。

GetSystemMetricsForDpi が使えないとHighDPI対応を2回やらないといけなくなる、という理由は理解出来ていません。Google検索すると #519 がヒットしたので読んでみました。

しかしなんかうまくコード書いてまとめないとHighDPI対応の為にWindowsAPIをそこらじゅうで呼び出しまくる事になってしまいますね。既にごちゃごちゃしてるのに輪をかけてとかいうと首を絞められるかも。。

berryzplus · 2022-01-10T15:23:33Z

自分も良く分かってなかったんですが、公式サイト（？）に Windows 10以降と書いてありました。
https://sakura-editor.github.io/

なので、Windows 8.1対応は考慮不要なはずです。
現状のプロジェクト設定だとwin7準拠コードしか書けませんけれども。

beru · 2022-01-10T15:45:55Z

自分も良く分かってなかったんですが、公式サイト（？）に Windows 10以降と書いてありました。 https://sakura-editor.github.io/
なので、Windows 8.1対応は考慮不要なはずです。現状のプロジェクト設定だとwin7準拠コードしか書けませんけれども。

記憶が正しければ文章（Ver. 2.4.0.0 以降のバージョンは MS Windows 10 で動作します。）をそのように書いたのは自分だったような気がします。Windows 8.1の記載を省略したのは記載を簡潔にする為だったか書くのが面倒くさかったのか、まぁ確信犯というか切り捨てたいバイアスが働いたのかもしれません。

ただ時間が経過してもうみんなWindows 10かWindows 11(自分はまだ未使用)を使っているだろうし、Windows 8.1は切り捨てても良いんじゃないかとも思いますが、

今から1年後の2023/01/10に延長サポートが過ぎるまでは切り捨てては駄目だ派
それが過ぎても切り捨てては駄目だ派

の意見が強ければ今のままでしょうか…。

berryzplus · 2022-01-11T00:42:18Z

GetSystemMetricsForDpi が使えないとHighDPI対応を2回やらないといけなくなる、という理由は理解出来ていません。Google検索すると #519 がヒットしたので読んでみました。

システムDPI対応で1回、PerMonitorV2対応で2回です。

現状ではシステムDPI対応も完了していなくって、
このPRの対応もシステムDPI対応にあたると思います。

beru · 2022-01-11T12:25:23Z

システムDPI対応で1回、PerMonitorV2対応で2回です。

現状ではシステムDPI対応も完了していなくって、このPRの対応もシステムDPI対応にあたると思います。

なるほど、それで2回という事だったんですね。GetSystemMetrics と比較して GetSystemMetricsForDpi は dpi 引数が追加されていて、Per-Monitor DPI対応をする場合にこのAPIを使うと。文章の背後の意図が読めてませんでした。

一足飛びに対応するか、2回に分けて対応するのかに特にこだわりはないです。Per-Monitor DPI対応をするとなると変更量が増えるので対応に時間は掛かりますね。

個人的にはWindows 8.1を切り捨てても構わないと思います。とはいえ _WIN32_WINNT=_WIN32_WINNT_WIN10 に変更するPRを出してそれがすんなりマージされるかは…何事もやってみないと分からないですね。

berryzplus · 2022-01-12T10:24:35Z

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

berryzplus

表示確認はできないのでコメント付けるつもりなかったんですが、コードチェックしてみました。

根拠不明なベタ書き定数を1件、誤記と思われるDPI変換を2件見つけました。
妥当な理由があれば修正要求は取り下げます。

sakura_core/env/CShareData.cpp

berryzplus · 2022-01-13T03:56:57Z

sakura_core/view/CRuler.cpp

@@ -122,7 +123,7 @@ void CRuler::DrawRulerBg(CGraphics& gr)
 	}
 	if (m_hFont == NULL) {
 		LOGFONT	lf = {0};
-		lf.lfHeight			= 1 - pCommon->m_sWindow.m_nRulerHeight;	//	2002/05/13 ai
+		lf.lfHeight			= 1 - DpiScaleY(pCommon->m_sWindow.m_nRulerHeight);	//	2002/05/13 ai


Suggested change

lf.lfHeight = 1 - DpiScaleY(pCommon->m_sWindow.m_nRulerHeight); // 2002/05/13 ai

lf.lfHeight = DpiScaleY(1 - pCommon->m_sWindow.m_nRulerHeight); // 2002/05/13 ai

じゃないですかね？

元のコードが 1 - n としてる理由が謎ですが。

元のコードが 1 - n している理由は自分も分かっていません。どちらの記述が良いのかは後で動作確認してみます。

お願いします。

LOGFONT.lfHeightに負数を指定してCreateFontIndirectすると、指定した高さのフォントが使われる、という仕様があるはずです。正数を指定した場合は、指定した値に近似するサイズが使われる（つまり、指定したサイズと一致しないかもしれない）仕様です。

1 - n はおそらく、
・指定したサイズより少し小さいフォント
を意味していると考えられます。
たぶん、2より小さい値が設定されることは想定してないっす。

なんか前に、ルーラーって消せなくていいんでしたっけ？というやりとりがあったようななかったような・・・。

負数は文字の高さ、正数はセルの高さですね。
正数だとセル内の余白の分、文字の高さが小さくなります。

sakura_core/view/figures/CFigure_ZenSpace.cpp

berryzplus · 2022-01-13T04:35:21Z

付けるつもりなかったのは、レビューですね・・・

k-takata · 2022-01-18T03:11:05Z

数日前にVimのPerMonitorV2対応を行いました。 vim/vim@c81e9bf
sakuraエディタのPerMonitorV2対応を行う際には何らかのアドバイスができるかもしれません。

berryzplus · 2022-01-21T04:16:27Z

残1件です。

1 - nでnだけDPI対応してるが、1 - nをDPI対応しなくてよいか？
納得できる理由があれば変更要求は取り下げます。

berryzplus · 2022-03-20T12:53:52Z

ステータス変わってないように見えました。

request changesで残1件です。
#1762 (comment)

元が1 - nのコードでnだけDPI対応してるのがなんで？という話です。
たぶん1 - n全体をDPI対応しないといかんはず。。。

beru · 2022-03-20T14:35:51Z

ステータス変わってないように見えました。

request changesで残1件です。 #1762 (comment)

元が1 - nのコードでnだけDPI対応してるのがなんで？という話です。たぶん1 - n全体をDPI対応しないといかんはず。。。

う、そういえば対応していませんでした。元の 1 - n の計算の意味合いが良く分からなくてそのまま引き延ばして良いのか謎です。まぁ動かして結果を見て確認するしかなさそうですね。

berryzplus · 2022-03-21T02:59:50Z

1 - n で負数にした値を指定する意味は takataさんコメントを参照です。
nだけ対応すると、高解像度のときのフォントが少し大きくなります。

beru · 2022-03-21T04:03:49Z

1 - n で負数にした値を指定する意味は takataさんコメントを参照です。 nだけ対応すると、高解像度のときのフォントが少し大きくなります。

#1762 (comment) のberryzplusさんのコメントとtakataさんコメントに答えが書いてありそうですね。

1 - n はおそらく、
・指定したサイズより少し小さいフォント
を意味していると考えられます。
たぶん、2より小さい値が設定されることは想定してないっす。

2より小さい値というのは正の数だと1や0ですが、1だと0になって0だと-1になるのでサイズ的には逆転しそうですが指定されないだろうから問題ないという事ですね。了解しました。

beru · 2022-03-21T04:40:57Z

2より小さい値が設定されないようにUI側でチェックが入っていました。

sakura/sakura_core/_main/global.h

Line 101 in e67c8ce

const int IDC_SPIN_nRulerHeight_MIN = 2;

定数 IDC_SPIN_nRulerHeight_MIN の値が 2 で、CPropWin の実装でそれを下回らないようにしてました。

1 - nの修正を確認したので。

berryzplus

システムDPI対応として問題なさそうに思います。

CIがエラーになってるので。

berryzplus

GHAのSonarCloudビルドが失敗していますが、PRに起因する問題ではなさそうなのでスルーします。

AppVeyorBot · 2022-03-21T05:41:57Z

✅ Build sakura 1.0.4097 completed (commit ae28b056ea by @beru)

beru · 2022-03-21T09:40:12Z

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

beru · 2022-03-21T10:45:35Z

1 - n の記述ですが、 -(n - 1) と同じ結果になるので、

berryzplusさんコメント : 指定したサイズより少し小さいフォント
takataさんコメント : 負数は文字の高さ、正数はセルの高さ、正数だとセル内の余白の分、文字の高さが小さくなる

を展開した記述と捉える事が出来そうです。

beru added the specification change ■仕様変更 label Jan 10, 2022

berryzplus previously requested changes Jan 13, 2022

View reviewed changes

レビュー指摘対応

89d6456

berryzplus previously approved these changes Mar 21, 2022

View reviewed changes

berryzplus approved these changes Mar 21, 2022

View reviewed changes

beru merged commit 535e951 into master Mar 21, 2022

beru deleted the dpiscale branch March 21, 2022 09:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

表示スケールに合わせて拡大する処理を色々と追加 #1762

表示スケールに合わせて拡大する処理を色々と追加 #1762

beru commented Jan 10, 2022

sonarcloud bot commented Jan 10, 2022

AppVeyorBot commented Jan 10, 2022

berryzplus commented Jan 10, 2022

berryzplus commented Jan 10, 2022

beru commented Jan 10, 2022

berryzplus commented Jan 10, 2022

beru commented Jan 10, 2022

berryzplus commented Jan 11, 2022

beru commented Jan 11, 2022

berryzplus commented Jan 12, 2022

berryzplus left a comment

berryzplus Jan 13, 2022

beru Jan 13, 2022

berryzplus Jan 14, 2022 •

edited

k-takata Jan 14, 2022

berryzplus commented Jan 13, 2022

k-takata commented Jan 18, 2022

berryzplus commented Jan 21, 2022

berryzplus commented Mar 20, 2022

beru commented Mar 20, 2022

berryzplus commented Mar 21, 2022

beru commented Mar 21, 2022

beru commented Mar 21, 2022

berryzplus left a comment

berryzplus left a comment

AppVeyorBot commented Mar 21, 2022

beru commented Mar 21, 2022

beru commented Mar 21, 2022

	lf.lfHeight = 1 - DpiScaleY(pCommon->m_sWindow.m_nRulerHeight); // 2002/05/13 ai
	lf.lfHeight = DpiScaleY(1 - pCommon->m_sWindow.m_nRulerHeight); // 2002/05/13 ai

表示スケールに合わせて拡大する処理を色々と追加 #1762

表示スケールに合わせて拡大する処理を色々と追加 #1762

Conversation

beru commented Jan 10, 2022

PR の目的

カテゴリ

PR の背景

PR のメリット

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

仕様・動作説明

PR の影響範囲

テスト内容

関連 issue, PR

参考資料

sonarcloud bot commented Jan 10, 2022

AppVeyorBot commented Jan 10, 2022

berryzplus commented Jan 10, 2022

berryzplus commented Jan 10, 2022

beru commented Jan 10, 2022

berryzplus commented Jan 10, 2022

beru commented Jan 10, 2022

berryzplus commented Jan 11, 2022

beru commented Jan 11, 2022

berryzplus commented Jan 12, 2022

berryzplus left a comment

Choose a reason for hiding this comment

berryzplus Jan 13, 2022

Choose a reason for hiding this comment

beru Jan 13, 2022

Choose a reason for hiding this comment

berryzplus Jan 14, 2022 • edited

Choose a reason for hiding this comment

k-takata Jan 14, 2022

Choose a reason for hiding this comment

berryzplus commented Jan 13, 2022

k-takata commented Jan 18, 2022

berryzplus commented Jan 21, 2022

berryzplus commented Mar 20, 2022

beru commented Mar 20, 2022

berryzplus commented Mar 21, 2022

beru commented Mar 21, 2022

beru commented Mar 21, 2022

berryzplus left a comment

Choose a reason for hiding this comment

berryzplus left a comment

Choose a reason for hiding this comment

AppVeyorBot commented Mar 21, 2022

beru commented Mar 21, 2022

beru commented Mar 21, 2022

berryzplus Jan 14, 2022 •

edited