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

タイプ別設定のインポートでバージョンチェックが正しく働いていなかった #1613

Conversation

usagisita
Copy link
Contributor

PR の目的

タイプ別設定インポートのときに表示される、iniファイルのバージョン情報が必ず「?」になっていた不具合を修正します。

カテゴリ

  • 不具合修正

PR の背景

Visual StudioでWarningLevel4のチェックをしていたところ、見つけた不具合のひとつです。
せっかくなので直したいです。

PR のメリット

バグが修正されます。

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

仕様・動作説明

読み込みのためにszKeyVersionという変数が宣言されていましたが、これが定数の同名のiniキー名を隠ぺいしていました。
そのため、IOProfileDataの行でキー名「?」を読み込もうとして、必ず失敗していました。
変数名を変更したうえで長さの心配がないstd::wstringに置き換えました。

PR の影響範囲

タイプ別設定のインポート時に表示される、バージョンが違う時の警告のダイアログの表示だけです。

テスト内容

テスト1

手順

古いバージョンのタイプ別設定のエクスポートしたiniファイルを用意します。
そうでない場合は、最新版でエクスポートしたiniファイルをエディタで開いて、末尾にある[Info]のszKeyStructureVersionを小さい値に書き換えます。

タイプ別設定一覧でインポートしようとすると、
「エクスポートしたsakura(?/177)とバージョンが異なります。
インポートしてもよろしいですか?」
というようなダイアログが表示されます。
修正前は「?」になっています。
PRの修正後は「2.4.2.0」のようにiniの[Info]szVersionキーの内容が表示されるようになります。

関連 issue, PR

参考資料

・変数名の隠ぺいによりバージョン情報のキー名が「?」になっていたのを修正
・バージョン情報読み取りをstd::wstringに置き換え
@sonarcloud
Copy link

sonarcloud bot commented Mar 28, 2021

Kudos, SonarCloud Quality Gate passed!

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

Build sakura 1.0.3625 completed (commit 2a55226a05 by @usagisita)

berryzplus
berryzplus previously approved these changes Mar 29, 2021
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.

やや悩ましいです。

sakura_core/typeprop/CImpExpManager.cpp Show resolved Hide resolved
@berryzplus berryzplus dismissed their stale review March 29, 2021 00:46

間違いました。

@usagisita
Copy link
Contributor Author

L"szKeyVersion"で直るというのは、自分もそう思います。
ただし「変数名の隠ぺい(?)」も「C-スタイル固定長配列文字列」も一応、SonarCloudの警告対象のようでした。
https://sonarcloud.io/project/issues?directories=sakura_core%2Ftypeprop&fileUuids=AWdx6FIKcrQ7oSxgA3n6&id=sakura-editor_sakura&open=AWdx6FwTcrQ7oSxgA4wX&resolved=false

@berryzplus
Copy link
Contributor

berryzplus commented Mar 29, 2021

L"szKeyVersion"で直るというのは、自分もそう思います。
ただし「変数名の隠ぺい(?)」も「C-スタイル固定長配列文字列」も一応、SonarCloudの警告対象のようでした。
https://sonarcloud.io/project/issues?directories=sakura_core%2Ftypeprop&fileUuids=AWdx6FIKcrQ7oSxgA3n6&id=sakura-editor_sakura&open=AWdx6FwTcrQ7oSxgA4wX&resolved=false

ようやく理解しました。

問題あるのはこっちのコードで、PR修正内容は正しそうです。

static const wchar_t szKeyVersion[] = L"szVersion";

👆のコードの問題点

  • グローバルスコープに宣言するものに、それと分かる命名をしていない。
    他を見たら分かると思いますが g_szKeyVersion ないし KEY_VERSION とする方針っぽいです。
  • 「定数」を定義するのに constexpr 修飾を付けていない。
    コンパイル時定数 constexpr は C++11 からなので、仕方ないといえば仕方ないっす。

この問題の対処は別件としてリファクタリングしたほうが良いかもしれません。

@usagisita
Copy link
Contributor Author

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

@usagisita usagisita merged commit 290999b into sakura-editor:master Mar 29, 2021
@usagisita usagisita deleted the feature/fix_import_type_setting_ver_check branch March 29, 2021 13:27
@beru beru added the 🐛bug🦋 ■バグ修正(Something isn't working) label Mar 31, 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.

4 participants