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

Unified search #600

Merged
merged 2 commits into from
Oct 1, 2020
Merged

Unified search #600

merged 2 commits into from
Oct 1, 2020

Conversation

korelstar
Copy link
Member

@korelstar korelstar commented Aug 27, 2020

Screenshot

Screenshot Screenshot Dark Mode

Open Questions

  • Which order index should we use? This is interesting, because notes will be shown by the notes provider and by the files provider. However, the files provider will open a note using the Text app and not using the Notes app. Therefore, it would be advantageous if notes will be shown first and files below. But I doubt that this is not desired.
  • Which icon should we use? For now, I use the app icon. Is this okay? Any other ideas?

@nextcloud/notes @nextcloud/designers

@korelstar korelstar added the enhancement New feature or request label Aug 27, 2020
@korelstar korelstar added this to the 3.7.0 milestone Aug 27, 2020
@skjnldsv
Copy link
Member

  • Which order index should we use? This is interesting, because notes will be shown by the notes provider and by the files provider. However, the files provider will open a note using the Text app and not using the Notes app. Therefore, it would be advantageous if notes will be shown first and files below. But I doubt that this is not desired.

Maybe 40 nextcloud/server#22120 (comment) ?

@tnyeanderson
Copy link
Contributor

I think the app icon looks good. Can we color-match it to the user's theme or is that a bad idea? If anything, just make it a dark gray because the dark black is a little jarring.

@nickvergessen
Copy link
Member

I think the app icon looks good. Can we color-match it to the user's theme or is that a bad idea? If anything, just make it a dark gray because the dark black is a little jarring.

Well black is better as that inverts to white for dark-theme then. Also it's consistent with all other apps.

@tnyeanderson
Copy link
Contributor

I did not think about inverting for dark mode, that is a great point. Matching the user theme is probably not a good idea.

Looking through my app icons in the search results, I see .wav, .als, .html, .mp3, .vcf files are all a gray color. Maybe I'm missing something here. The gray can invert to a darker white as well, no? It's just easier on the eyes and looks much more clean. A stark black looks "outdated" if you know what I mean.

@korelstar korelstar mentioned this pull request Sep 2, 2020
Base automatically changed from nc20 to master September 15, 2020 18:49
@korelstar korelstar force-pushed the unified-search branch 2 times, most recently from 59eb6e6 to 3d29e99 Compare September 16, 2020 06:22
@korelstar korelstar marked this pull request as ready for review September 16, 2020 06:30
@korelstar
Copy link
Member Author

Thanks for you feedback, also in the Talk channel. I've implemented your suggestions and updated the screenshot. Time for a final review 😃

} elseif (strpos($route, Application::APP_ID . '.') === 0) {
return -1;
}
return 4;
Copy link
Member

Choose a reason for hiding this comment

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

Default is far too high.
Do not inject yourself above default nextcloud apps please


public function getOrder(string $route, array $routeParameters): int {
if (strpos($route, 'files' . '.') === 0) {
return 25;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you put yourself lower when you're on files?

Comment on lines +47 to +52
if (strpos($route, 'files' . '.') === 0) {
return 25;
} elseif (strpos($route, Application::APP_ID . '.') === 0) {
return -1;
}
return 4;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (strpos($route, 'files' . '.') === 0) {
return 25;
} elseif (strpos($route, Application::APP_ID . '.') === 0) {
return -1;
}
return 4;
if (strpos($route, Application::APP_ID . '.') === 0) {
return -1;
}
return 40;

Copy link
Member

Choose a reason for hiding this comment

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

So yeah, as korel already mentioned. Notes are files, but the notes app can present them better. So it should be the prefered app when you search anywhere over files, hence the 4 in general, because it should be before files which has 5. Then again on files you might prefer the files search (or not) which is why 25 was picked there, and -1 when in notes.

@korelstar
Copy link
Member Author

@skjnldsv This was attuned via https://cloud.nextcloud.com/call/b8rjbqp6 :

Joas Schilling @nickvergessen 28.08.2020 08:08:

although notes sounds like a different case, because of how the info is opened. So maybe when you are on the files app you use something after files, when on any other app 4 and in notes -1

Jan C Borchardt @jancborchardt 09.09.2020 14:50:

Late to the party as I was on vacation, but Joas Schilling feedback sounds good for me too :)

I asked if this is fine for everybody and nobody disagreed. But we can restart the discussion again, if you want 😐. The start was this:

Which order index should we use? This is interesting, because notes will be shown by the notes provider and by the files provider. However, the files provider will open a note using the Text app and not using the Notes app. Therefore, it would be advantageous if notes will be shown first and files below. But I doubt that this is not desired.

What's your suggestion to deal with this problem? @skjnldsv

@tnyeanderson
Copy link
Contributor

@korelstar thank you for the changes to the icons... it looks really great!

For the result index... I definitely agree that our results need to be above the "Files" results. I think that putting a higher priority index is the best short term solution for that. This might be a more long term strategy (if it's not already implemented) but perhaps there should be a way to set one's application as the default for files based on type or location. So "All txt files in this folder should be listed to open with Notes"... so more granular than just overall indexes, because I think that's a recipe for confusion/disaster (as seen here, there are common cases where very "high" priority nextcloud apps need to be superceded by user apps... and I understand @skjnldsv hesitance because of this).

Obviously this would have to be implemented in nextcloud search itself, and would take a while to plan/implement if it ever is even considered, so we have to have a stopgap in the meantime.

@jancborchardt
Copy link
Member

Ah btw @korelstar same as for the Dashboard widget #614 (comment) – it would be cool if the subline would be the beginning of the note content instead of the category. :)

@korelstar
Copy link
Member Author

I implemented the last improvements (e.g. show note's excerpt instead of category @jancborchardt ) and I will merge this now, due to the pending major release.

@skjnldsv Sorry that I can't wait for your answer, but we let's try it with the current implementation and fix it afterwards, if there is a problem with it.

@korelstar korelstar merged commit 46a6bd1 into master Oct 1, 2020
@korelstar korelstar deleted the unified-search branch October 1, 2020 19:42
@raimund-schluessler
Copy link
Member

You can still filter notes by the unified search input, server emits an event when the search string is updated, see e.g. https://github.com/nextcloud/tasks/blob/master/src/main.js#L90-L96

@korelstar
Copy link
Member Author

@raimund-schluessler Thanks for the hint, I will have a look into that. But client-side filtering didn't work anymore for some time, since we do not load the mote's content for all notes anymore. Hence, searching/filtering must be realized on behalf of the server. Therefore, I think the current approach is fine, at least for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search-filter for notes list doesn't use the notes' contents [$20]
6 participants