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

exeと設定ファイルのパス取得関数をリファクタリングしてテストできるようにする #1512

Merged

Conversation

sanomari
Copy link
Contributor

@sanomari sanomari commented Jan 22, 2021

PR の目的

タイトル通りです。

カテゴリ

  • リファクタリング

PR の背景

#1435 (comment) を参照してください。

PR のメリット

INIファイルパスの生成規則が少しわかりやすくなります。

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

なにもないと思います。

仕様・動作説明

表面的な仕様・動作に変更はありません。

このPRは共有メモリの構造体メンバーを変更するため、共有メモリバージョンを変更します。

-- 共有メモリバージョン
変更前 176
変更後 177

このPRはINIファイルパスの生成規則を明確にします。

プロセスの種類 タイミング 変更前仕様 変更後仕様
CControlProcess 生成前 - EXEファイルと同じフォルダ
CControlProcess 初期化前 - 導出規則で生成
CControlProcess 初期化後 共有メモリより取得 共有メモリより取得
CNormalProcess 生成前 - EXEファイルと同じフォルダ
CNormalProcess 初期化前 - 共有メモリより取得
CNormalProcess 初期化後 共有メモリより取得 共有メモリより取得

導出規則

  • マルチユーザ構成設定ファイル(sakura.exe.ini)のSettingsセクションのMultiUserを見る。
    • 0ならマルチユーザ設定無効。
    • 1ならマルチユーザ設定有効。
  • マルチユーザ設定有効の場合、SettingsセクションのUserRootFolderを見る。
    • 1ならユーザのルートフォルダをルートフォルダにする。
    • 2ならユーザのドキュメントフォルダをルートフォルダにする。
    • 3ならユーザのデスクトップフォルダをルートフォルダにする。
    • 上記以外はユーザのアプリケーションデータフォルダをルートフォルダにする。
    • 設定ファイルにエントリがなかった場合、0を指定したものと看做される。
  • マルチユーザ設定有効の場合、SettingsセクションのUserSubFolderを見る。
    • 指定された名前のサブフォルダがルートフォルダ直下に作られる。
    • 空を指定した場合、"sakura"を指定したものと看做される。
    • 設定ファイルにエントリがなかった場合、"sakura"を指定したものと看做される。
  • プロファイル指定がある場合、プロファイル名のフォルダが作られる。
    • この仕様はマルチユーザ設定無効の場合でも有効。

PR の影響範囲

ちょっと広いです。

  • プロセス生成前のINIファイルパス取得ができるようになります。
  • EXE基準ファイルパスの取得に影響します。
  • INI基準ファイルパスの取得に影響します。
  • プロファイルマネージャの機能に映鏡する可能性があります。
  • 共有メモリバージョンに依存する処理に影響する可能性があります。

テスト内容

修正したコードのほぼすべてを網羅するテストを作成しました。
カバーできなかった行が3行あるので完全ではありませんが、充分なテストを行えていると思います。

なお、カバーできなかったのはプロファイル指定ありで起動するケースです。

関連 issue, PR

#1435

参考資料

https://cpprefjp.github.io/reference/filesystem/path/remove_filename.html
https://docs.microsoft.com/ja-jp/windows/win32/api/shlobj_core/nf-shlobj_core-shgetfolderlocation
https://docs.microsoft.com/en-us/windows/win32/api/shlobj_core/nf-shlobj_core-shgetknownfolderpath
https://docs.microsoft.com/en-us/windows/win32/shell/knownfolderid
https://docs.microsoft.com/en-us/windows/win32/api/processenv/nf-processenv-expandenvironmentstringsw

@AppVeyorBot
Copy link

Build sakura 1.0.3356 failed (commit f095a4a1d4 by @sanomari)

@sanomari sanomari force-pushed the feature/refactoring_of_getinipath branch from 30ff159 to f41f9c6 Compare January 22, 2021 15:37
@AppVeyorBot
Copy link

Build sakura 1.0.3357 failed (commit 37ac09d961 by @sanomari)

@sanomari sanomari force-pushed the feature/refactoring_of_getinipath branch from f41f9c6 to fff3063 Compare January 23, 2021 02:52
@AppVeyorBot
Copy link

Build sakura 1.0.3358 completed (commit c1a3c17a12 by @sanomari)

@AppVeyorBot
Copy link

Build sakura 1.0.3359 failed (commit 5f3784063d by @sanomari)

@AppVeyorBot
Copy link

Build sakura 1.0.3361 completed (commit b2a8a385a9 by @sanomari)

@AppVeyorBot
Copy link

Build sakura 1.0.3362 completed (commit 8cb93e0399 by @sanomari)

@berryzplus
Copy link
Contributor

もしかしてSonarCloudの指摘事項を駆逐しようとしてます?

個人的には、残課題ありでもOKなつもりでいます。

@AppVeyorBot
Copy link

Build sakura 1.0.3363 completed (commit 12bcdf8594 by @sanomari)

@AppVeyorBot
Copy link

Build sakura 1.0.3364 completed (commit 6e91d5bd30 by @sanomari)

@AppVeyorBot
Copy link

Build sakura 1.0.3365 completed (commit e581649ebf by @sanomari)

@sanomari sanomari force-pushed the feature/refactoring_of_getinipath branch from 6549b71 to 1f1b98c Compare January 24, 2021 02:40
@sanomari sanomari added refactoring リファクタリング 【ChangeLog除外】 SonarQube labels Jan 24, 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 0 Code Smells

97.1% 97.1% Coverage
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

Build sakura 1.0.3370 completed (commit e93e51042a by @sanomari)

@sanomari sanomari marked this pull request as ready for review January 24, 2021 03:18
@sanomari
Copy link
Contributor Author

もしかしてSonarCloudの指摘事項を駆逐しようとしてます?

個人的には、残課題ありでもOKなつもりでいます。

このあたりの修正でミスると「起動できない」になるので徹底的にやりました。

カバレッジに関してだけは残課題として残してしまいます。
対処するにはtest-winmain.cppに大幅な変更が要りそうです。
PRの趣旨と離れてしまうので今回は対応を見送りました。

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.

問題なさそうに見えます。

もともと自分がやりたかった変更を引き継いでもらった形なので、「対応を入れることそのもの」の是非判断については甘いかも知れません。

@sanomari sanomari merged commit 6bbeb0e into sakura-editor:master Jan 24, 2021
@sanomari sanomari deleted the feature/refactoring_of_getinipath branch January 24, 2021 09:05
@berryzplus
Copy link
Contributor

2chで報告されているデグレの原因になりうるのはこちらですかね。
#1511 (comment)

詳細が何も書いてないので何がおきているかはわかりませんが。

@sanomari
Copy link
Contributor Author

詳細が何も書いてないので何がおきているかはわかりませんが。

これじゃないですかね?
https://osdn.net/projects/sakura-editor/forums/34071/43611/

1.0.3370以降で使えないとあり、このPRは1.0.3371でマージされています。
6bbeb0e

このPRはGetInidirの実装を変えていますが、コーナーケースをうまくハンドリングできてないかも知れないです。

sanomari added a commit to sanomari/sakura-editor that referenced this pull request May 1, 2022
N_SHAREDATA_VERSIONの修正ミスを訂正します。
SonarCloudの指摘に従ってdefineをconstexprに置換しましたが、既存のCマクロに悪影響が出るので元に戻します。
sanomari added a commit that referenced this pull request May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring リファクタリング 【ChangeLog除外】 SonarQube
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants