Skip to content

Commit

Permalink
fix(custom_settings): load built settings from $user_data_dir/build d…
Browse files Browse the repository at this point in the history 
…irectory
  • Loading branch information
@Prcuvu
Prcuvu committed Feb 13, 2018
1 parent 88f1dc2 commit 463dc09
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/rime/lever/custom_settings.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ CustomSettings::CustomSettings(Deployer* deployer,
bool CustomSettings::Load() {
fs::path user_data_path(deployer_->user_data_dir);
fs::path shared_data_path(deployer_->shared_data_dir);
fs::path config_path(user_data_path / (config_id_ + ".yaml"));
fs::path config_path(user_data_path / "build" / (config_id_ + ".yaml"));
if (!config_.LoadFromFile(config_path.string())) {
config_path = shared_data_path / (config_id_ + ".yaml");

This comment has been minimized.

Sign in to view

Copy link
@lotem

lotem Feb 14, 2018

Member

Thank you for fixing this.
I will make an additional change, once verified:

The fallback path for config (as ConfigComponent<ConfigLoader>) should be changed to $shared_data_path/build/.
This is because Config::LoadFromFile() only loads contents of the YAML file without using a ConfigCompiler, which is part of ConfigComponent<ConfigBuilder>. In order to load complete config data, the file has to be a built copy rather than a source file which may have external references.

This commit should fix everything for Weasel, in which the shared data is not prebuilt and the installer deploys user data instead. But in macOS and Linux the installer runs as root and has to deploy in shared data directory. config_builder will create built copies in $shared_data_dir/build/ which may be the copy used at run-time if user doesn't customize it; I anticipate in this case the user copy is not generated at all.

This comment has been minimized.

Sign in to view

Copy link
@Prcuvu

Prcuvu Feb 14, 2018 •
edited
Loading

Author Contributor

Please note that in the case of Weasel, $shared_data_path is located by default under $PROGRAMFILES, which requires administrator privilege.

Building copies in different directories depending on OS may create inconsistency, making frontend coding somewhat tricky; it also bring redundant binary data, and we lose advantages from runtime compilation. I would suggest that initial data deployment be triggered after installation, in the case of POSIX frontends.

This comment has been minimized.

Sign in to view

Copy link
@lotem

lotem Feb 14, 2018 •
edited
Loading

Member

The reason for introducing the complexity is to make sure Rime is ready when User begin to type their first word.

In Windows I assume that the (login) user who run the installer (usually the sole user in the system) is the "owner" of the data. Deploying only for the current user is much simpler.
In Linux the installer can't deploy for "the current login user", because at the time of installation the typist user is unknown. Therefore, to save time at first run, data is prebuilt before or during installation in shared data directory.

This comment has been minimized.

Sign in to view

Copy link
@lotem

lotem Feb 14, 2018

Member

Continue the discussion in #176

This comment has been minimized.

Sign in to view

Copy link
@nameoverflow

nameoverflow Feb 15, 2018

Member

On windows 10 we can install Weasel in %LOCALAPPDATA% by default which could be a more common choice for applications requiring complete file controlling privilege 🤔
if (!config_.LoadFromFile(config_path.string())) {
Expand Down

0 comments on commit 463dc09

Please sign in to comment.