Skip to content

Conversation

@danxuliu
Copy link
Member

OC.dirname removes everything after the last /, so a path without slashes is returned without changes. result.path does not include leading nor trailing /, so when the path is for a file or folder in the base folder OC.dirname(result.path) returns result.path instead of an empty string.

To test this just follow the steps in the acceptance tests open a search result for a comment in a file and open a search result for a comment in a folder named like its child folder (or just check the CI results :-P ).

There is another issue with opening search results for comments, though. This pull request fixes the link to the file, but theoretically clicking on a result should be intercepted before the link is opened, and instead of loading the new URL the file list should be simply updated with the desired file. However, setFileList is called only on the search provider of files, so there will be no file list set in the search provider for comments and thus the click event will be handled by opening the link.

Fixing that is more a structural change in the search (as the file list should be set for any search provider, it would not be enough to just set it explicitly on the search provider for comments), so that could be fixed for Nextcloud 16 in a different pull request if desired; this one is just a little bug fix (with its corresponding acceptance tests, of course ;-) ) to be backported to previous versions.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
"OC.dirname" removes everything after the last "/", so a path without
slashes is returned without changes. "result.path" does not include
leading nor trailing "/", so when the path is for a file or folder in
the base folder "OC.dirname(result.path)" returns "result.path".

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Copy link
Member Author

/backport to stable15

@danxuliu
Copy link
Member Author

/backport to stable14

@skjnldsv
Copy link
Member

@danxuliu where do you search for comments? 🤔

@danxuliu
Copy link
Member Author

@skjnldsv In the search box in the header of the Files app (the same place you use to search files).

@skjnldsv
Copy link
Member

TIL comments are part of the search provider 😅
We really need to fix that :(

@danxuliu
Copy link
Member Author

TIL comments are part of the search provider 😅
We really need to fix that :(

Well, I think that nothing needs to be fixed there ;-) If you are looking for a file it makes sense that you can look also for the file based on comments written on it.

@skjnldsv
Copy link
Member

TIL comments are part of the search provider sweat_smile
We really need to fix that :(

Well, I think that nothing needs to be fixed there ;-) If you are looking for a file it makes sense that you can look also for the file based on comments written on it.

Yes, you're totally right! 😉
It is the way the search is displayed that bother me. UX wise I'm sure no one knows you can search comments. #9917

@rullzer rullzer merged commit 027f69d into master Dec 24, 2018
@rullzer rullzer deleted the fix-opening-search-results-for-comments branch December 24, 2018 13:20
@backportbot-nextcloud
Copy link

backport to stable15 in #13253

@backportbot-nextcloud
Copy link

backport to stable14 in #13254

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.

4 participants