jellyfin / jellyfin-web

Web Client for Jellyfin
https://jellyfin.org
GNU General Public License v2.0
2.37k stars 1.26k forks source link

Identify TMDB Collections Block Mislabeled #900

Closed Artiume closed 4 years ago

Artiume commented 4 years ago

We get double the metadata!

image

Artiume commented 4 years ago

Interesting behavior, using a TMDB link without a name will return 0 results.

Also, let's say using only the name returns 20 results. Using Name + TMDB block 1 will return 10 names. Using Name + TMDB block 2 will return 20 results (seems to be the false block).

Artiume commented 4 years ago

Duplicate of https://github.com/jellyfin/jellyfin-web/issues/808 but the other ticket got no love

dmitrylyzo commented 4 years ago

I think we discussed that somewhere (on Matrix?). They are different but named incorrectly. The first one is "Tmdb". The second is "TmdbCollection".

Artiume commented 4 years ago

Interesting, didn't know about the tmdb collections

dmitrylyzo commented 4 years ago

Just for note

Names are generated here https://github.com/jellyfin/jellyfin-web/blob/2a99df8365993c0d0f02f80f11f6bd20fb697829/src/components/itemidentifier/itemidentifier.js#L309 LabelDynamicExternalId is "{0} Id:"

Server returns

[
    {
        "Name":"IMDb",
        "Key":"Imdb",
        "UrlFormatString":"https://www.imdb.com/title/{0}"
    },
    {
        "Name":"TheMovieDb",
        "Key":"Tmdb",
        "UrlFormatString":"https://www.themoviedb.org/movie/{0}"
    },
    {
        "Name":"TheMovieDb",
        "Key":"TmdbCollection",
        "UrlFormatString":"https://www.themoviedb.org/collection/{0}"
    }
]
Artiume commented 4 years ago

Can you break down the js for that line? I'm trying to get that eureka moment

dkanada commented 4 years ago

It looks like the server isn't returning the proper names for these providers. If someone can get a list of all instances of broken provider names I know where to look for a fix.

heyhippari commented 4 years ago

Can you break down the js for that line? I'm trying to get that eureka moment

When it generates the translation string's name for the input's label, it is essentially going over the list of fields returned by the server and using the name provided to complete the string's name.

As the name returned by the server isn't right, it's not showing the correct translation. This is a server-side fix (with maybe a translation edit on this side).

dmitrylyzo commented 4 years ago

"Name" in server response is name of metadata provider - I think this is valid because we can use it as a marker. The problem is that "Key" value is not used. Or server should return full label in additional field "Label".

For TMDB. First field: https://github.com/jellyfin/jellyfin/blob/cbd1e924866cef8aae12ee9afb4342041337f0de/MediaBrowser.Providers/Tmdb/Movies/TmdbMovieExternalId.cs#L7-L18 https://github.com/jellyfin/jellyfin/blob/cbd1e924866cef8aae12ee9afb4342041337f0de/MediaBrowser.Providers/Tmdb/People/TmdbPersonExternalId.cs#L5-L16 https://github.com/jellyfin/jellyfin/blob/cbd1e924866cef8aae12ee9afb4342041337f0de/MediaBrowser.Providers/Tmdb/TV/TmdbSeriesExternalId.cs#L5-L16

Second field: https://github.com/jellyfin/jellyfin/blob/cbd1e924866cef8aae12ee9afb4342041337f0de/MediaBrowser.Providers/Tmdb/BoxSets/TmdbBoxSetExternalId.cs#L6-L17

Some stupid idea Join LabelDynamicExternalId and Key to get

And then translate them. But I am not totally satisfied with their translation on jf-web side - someday they will be removed because of no reference :smile: https://github.com/jellyfin/jellyfin-plugin-autoorganize/pull/31#discussion_r387883121

IMO, all providers are here https://github.com/jellyfin/jellyfin/blob/cbd1e924866cef8aae12ee9afb4342041337f0de/MediaBrowser.Model/Entities/MetadataProviders.cs#L9-L40

heyhippari commented 4 years ago

ome stupid idea Join LabelDynamicExternalId and Key to get

* LabelDynamicExternalIdImdb

* LabelDynamicExternalIdTmdb

* LabelDynamicExternalIdTmdbCollection

* LabelDynamicExternalIdTvdb

* LabelDynamicExternalIdTvcom, etc

This is actually probably how it was meant to work.

It makes sense as well, don't use the name of the provider but the key of the field.

Now, @dkanada just has to check all the other repositories before going on a string cleaning rampage :D

dkanada commented 4 years ago

Nope, technically the name is supposed to be used directly with no translations. I think @Bond-009 or someone made this change a while back during a refactor, so what we need to do is update the names on the server.

dkanada commented 4 years ago

The key itself is only used as a unique identifier. Many of the interfaces (scheduled tasks, plugins, notifications) have the same structure. The key is for identification only, and the other fields are usually translated by the server and sent to clients as metadata, although I have often questioned whether the server should be translating anything.

Artiume commented 4 years ago

One thing I noticed was if I put only the tmdb or imdb reference and no name in the identify block, it would return 0 results, it's dependent upon having a name.

A bit off topic but I've been stratching my head at this one for a while. I'm trying to figure out how to point the metadata grabbers to the livetv content. https://github.com/jellyfin/jellyfin/issues/2336

mark-monteiro commented 4 years ago

EDIT: @lfoust has broken down his approach here and it seems to makes sense to me, so I think my idea below can be disregarded


If the translation is going to be done client-side, I think this should be implemented something like this. Though I think this would still require @dkanada to be careful on his string cleaning rampages ;)

var providerName = globalize.translate("someprefix" + idInfo.Key); 
var idLabel = globalize.translate("LabelDynamicExternalId").replace("{0}", providerName); 

It seems like we were leaning towards doing a client-side translation based on the discussions here:

dkanada commented 4 years ago

We will definitely not be generating any more string keys dynamically on the client side, because that's a terrible idea that won't work well once we start using a more modern toolset.

mark-monteiro commented 4 years ago

We will definitely not be generating any more string keys dynamically on the client side, because that's a terrible idea that won't work well once we start using a more modern toolset.

@dkanada I'm not familiar with the limitations of the translation framework. The "someprefix" in my example isn't mandatory, but to translate this client side you would still have to use some sort of key variable from the server like globalize.translate(idInfo.TranslationKey); Is that okay, or should only static strings be passed to the translate() function?

heyhippari commented 4 years ago

@dkanada I'm not familiar with the limitations of the translation framework.

Good thing, because I'm fully intending to rip it away at some point and use something like i18next. :)

mark-monteiro commented 4 years ago

Good thing, because I'm fully intending to rip it away at some point and use something like i18next. :)

Ugh, I should have know the existing framework was some custom nonsense 🤦‍♂

I am asking about the limitations with using a dynamic key, because the PR to solve this issue specifically uses a dynamic value to translate and I don't understand what the issue is with that: https://github.com/jellyfin/jellyfin-web/pull/971/files#diff-3a46506e1b5bc073dbe2e15ed553a23fR309

stale[bot] commented 4 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh by adding a comment or commit. Stale issues close after an additional 14d of inactivity. If this issue is safe to close now please do so. If you have any questions you can reach us on Matrix or Social Media.

mark-monteiro commented 4 years ago

Shoo bot

heyhippari commented 4 years ago

Fixed as of 10.6.