maweki / twitchlive-extension

TwitchLive-Panel Gnome Shell Extension displaying your favorite streamers
GNU General Public License v3.0
38 stars 8 forks source link

Live channels aren't shown if I follow a user with a space in their display name. #63

Closed ivy-one closed 4 years ago

ivy-one commented 4 years ago

Examples of such users:

I'm using the feature "Import from twitch users" to get all my follow. May I suggest to remove space from the name at this stage? Or use IDs to get live channels?

maweki commented 4 years ago

Thank you for the report. We probably just do not encode the space correctly.

See: https://github.com/maweki/twitchlive-extension/blob/master/api.js#L56-L67

We just send the names via GET. I think we need to urlencode them beforehand, to have the correct spaces.

maweki commented 4 years ago

@ivy-one I currently can't try it out. Maybe you can try master where I have added what I think is a fix. If this works, I do a new release to ego

ivy-one commented 4 years ago

It didn't work unfortunately. The API returns an error. curl -H 'Client-ID: 4yzkpoa13a9zqepwguxejohaqulrgbu' -X GET 'https://api.twitch.tv/helix/users?login=CD%20PROJEKT%20RED' {"error":"Bad Request","status":400,"message":"Invalid login names, emails or IDs in request"}

I suggested to remove space because that's probably the biggest change people can do to their display name. In fact as a non partner I can only change the capitalisation to my display name.

maweki commented 4 years ago

Thank you for trying. I'll revert the change and look into it a bit more deeply

ghost commented 4 years ago

Hello friends :wave:

Since I found this plugin a couple of months ago I have loved to use it. So thank you @maweki for making it. However I also encountered this bug. At first I did a quick fix like the one ivy-one suggests here. Just string regex replace all spaces in the STREAMERS variable with the empty string.

However that felt like cheating. So I looked into why this happens in the first place so here we go.

When you import users from your followers you are using this function: https://dev.twitch.tv/docs/api/reference/#get-users-follows

in the documentation it says that the name that returns is the display name. You save this as if that was the login field of the user account which is not correct. So then when it tries to use that value as a login the API returns an error in cases when the display name does not mirror the login. In the documentation for this function:

https://dev.twitch.tv/docs/api/reference/#get-users

It says that you can call it with either an identifier or the login. In the get-users-follows function you get the display name and the identifier.

So maybe we should save both? identifier1:displayName1,identifier2:display Name2,... Or perhaps in two different lists. StreamerIdentifiers and StreamerDisplayNames.

I guess that would be the safest approach to save both because I don't think there is a strict requirement for the display name to be equal to the login for those channels that can change the display name. It is the most complete solution compared to doing like here below(even though it is very tempting to make this hack of a solution):

extension.js const streamersList = STREAMERS.map((d) => d.replace(/ /g,"")).filter((d) => d != "");

By replacing the trim() invokation that just trims the leading and trailing white spaces with a replace all white spaces regex we avoid having to update the API and schema and all that stuff but leaves the STREAMER variable still holding psuedo safe data for the API. Data where we assume that the letters in the display name MUST BE EQUAL to the login... maybe that shortcut is worth it until we find that it creates a problem. I actually don't know. I mean I could start implementing so that the application saves both the identifier and display name to be safe in that regard but I think I would like some input on that before doing so.

Have a nice day everyone! :hugs:

maweki commented 4 years ago

As you correctly pointed out, we have to use the get-user function which we already have in our api file. The correct solution would just be piping the result of follows (https://github.com/maweki/twitchlive-extension/blob/master/api.js#L71) through the login-resolution (https://github.com/maweki/twitchlive-extension/blob/master/api.js#L47).

I do not think that this would be a lot of code. Also, I am happy you like the extension.

ghost commented 4 years ago

Yes! that also works :smiley:. That's the best least intrusive approach to the existing API. Should just be a few lines. :+1:

Have a nice day

maweki commented 4 years ago

I think I fixed this now. Can you try master? We now only use login names (which you can retrieve through your follows)

ivy-one commented 4 years ago

Yes, it works. I just needed to update this line to class NobodyMenuItem extends PopupMenu.PopupBaseMenuItem { , otherwise the pop up don't appear.

maweki commented 4 years ago

Thanks for the info. I'll push that change right away and will do a proper release as soon as possible.

maweki commented 4 years ago

This is fixed and works now. :)