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

CTextMetrics のテストを追加する #1545

Merged

Conversation

kengoide
Copy link
Member

PR の目的

CTextMetrics の自動テストを追加します。

カテゴリ

  • リファクタリング
  • その他の問題

PR のメリット

コードカバレッジが向上します。

PR のデメリット

今回はないと思います。

PR の影響範囲

既存コードの動作を変更するものではありません。

* 使われていなかった CTextMetrics::m_anZenkakuDx を削除
* CCharWidthCache を使用するように変更
@kengoide kengoide changed the title Feature/tests for ctextmetrics CTextMetrics のテストを追加する Feb 19, 2021
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@kengoide
Copy link
Member Author

This function has 8 parameters, which is greater than the 7 authorized.

テスト導入後に予定しているコード整理パッチで対応します。

@kengoide kengoide marked this pull request as ready for review February 19, 2021 11:52
@AppVeyorBot
Copy link

Build sakura 1.0.3459 completed (commit 603510cffa by @k-kagari)

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

問題ないと思います。

@@ -29,6 +29,9 @@
//2007.08.25 kobake 追加

#include <vector>
#include <Windows.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

どっちでもいいですが、Windows.hはvectorよりも前な気がします。

ヘッダー内インクルードの優先順

  1. osのインクルードファイル(Windows.hが最優先)
  2. C言語ランタイムのヘッダーファイル(<>でインクルードするもののうち、.hが付くもの)
  3. C++ヘッダー(<>でインクルードするもの)
  4. サクラエディタ独自のヘッダー

実は誰も気にしとらんかもしれないです・・・。

Copy link
Member Author

Choose a reason for hiding this comment

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

気にしていますが正解が分からないです…。

個人的には標準ライブラリ→他ライブラリ→プラットフォーム依存ライブラリ→プロジェクト内ファイルのつもりで書いていました。

Copy link
Contributor

Choose a reason for hiding this comment

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

個人的には標準ライブラリ→他ライブラリ→プラットフォーム依存ライブラリ→プロジェクト内ファイルのつもりで書いていました。

.hとC++ヘッダーに優先順を付けるか?の違いなのかな、と。

細かいところは、なんとなくまんべんなくコードを動かせるようになってから対応したいなぁ、と思っています。

@@ -128,7 +134,6 @@ class CTextMetrics{
int m_nDxBasis; //!< 半角文字の文字間隔 (横幅+α)
int m_nDyBasis; //!< 半角文字の行間隔 (縦幅+α)
int m_anHankakuDx[64]; //!< 半角用文字間隔配列
int m_anZenkakuDx[64]; //!< 全角用文字間隔配列
Copy link
Contributor

Choose a reason for hiding this comment

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

指摘じゃないですが、全角デラックスを削除するのはいいとして、半角デラックスは残しといていいんですかね?

Copy link
Member Author

Choose a reason for hiding this comment

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

全要素に同じ値が入る無意味な配列ですが、
ExtTextOut の引数として使用している例があるため存置しています。最終形からは削除予定です。

Copy link
Contributor

Choose a reason for hiding this comment

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

生GDI ExtTextOutの引数にするのであれば、
この配列にはデバイス単位の文字幅(px)を入れる仕様のはずで、
等幅フォントの半角幅なら全部同じでよいっす。
プロポーショナルフォント対応や結合文字の考慮を始めるとゼロとか入ってくる感じです。

tests/unittests/test-ctextmetrics.cpp Show resolved Hide resolved
protected:
CTextMetricsWithGDI() {
lf1.lfCharSet = DEFAULT_CHARSET;
std::wcscpy(lf1.lfFaceName, L"MS Gothic");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::wcscpy(lf1.lfFaceName, L"MS Gothic");
::wcscpy_s(lf1.lfFaceName, L"MS Gothic");

https://docs.microsoft.com/ja-jp/cpp/c-runtime-library/reference/strcpy-s-wcscpy-s-mbscpy-s?view=msvc-160

Copy link
Member Author

Choose a reason for hiding this comment

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

次回からはセキュア版関数にしておきます…。

@kengoide kengoide merged commit f2e38fe into sakura-editor:master Feb 20, 2021
@kengoide kengoide deleted the feature/tests-for-ctextmetrics branch February 20, 2021 04:38
@kengoide
Copy link
Member Author

レビューありがとうございました。

@beru beru added the refactoring リファクタリング 【ChangeLog除外】 label Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring リファクタリング 【ChangeLog除外】
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants