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

Use metadata title in search results if present #1872

Closed
wants to merge 2 commits into from
Closed

Use metadata title in search results if present #1872

wants to merge 2 commits into from

Conversation

mill1000
Copy link
Contributor

Use metadata title in search results (if present) instead of the raw dc_title field. Closes #1867

obj->setMetadata(meta);

// Update tile from metadata if present
if (meta.count(MetadataHandler::getMetaFieldName(M_TITLE)))
Copy link
Contributor

Choose a reason for hiding this comment

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

find is a bool, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

to clarify, find() is faster than count() as it exits early.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

find() returns an iterator, so the proper form would be meta.find() != meta.end() IIRC. In the case of a std::map since all keys are unique I believe the performance would be equivalent.

Copy link
Contributor

@neheb neheb Aug 31, 2021

Choose a reason for hiding this comment

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

I checked this. Seems correct. I would still make the change as count() is not used in if statements in the code base.

edit: sorry, not used as a bool like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure no problem. I'll switch to find() later today

@KarlStraussberger
Copy link
Member

title is handled exactly the same way for search and browse (

for (auto&& cdsObject : results) {
). The text depends on the item in the virtual layout. If you want to change the output, this is the better place.

@mill1000
Copy link
Contributor Author

mill1000 commented Sep 2, 2021

Hi Karl. I don't agree that they are handled exactly the same way. My control point clearly displays different titles for search results vs browsing.

Ultimately, I don't care where this code goes. But I'd argue that it makes more sense for it to be located where the CdsObject is created. I'd even go to say that the above blocks of code should be moved into createObjectFromSearchRow and createObjectFromRow. Probably even broken out into it's own function since there are nearly identical.

Search
Screenshot_20210901-200841
Browse
Screenshot_20210901-200907

@KarlStraussberger
Copy link
Member

I double checked the code in sql_database and it returns the same column. I may happen that your endpoint uses metadata in case of browsing while the text displayed after search is the original set by the layout script on import. Please, try another client, like foobar, on your mobile.

@mill1000
Copy link
Contributor Author

mill1000 commented Sep 3, 2021

@KarlStraussberger I've tried BubbleUpnp on mobile, and foobar2000 on desktop. Both have the same behavior of showing the metadata title in browse, and the dc_title in search.

foobar2000
foobar search
foobar browse

BubbleUpnp
Screenshot_20210903-072623

@mill1000
Copy link
Contributor Author

mill1000 commented Sep 3, 2021

I also saw no ability to perform a search against Gerbera on foobar2000 mobile.

@KarlStraussberger
Copy link
Member

I still don't see where it happens, did you configure title-properties? (https://docs.gerbera.io/en/latest/config-server.html#upnp)

@mill1000
Copy link
Contributor Author

mill1000 commented Sep 4, 2021

I did not configure any additional properties. (Plus the documentation says title-properties can't be changed?)

It looks like the browse title is coming directly from the database. When I re-run the browse query on the DB directly I get the desired dc:title. This must be because browse is looking at the virtual items, while search (for tracks) is looking directly at the non-virtual items marked with object.item.audioItem.musicTrack.
browse query

So perhaps this PR was just a poorly thought out band-aid for a bigger issue. Along with the duplicate album issue in #1863. Perhaps this database structure just isn't ideal for search.

@KarlStraussberger
Copy link
Member

The real reason for the behaviour is that the title for the source items is determined from the file name.

I took your idea and implemented a configuration option for the title.

@mill1000 mill1000 deleted the issues/1867_search_title branch September 29, 2021 00:04
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.

Title displayed in search results does not match title displayed during browse
3 participants