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

feat(DeclarativeSettings): Allow to define getter and setters in declarative settings form class #48721

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Oct 15, 2024

Summary

Instead of being required to write 3 event listeners this short cuts the declarative settings by being able to implement the DeclarativeSettingsFormWithHandlers interface to have a class providing getSchema, setValue and getValue.
There is also an example implementation in the files app that I migrated for demonstration purpose.

Checklist

… directly in Form

Instead of implementing the form class, a setter event listener and a getter event listener,
this allows to simply write a basic class that provides `getSchema`, `setValue` and `getValue` functions.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
…ve settings

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux added this to the Nextcloud 31 milestone Oct 15, 2024
@susnux susnux requested review from skjnldsv, provokateurin, a team, ArtificialOwl and come-nc and removed request for a team October 15, 2024 18:40
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh sure :D

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum wouldn’t listening to the three events with the same listener results in the same code more or less?

@susnux
Copy link
Contributor Author

susnux commented Oct 17, 2024

more or less

yes, but with "a lot" more boilerplate code (register event listener, wrapping things in the listener). And also potentially slower as all event listeners are called, no problem if only one app is using this, but if there are many apps then every app would be called to handle other settings.

For exapps this makes sense but for native PHP apps this I would say is cleaner.

@susnux susnux requested a review from come-nc October 20, 2024 12:53
@come-nc
Copy link
Contributor

come-nc commented Nov 4, 2024

more or less

yes, but with "a lot" more boilerplate code (register event listener, wrapping things in the listener). And also potentially slower as all event listeners are called, no problem if only one app is using this, but if there are many apps then every app would be called to handle other settings.

For exapps this makes sense but for native PHP apps this I would say is cleaner.

I don’t get it.

Why is it more code to register for the 3 events a listener rather than registering a "Declarative settings"?
The only boilerplate code is a switch on the event class to call the matching function?

"all event listeners are called", yes but also with your solution they are all called, no? I do not see the performance difference.

@skjnldsv skjnldsv merged commit 8fab143 into master Nov 6, 2024
177 checks passed
@skjnldsv skjnldsv deleted the feat/allow-getter-setter-decl-fors branch November 6, 2024 07:54
@susnux
Copy link
Contributor Author

susnux commented Nov 6, 2024

Why is it more code to register for the 3 events a listener rather than registering a "Declarative settings"?
The only boilerplate code is a switch on the event class to call the matching function?

Switch on event listener + registering the event listener in the first place.
But in general we have already two ways:

  1. Register by event -> This is still the case here I would also expect using events for get and set.
  2. Register a settings class -> We register a class, why should this class only provide the form but not the getter setter? No events needed.

"all event listeners are called", yes but also with your solution they are all called, no? I do not see the performance difference.

No the getValue and setValue terminate if the form was registered as a class with handlers.

@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Declarative Settings: Allow to get and set value inside of form
4 participants