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(web): search by filename #7624

Merged
merged 15 commits into from Mar 5, 2024
Merged

feat(web): search by filename #7624

merged 15 commits into from Mar 5, 2024

Conversation

alextran1502
Copy link
Contributor

@alextran1502 alextran1502 commented Mar 4, 2024

Added radio button to search for file's name

image

image

Copy link

cloudflare-pages bot commented Mar 4, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: a84dac1
Status: ✅  Deploy successful!
Preview URL: https://ed712adb.immich.pages.dev
Branch Preview URL: https://web-filename-search.immich.pages.dev

View logs

Copy link
Contributor

@michelheusschen michelheusschen left a comment

Choose a reason for hiding this comment

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

You should also add an entry to this object

function getHumanReadableSearchKey(key: keyof SearchTerms): string {
const keyMap: Partial<Record<keyof SearchTerms, string>> = {

@waclaw66
Copy link
Contributor

waclaw66 commented Mar 4, 2024

Wouldn't be better to not add another control and search filenames automatically? Is it because some performance reason?

@alextran1502
Copy link
Contributor Author

@waclaw66 SmartSearch and MetadataSearch are two different categories with separate API endpoints. File name search falls into the MetadataSearch category

@alextran1502 alextran1502 changed the title Toggle to search by filename feat(web): search by filename Mar 4, 2024
@mertalev
Copy link
Contributor

mertalev commented Mar 4, 2024

I feel like this should just be a toggle for smart / context vs full-text search. We don't want a radio button for each text field we support searching for

@waclaw66
Copy link
Contributor

waclaw66 commented Mar 4, 2024

@waclaw66 SmartSearch and MetadataSearch are two different categories with separate API endpoints. File name search falls into the MetadataSearch category

I see, however that control is something not interesting from user perspective, to much controls bring confusion. Please just take this as a kind reminder.

@alextran1502
Copy link
Contributor Author

@mertalev @waclaw66 both of you bring good points, let me think again how to best handle this situation

@mertalev
Copy link
Contributor

mertalev commented Mar 4, 2024

This is probably where the hybrid search stuff would be ideal. Then we could hit the smart search endpoint and not expose a separate control for this.

@mertalev
Copy link
Contributor

mertalev commented Mar 4, 2024

But it might have an effect on performance.

@aviv926
Copy link
Contributor

aviv926 commented Mar 4, 2024

Wouldn't be better to not add another control and search filenames automatically? Is it because some performance reason?

Does sending 2 API requests one for CLIP and one for MetadataSearch sound like something possible (or does it add a lot of load to the system)?

Or since a thumb file with a known extension is created for each file, maybe it can be used so that in every search it will search if there is a webp type file that matches [search] if so it will retrieve it in the search results.

builder.andWhere(_.omitBy(path, _.isUndefined));

if (options.originalFileName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should rename this since we search on the file path

builder.andWhere(_.omitBy(path, _.isUndefined));

if (options.originalPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this not support doing an exact match on original path anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Seems fine. Not a huge fan of this code change for search requiring an update to a method used for so many other features.

@alextran1502 alextran1502 merged commit 2f53f6a into main Mar 5, 2024
25 checks passed
@alextran1502 alextran1502 deleted the web/filename-search branch March 5, 2024 23:08
@aviv926
Copy link
Contributor

aviv926 commented Mar 6, 2024

Does it actually replace m: or a feature in addition to it?

@alextran1502
Copy link
Contributor Author

Does it actually replace m: or a feature in addition to it?

It is a replacement, there is no more m: syntax

@mertalev
Copy link
Contributor

mertalev commented Mar 6, 2024

As a feature it replaces m:, but it's specifically for file names right now.

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.

None yet

6 participants