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

Extend reference API for (future) frontend picker #35557

Merged

Conversation

julien-nc
Copy link
Member

refs #31667

We forgot something in the specs: the fontend only knows the list of registered widgets to render references. We need more information for the available providers (title, icon etc...) to feed the reference picker. I can think of 2 ways to go:

  • provide more information when registering the reference widget in the frontend (feels weird because widgets are just for rendering, the picker stuff should be independent)
  • add a new "registerProvider" function to vue-richtext
  • add an API endpoint to get info on the available providers

I went with the API endpoint. (Or did I miss something obvious?)

  • 2 new interfaces:
    • IDiscoverableReferenceProvider for reference providers that can be discovered by the frontend
    • ISearchableReferenceProvider for discoverable ones that support search
  • 1 new method to get those providers in ReferenceManager
  • 1 new route/controller method

So, a reference provider can support multiple search providers. Thinking out loud: We need to be careful to properly render heterogeneous search results in the frontend.

@julien-nc julien-nc added the 3. to review Waiting for reviews label Dec 2, 2022
@julien-nc julien-nc added this to the Nextcloud 26 milestone Dec 2, 2022
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@julien-nc
Copy link
Member Author

Oh I forgot to mention: in the reference-discovery of integration_github those 2 new provider interfaces are implemented.

@julien-nc julien-nc force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch from b128634 to d1eb33b Compare December 2, 2022 17:16
@julien-nc julien-nc force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch from d1eb33b to 7ef7268 Compare December 5, 2022 12:00
@julien-nc
Copy link
Member Author

@juliushaertl Where can we listen to RenderReferenceEvent to provide the provider list as an initial state?

In other words, in server, where can we call (and where does it makes sense to call) IRegistrationContext::registerEventListener(RenderReferenceEvent::class, ...) ?

I've seen similar-ish stuff in lib/base.php but it didn't feel right/straightforward to put it there...

@juliushaertl
Copy link
Member

juliushaertl commented Dec 5, 2022

Same feeling that there could be a better place but I also added one to base.php in the first PR:

private static function registerFileReferenceEventListener() {

So maybe we just stick with that for now.

@julien-nc
Copy link
Member Author

Thanks, gotta problem though. Just added 311527e

I don't know why but I can't inject IInitialState in the listener class. Server logs say:

Could not load event listener service OC\\Collaboration\\Reference\\RenderReferenceEventListener:
Could not resolve initialStateService! Class \"initialStateService\" does not exist. 
Make sure the class is auto-loadable by the Nextcloud server container

But initialStateService is just the name of the listener class attribute 🤔

I don't get why IReferenceManager can be injected just fine. I tried to use \OC::$server->get() in the listener's handle() method, same problem.

I also tried injecting \OCP\Server (which works) and then use it to get IInitialState. Slightly better error log message but still no luck:

Could not resolve OCP\\AppFramework\\Services\\IInitialState! Class can not be instantiated

Any idea?

@juliushaertl
Copy link
Member

IInitialState is only available within an apps context through the apps DI container.

However you can inject the IInitialStateService directly e.g. as in

$this->initialStateService->provideInitialState('core', 'config', $config);

@julien-nc julien-nc force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch from 311527e to e0f51b3 Compare December 7, 2022 00:16
@julien-nc
Copy link
Member Author

@juliushaertl Thanks. Done, works.

I thought base.php was in the 'core' app context 😁.

I didn't try to inject IInitialStateService because it's deprecated. Any problem with that?

@juliushaertl
Copy link
Member

I didn't try to inject IInitialStateService because it's deprecated. Any problem with that?

At least there is no replacement yet for core parts that need to inject initial state. And the new interface just wraps the deprecated, so while apps should use the new one, the deprecated one should still be fine within the affected core code base.

@julien-nc julien-nc force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch 2 times, most recently from f80bb98 to c3b8e1c Compare December 20, 2022 11:14
@julien-nc julien-nc force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch from 783e49c to 13ab5cc Compare December 20, 2022 12:54
@julien-nc
Copy link
Member Author

I don't get the remaining psalm check failure:

Error: lib/private/Collaboration/Reference/RenderReferenceEventListener.php:45:21: 
InvalidArgument: Incompatible types found for T 
(OCP\Collaboration\Reference\RenderReferenceEvent is not in OCP\EventDispatcher\Event)
(see https://psalm.dev/004)

It's fine for

$eventDispatcher->addServiceListener(NodeDeletedEvent::class, FileReferenceEventListener::class);
$eventDispatcher->addServiceListener(ShareDeletedEvent::class, FileReferenceEventListener::class);
$eventDispatcher->addServiceListener(ShareCreatedEvent::class, FileReferenceEventListener::class);

@julien-nc julien-nc force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch from 13ab5cc to ce43b6f Compare December 20, 2022 16:06
@julien-nc julien-nc force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch 2 times, most recently from e6c2573 to 34893d7 Compare January 5, 2023 11:53
@julien-nc julien-nc force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch from 34893d7 to abb9a46 Compare January 11, 2023 15:45
@julien-nc julien-nc force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch from d6cc645 to 39ac533 Compare January 19, 2023 10:03
@julien-nc
Copy link
Member Author

@juliushaertl This is ready for review IMO. Nothing more to add for now.

Some recent additions to this PR:

  • a new OCS API endpoint to update the "last used timestamp" of a provider
  • inject the provider timestamps with initial state on RenderReferenceEvent

@juliushaertl
Copy link
Member

Pushed a fix for the psalm error

@julien-nc julien-nc force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch from 3b2fe91 to ea66a7f Compare January 24, 2023 11:34
@julien-nc julien-nc force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch from ea66a7f to 15e0608 Compare January 26, 2023 12:21
@julien-nc
Copy link
Member Author

@marcelklehr @juliushaertl Thanks for the review.

@juliushaertl juliushaertl mentioned this pull request Jan 26, 2023
13 tasks
@juliushaertl juliushaertl force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch from 15e0608 to cf39328 Compare January 26, 2023 21:47
@juliushaertl
Copy link
Member

Rebased and squashed the fixup commits and updated the autoloader.

@juliushaertl juliushaertl force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch from 28f2363 to fbc249e Compare January 26, 2023 22:09
@juliushaertl juliushaertl force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch from 4eab4b5 to ff79b40 Compare January 27, 2023 07:34
julien-nc and others added 8 commits January 27, 2023 11:10
- add 2 interfaces for discoverable and searchable reference providers
- new OCS route to get info on discoverable/searchable reference providers
- new abstract ADiscoverableReferenceProvider that only implements jsonSerialize
- listen to RenderReferenceEvent to inject provider list with initial state

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl juliushaertl force-pushed the enh/31667/extend-reference-api-for-frontend-picker branch from ff79b40 to 81c2122 Compare January 27, 2023 10:11
@juliushaertl juliushaertl added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 27, 2023
@juliushaertl juliushaertl merged commit a63b557 into master Jan 27, 2023
@juliushaertl juliushaertl deleted the enh/31667/extend-reference-api-for-frontend-picker branch January 27, 2023 11:35
@nickvergessen
Copy link
Member

Please document in #34692

@nickvergessen nickvergessen added the pending documentation This pull request needs an associated documentation update label Jan 29, 2023
@blizzz blizzz mentioned this pull request Feb 1, 2023
@DaphneMuller
Copy link

hello @julien-nc
Thank you so much for your work on this pull request! This ticket seems to have the tag 'pending documentation'.
Is there any chance you could clarify what documentation is missing? Is this for admins or for app developers?

@DaphneMuller
Copy link

Hello!
Thank you for your work on this! I noticed that your PR still has the label 'documentation'. I'm not sure if you already had a look at the documentation for your PR, but at Nextcloud we strive to document changes that affect other app developers or other admins before the release takes place, and I'm pinging you with the hope to get this done.

Are you familiar with our documentation process already?
Changes that affect app developers should be documented here:
https://docs.nextcloud.com/server/27/developer_manual/app_publishing_maintenance/app_upgrade_guide/upgrade_to_27.html

Changes that affect administrators should be documented here:
https://docs.nextcloud.com/server/latest/admin_manual/release_notes/upgrade_to_27.html

While I understand sometimes it's handy to merge fast to get your changes in, it would be great if next time you could try to add the documentation before merging. You can find more information on that here:
https://docs.nextcloud.com/server/27/developer_manual/prologue/compatibility_app_ecosystem.html#documentation-procedures-of-changes-that-affect-app-developers

If all your documentation efforts are done, please remove the label 'pending documentation' and check the checkbox in your opening note and I'll stop bugging you :)

Many thanks in advance and thanks again for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants