Skip to content

Conversation

@tomasz-grobelny
Copy link

Idea behind this pull request is twofold:

  1. Separate controller from the actual logic of extracting metadata. This will allow the service to be used in other parts of nextcloud (eg. indexing).
  2. Add support for some commonly used fields (subject, rating). These are used by Windows or F-Stop gallery on Android.

@gino0631
Copy link
Owner

gino0631 commented Aug 3, 2018

Any more info on how this will be used in indexing (or other parts of Nextcloud)?

@tomasz-grobelny
Copy link
Author

How this will be used in indexing is not completely clear to me as well. What I want to achieve is the full workflow where I take pictures, rotate and classify them on tablet using F-Stop, upload to nextcloud and I am able to search by tags. Other features along the way are a bonus (like support for Rating field), but I want to focus on enabling the complete basic scenario without getting into too many details.

This means that at least one other app needs to be modified: files_fulltextsearch, but maybe also core fulltextsearch. Frankly speaking this is my first php pull request ever so I hope it all makes sense. Getting back to fixing the tests...

@tomasz-grobelny
Copy link
Author

Any chance to have my changes merged or do you think some further changes should be made before pull request is ready for inclusion?

@gino0631
Copy link
Owner

gino0631 commented Aug 6, 2018

I have some local changes, as I was fixing #15, and I would like to finish with that before starting with other features.

As for the search, the request is clear, and is registered as #6, but I can't see how the proposed changes should help to achieve the result. I would like to understand the design first, before starting with the changes. Of course, if they appear useful, I will integrate them.

In any case, thank you for the contribution.

 - tweak interface to be able to use MetadataService from other parts of Nextcloud
@tomasz-grobelny
Copy link
Author

tomasz-grobelny commented Aug 10, 2018

For how this can be used in search please have a look here:
nextcloud/files_fulltextsearch#17
nextcloud/fulltextsearch#358

Does it make sense now?

@tomasz-grobelny
Copy link
Author

I see that you have integrated some of my changes already in your code (eg. dc:subject), but in a slightly different way (which might be better). Still the core part that I need is the service/controller separation. I think it makes more sense to create a separate PR with only this change. See #18. What do you think about the new PR?

@gino0631
Copy link
Owner

I'm not sure if it's OK for other apps to depend on this app (which might be event not installed) this way, and was thinking about indexing in the app itself, however, if the authors of the other apps confirm they wish to integrate as proposed, I will merge the relevant changes.

@tomasz-grobelny
Copy link
Author

Ok, I'll try to get some feedback from fulltextsearch guys. Two points here:

  1. I think that the separation makes either way. It is just a step towards following SRP.
  2. I totally agree that fulltextsearch should work without metadata being installed. I haven't checked that yet, but my point until now was to have a PoC to show how change in metadata is useful in practical way.

@gino0631
Copy link
Owner

Support for keywords was registered as #13; after implementing that feature I realised your changes already included a similar one. Also, I have reviewed the changes and integrated some smaller ones, but for the major ones I would like to have more time for testing. I agree they may be useful, but let's wait for the feedback from the maintainers of fulltextsearch.

@tomasz-grobelny
Copy link
Author

Independent from the feedback, could you also have a look whether my changes from PR#18 work for you? I noticed that the only fulltextsearch maintainer (daita) is not very active last few days... :-(

@gino0631
Copy link
Owner

I'll take a look, but I also have some real-life jobs, and a few days is not much :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants