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

マルチユーザー設定ファイルの不具合修正 #1865

Merged

Conversation

berryzplus
Copy link
Contributor

PR の目的

タイトル通りです。

カテゴリ

  • 不具合修正

PR の背景

#1863 の不具合を見つけました。

PR のメリット

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

仕様・動作説明

マルチユーザー設定ファイルのUserRootFolderに
2:ドキュメントフォルダ、3:デスクトップを指定した場合、
OneDriveのフォルダが取得されてしまう不具合を修正します。

この不具合はOneDriveをセットアップしないと再現しないのでCI/CDではチェックできません。

PR の影響範囲

テスト内容

テスト1

手順

関連 issue, PR

参考資料

マルチユーザー設定ファイルのUserRootFolderに
2:ドキュメントフォルダ、3:デスクトップを指定した場合、
OneDriveのフォルダが取得されてしまう不具合を修正する。

この不具合はOneDriveをセットアップしないと再現しないのでCI/CDではチェックできない。
@sonarcloud
Copy link

sonarcloud bot commented Sep 10, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

@beru
Copy link
Contributor

beru commented Sep 11, 2022

OneDriveは使っておらず現象を確認出来ませんが、コードレビューだけ行いました。もし必要であれば動作確認も行います。

https://github.com/sakura-editor/sakura/blob/91a09d7dd8a1309c3025945f1e720764fcedaf87/installer/sinst_src/sakura.exe.ini

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

FOLDERID_Profile
Default Path	%USERPROFILE% (%SystemDrive%\Users\%USERNAME%)

FOLDERID_Documents
Default Path	%USERPROFILE%\Documents

FOLDERID_Desktop
Default Path	%USERPROFILE%\Desktop

3:デスクトップ については対処がされていますが、 2:ドキュメントフォルダ については対処は不要でしょうか? このMRの説明の 仕様・動作説明 には下記のように記載されていたので気になりました。

マルチユーザー設定ファイルのUserRootFolderに
2:ドキュメントフォルダ、3:デスクトップを指定した場合、
OneDriveのフォルダが取得されてしまう不具合を修正します。

@beru
Copy link
Contributor

beru commented Sep 11, 2022

Windowsの各ユーザーのデスクトップフォルダの場所はカスタマイズしてデフォルトの場所とは違う場所に出来るようですが、サクラエディタがそれに対応するべきかどうかが気になります。

https://superuser.com/questions/328763/can-you-change-the-location-of-the-desktop-folder-in-windows
https://www.inasoft.org/webhelp/rnsf7/HLP000209.html?pc

今不具合が出ているという事はインストーラー側が正しくないってことなんですかね?

default:
refFolderId = FOLDERID_RoamingAppData; // ユーザーのアプリケーションデータフォルダー
break;
}

PWSTR pFolderPath = nullptr;
::SHGetKnownFolderPath(refFolderId, KF_FLAG_DEFAULT, nullptr, &pFolderPath);
::SHGetKnownFolderPath(refFolderId, KF_FLAG_DEFAULT_PATH, NULL, &pFolderPath);
Copy link
Contributor 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.

https://docs.microsoft.com/en-us/windows/win32/api/shlobj_core/ne-shlobj_core-known_folder_flag

を見ると下記のように書かれていたので、きっとそれでユーザー設定の変更を受けないデフォルトのパスってことなんですね。

KF_FLAG_DEFAULT_PATH
Value: 0x00000400
0x00000400. Gets the default path for a known folder. If this flag is not set, the function retrieves the current—and possibly redirected—path of the folder. The execution of this flag includes a verification of the folder's existence unless KF_FLAG_DONT_VERIFY is set.

@berryzplus
Copy link
Contributor Author

Windowsの各ユーザーのデスクトップフォルダの場所はカスタマイズしてデフォルトの場所とは違う場所に出来るようですが、サクラエディタがそれに対応するべきかどうかが気になります。

https://superuser.com/questions/328763/can-you-change-the-location-of-the-desktop-folder-in-windows https://www.inasoft.org/webhelp/rnsf7/HLP000209.html?pc

今不具合が出ているという事はインストーラー側が正しくないってことなんですかね?

現状で、インストーラーはマルチユーザー設定ファイルの内容を制御できないため、少なくともインストーラーの不具合ではないと思います。

発生している不具合は、
Windows 2000時代から利用しているユーザーがsakura.exe.iniの仕様に従って2,3を設定した場合のsakura.iniの保存場所が予期しない場所になる
というものです。

@beru
Copy link
Contributor

beru commented Sep 11, 2022

現状で、インストーラーはマルチユーザー設定ファイルの内容を制御できないため、少なくともインストーラーの不具合ではないと思います。

マルチユーザー設定ファイル、というのはきっと sakura.exe.ini の事で UserRootFolder エントリの値はインストーラーは変更しないのでインストーラーが関与はしてませんよ、って事ですかね?

installer/sakura-common.iss の記述をみるとマルチユーザー有効時の時に sakura.exe.ini を配置するようにしていますが、書き換えてはいないですね。

発生している不具合は、 Windows 2000時代から利用しているユーザーがsakura.exe.iniの仕様に従って2,3を設定した場合のsakura.iniの保存場所が予期しない場所になる というものです。

念のため確認ですが sakura.exe.ini の仕様というのは sakura.exe.ini に書かれている通りの事ですかね。

;	UserRootFolder: 基準となるパスの指定
;		0:アプリケーションデータフォルダ(デフォルト)	ex. C:\Documents and Settings\<username>\Application Data
;		1:ユーザフォルダ								ex. C:\Documents and Settings\<username>
;		2:ドキュメントフォルダ							ex. C:\Documents and Settings\<username>\My Documents
;		3:デスクトップフォルダ							ex. C:\Documents and Settings\<username>\デスクトップ

OS側でカスタマイズした場合はこの 仕様 と異なるという事ですが、そもそもこの仕様とか単体テストの期待値が、OS側のカスタマイズ値に追従出来ていない事が問題という事は無いでしょうか?

サクラエディタ側の仕様としてOSのカスタマイズに対応するかしないかがよくわかりません。

@berryzplus
Copy link
Contributor Author

マルチユーザー設定ファイル、というのはきっと sakura.exe.ini の事で UserRootFolder エントリの値はインストーラーは変更しないのでインストーラーが関与はしてませんよ、って事ですかね?

はい、そうです。

発生している不具合は、 Windows 2000時代から利用しているユーザーがsakura.exe.iniの仕様に従って2,3を設定した場合のsakura.iniの保存場所が予期しない場所になる というものです。

念のため確認ですが sakura.exe.ini の仕様というのは sakura.exe.ini に書かれている通りの事ですかね。

はい、そうです。

OS側でカスタマイズした場合はこの 仕様 と異なるという事ですが、そもそもこの仕様とか単体テストの期待値が、OS側のカスタマイズ値に追従出来ていない事が問題という事は無いでしょうか?

サクラエディタ側の仕様としてOSのカスタマイズに対応するかしないかがよくわかりません。

OSのカスタマイズに対応するかどうかは、メンバーが勝手に決めてよいと思っています。
対応すると単体テストが複雑になるので、今回は「対応しない」に倒そうとしてます。
(COM関数を呼ぶときは先にCoInitializeしなければならない)

単体テスト側の条件を修正して対応するのもアリだとは思います。
その場合 #1864 をマージしてから先に進みたいです。

@beru
Copy link
Contributor

beru commented Sep 12, 2022

OSのカスタマイズに対応するかどうかは、メンバーが勝手に決めてよいと思っています。

それであればこのPRの変更内容は問題無いと思いますのでApproveしました。
もしOSのカスタマイズに対応したいユーザーがいたらissueやPRが作られるかなと。

対応すると単体テストが複雑になるので、今回は「対応しない」に倒そうとしてます。
(COM関数を呼ぶときは先にCoInitializeしなければならない)

サクラエディタ本体側はおそらく既にそうしていると思うので、単体テスト側が、って事ですよね。

単体テスト側の条件を修正して対応するのもアリだとは思います。
その場合 #1864 をマージしてから先に進みたいです。

単体テスト側の動作確認はしていませんが、変更内容的にはフィクスチャクラスを使ってテスト終了時(失敗時も含む)に TearDown で確実に設定ファイルが削除されるようにしたようですね。

@berryzplus
Copy link
Contributor Author

レビューありがとうございます。
とりあえずマージしておきます。

@berryzplus berryzplus merged commit b0b7b83 into sakura-editor:master Sep 17, 2022
@berryzplus berryzplus deleted the feature/fix_exeini_settings branch September 17, 2022 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OneDriveをセットアップを完了させた端末で単体テストが失敗する
3 participants