-
Notifications
You must be signed in to change notification settings - Fork 157
add setting for default file suffix #223
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,8 +17,9 @@ class SettingsService | |
| private $root; | ||
|
|
||
| /* Default values */ | ||
| private $settings = [ | ||
| private $defaults = [ | ||
| "notesPath" => "Notes", | ||
| "fileSuffix" => ".txt", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please make this value globally changeable. this is the most important part of the issue your are fixing here.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR is about a per user setting. I think that we should do a system-wide (admin) setting in a separate PR (if it is really needed). |
||
| ]; | ||
|
|
||
| public function __construct( | ||
|
|
@@ -33,16 +34,42 @@ public function __construct( | |
| * @throws \OCP\PreConditionNotMetException | ||
| */ | ||
| public function set($settings) { | ||
| foreach($this->settings as $name => $value) { | ||
| $this->settings[$name] = isset($settings[$name]) ? $settings[$name] : $value; | ||
| // remove illegal, empty and default settings | ||
| foreach($settings as $name => $value) { | ||
| if(!array_key_exists($name, $this->defaults) | ||
| || empty($value) | ||
| || $value === $this->defaults[$name] | ||
| ) { | ||
| unset($settings[$name]); | ||
| } | ||
| } | ||
|
|
||
| $this->config->setUserValue($this->uid, 'notes', 'settings', json_encode($this->settings)); | ||
| $this->config->setUserValue($this->uid, 'notes', 'settings', json_encode($settings)); | ||
| } | ||
|
|
||
| public function get($name = null, $default = null) { | ||
| public function getAll() { | ||
| $settings = json_decode($this->config->getUserValue($this->uid, 'notes', 'settings')); | ||
| if(!$settings || !is_object($settings)) $settings = $this->settings; | ||
| return $name ? (property_exists($settings, $name) ? $settings->{$name} : $default) : $settings; | ||
| if(is_object($settings)) { | ||
| // use default for empty settings | ||
| foreach($this->defaults as $name => $defaultValue) { | ||
| if(!property_exists($settings, $name) || empty($settings->{$name})) { | ||
| $settings->{$name} = $defaultValue; | ||
| } | ||
| } | ||
| } else { | ||
| $settings = (object)$this->defaults; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i personally would prefer to have a real settings class here and call the constructor with the defaults array. typecasting here makes me have a bad feeling but as i don't know the coding standard in this project and in nextcloud i assume you can ignore this. for projects where i approve the PR i would probably request a change. in gerneral i would use symfonys option resolver for handling all the settings https://symfony.com/doc/current/components/options_resolver.html
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are not using symfony in this app, yet. Personally, I don't want to use another library just for this small thing. But please feel free to provide a PR with your proposed implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. symfony is not only a full stack framework, its components can easily used in many projects but as i wrote, nothing i really thought you would change. i only try to be constructive. |
||
| } | ||
| return $settings; | ||
| } | ||
|
|
||
| /** | ||
| * @throws \OCP\PreConditionNotMetException | ||
| */ | ||
| public function get($name) { | ||
| $settings = $this->getAll(); | ||
| if(property_exists($settings, $name)) { | ||
| return $settings->{$name}; | ||
| } else { | ||
| throw new \OCP\PreConditionNotMetException('Setting '.$name.' not found.'); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the change for
fileSuffixinstead ofextensionin all files?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is your comment about the naming? I chose
fileSuffixbecause the value includes the dot. I wanted to have the dot included, because it's more generic and I think that displaying.mdto the user is more helpful than displayingmd.