add setting for default file suffix by korelstar · Pull Request #223 · nextcloud/notes

korelstar · 2018-07-03T20:07:23Z

fixes #24

add setting for default file suffix: drop-down box for selecting values (txt, md)
use setting when creating new files
use default value for empty settings

korelstar · 2018-07-05T20:38:45Z

Ready for review

c33s · 2018-07-08T11:28:55Z

please keep in mind, that i am not familar with the nextcloud code and plugin development at all, i don't know if all the things i am writing here are possible/not possibe/completly wrong/has sideeffects/are already possible with this code/... please correct me if i am wrong.

as far as i can see you implemented a setting for the extension which can be changed in the plugin settings page. which is very nice, but isn't this only a per user setting?

in the issue #24 there was a lot of discussion about this topic:

gui vs no gui
default extension
auto migration
per user- vs global setting
saving user from complicated things

often i read something like "saving the user from ..." but with this PR we don't save the user and leave the admin without an influence to that.

the simplest approach for this would have been, to declare it as admin only feature, so no user can change it, the default is defined by the admin in config.php (or wherever such a global setting is placed in nextcloud). so the admins can choose what they want to support.
decide which extension should be the default (i vote for .md here but i am ok with .txt as long this is globally changeable for all users (no no manual query in the database where i change it for all users and then define a stored procedure / trigger where i change it for all new created user)
for me after implementing and deciding step 1. & 2. the next step would have been to add it as gui = per user setting but maybe only with the option to disable the user config for certain groups or completly (we want to save the user from overcomplicated interfaces right? (it is not my way but some people expressed their concern about it. so its no sarkasm i really mean it)

with this PR it looks for me we are only a part having [3]. instead of [1] or [1, 2] or [1, 2, 3] where for me having [1] is the most important part.

what is really missing for me would to be able to configure the fileSuffix you added in $defaults, in a simply manner via cli (without browser, without login). for example add a simple string to config.php or run a cli cmd php bin/console plugin:setting notes --fileSuffix='.md' (borrowed the cmd syntax from symfony, i don't know if there is something equivalent in nextcloud)

c33s · 2018-07-08T11:38:17Z

        // pass -1 because no file has id -1 and that will ensure
        // to only return filenames that dont yet exist
-        $path = $this->generateFileName($folder, $title, "txt", -1);
+        $path = $this->generateFileName($folder, $title, $this->settings->get('fileSuffix'), -1);


why the change for fileSuffix instead of extension in all files?

Is your comment about the naming? I chose fileSuffix because the value includes the dot. I wanted to have the dot included, because it's more generic and I think that displaying .md to the user is more helpful than displaying md.

c33s · 2018-07-08T11:39:58Z

-	private $settings = [
+	private $defaults = [
 		"notesPath" => "Notes",
+		"fileSuffix" => ".txt",


please make this value globally changeable. this is the most important part of the issue your are fixing here.
maybe even configuring the notesPath globally would be nice.

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).

c33s · 2018-07-08T11:46:42Z

+				}
+			}
+		} else {
+			$settings = (object)$this->defaults;


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

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.

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.

korelstar · 2018-07-08T12:19:34Z

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). What's your opinion on this, @jancborchardt ?

jancborchardt · 2018-07-17T22:20:00Z

This is absolutely a per-user setting. It should be none of the admins concern what the default file extension for Notes is. Everyone might use different systems and workflows.

Also, the default should of course stay .txt while Windows still can’t really read .md by default. This is not a voting for which format we like better, but we need to stay compatible. :)

jancborchardt · 2018-07-18T14:19:51Z

@camilasan @stefan-niedermann and @nextcloud/notes can you review this too? :)

jancborchardt

@korelstar I would only say to change it to "File extension for new notes" instead of "File suffix", because "File extension" is the term people use. :) Otherwise good from UX! :)

Also, the colon ":" can be removed from both of the settings, and there should ideally be some whitespace between the entries?

korelstar · 2018-07-22T20:34:25Z

@jancborchardt I implemented your change requests and I added the confirm buttons, you requested in nextcloud/server#1068.

Please review again ...

jancborchardt · 2018-07-23T15:14:02Z

Good stuff, looks great! Thanks for also adding the confirm function! :) (We could also think about not having the confirm for the file extension and simply applying when you switch.)

korelstar · 2018-07-23T20:56:04Z

(We could also think about not having the confirm for the file extension and simply applying when you switch.)

Could we only think about this or should I remove the button? I have no opinion on this. You're the design boss. ;-)
However, it would be nice to have a clear recommendation for the general usage of these buttons.

korelstar · 2018-07-24T19:34:18Z

We can do this later, if needed.

jancborchardt · 2018-08-09T15:42:52Z

Could we only think about this or should I remove the button? I have no opinion on this.

Hehe – ok then I will say it would be great to not have to confirm but simply apply when the select value is changed. :)

korelstar added the enhancement New feature or request label Jul 3, 2018

korelstar mentioned this pull request Jul 3, 2018

Change default file extention #153

Closed

korelstar force-pushed the file-extension branch from 840d422 to d841585 Compare July 5, 2018 20:36

korelstar changed the title ~~[WiP] add setting for default file suffix~~ add setting for default file suffix Jul 5, 2018

korelstar mentioned this pull request Jul 5, 2018

Add setting to create new files with markdown file extension (.md) #24

Closed

add setting for default file suffix

43c5a46

korelstar force-pushed the file-extension branch from bac9082 to 43c5a46 Compare July 8, 2018 11:57

c33s suggested changes Jul 8, 2018

View reviewed changes

jancborchardt requested review from camilasan and stefan-niedermann July 18, 2018 14:19

camilasan approved these changes Jul 18, 2018

View reviewed changes

stefan-niedermann approved these changes Jul 18, 2018

View reviewed changes

jancborchardt requested changes Jul 18, 2018

View reviewed changes

UI fixes

9073489

jancborchardt approved these changes Jul 23, 2018

View reviewed changes

korelstar merged commit fe52c71 into master Jul 24, 2018

korelstar deleted the file-extension branch July 24, 2018 19:34

korelstar added this to the 2.4.0 milestone Aug 2, 2018

korelstar mentioned this pull request Aug 13, 2018

remove unnecessary confirm button for select #240

Merged

korelstar mentioned this pull request Sep 23, 2018

Md file extension #257

Closed

korelstar restored the file-extension branch October 14, 2018 15:41

korelstar deleted the file-extension branch October 14, 2018 17:10

Uh oh!

Conversation

korelstar commented Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

korelstar commented Jul 5, 2018

Uh oh!

c33s commented Jul 8, 2018

Uh oh!

c33s Jul 8, 2018

Choose a reason for hiding this comment

Uh oh!

korelstar Jul 8, 2018

Choose a reason for hiding this comment

Uh oh!

c33s Jul 8, 2018

Choose a reason for hiding this comment

Uh oh!

korelstar Jul 8, 2018

Choose a reason for hiding this comment

Uh oh!

c33s Jul 8, 2018

Choose a reason for hiding this comment

Uh oh!

korelstar Jul 8, 2018

Choose a reason for hiding this comment

Uh oh!

c33s Jul 8, 2018

Choose a reason for hiding this comment

Uh oh!

korelstar commented Jul 8, 2018

Uh oh!

jancborchardt commented Jul 17, 2018

Uh oh!

jancborchardt commented Jul 18, 2018

Uh oh!

jancborchardt left a comment

Choose a reason for hiding this comment

Uh oh!

korelstar commented Jul 22, 2018

Uh oh!

jancborchardt commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

korelstar commented Jul 23, 2018

Uh oh!

korelstar commented Jul 24, 2018

Uh oh!

jancborchardt commented Aug 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

korelstar commented Jul 3, 2018 •

edited

Loading

jancborchardt commented Jul 23, 2018 •

edited

Loading