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

Adding profile support #6

Closed
wants to merge 12 commits into from
Closed

Adding profile support #6

wants to merge 12 commits into from

Conversation

Beanow
Copy link
Collaborator

@Beanow Beanow commented Mar 28, 2022

Fixes emilsvennesson#34

Making a draft PR to get your feedback on this so far. It adds "Select profile" and "Log out" to the settings.
(As well as the user_id and profile_id, while it's in development, final would hide those.)

image

It will automatically ask to choose a profile when opening the root page, if you haven't already.
Kind of like the "Who's watching?" dialog on the official apps.

An easy way to verify your profile is correctly selected, is to see if viaplay:starred or viaplay:watched has the correct items.

@Mariusz89B @heppen-dev @zuzia-dev
What's your opinion on using the settings menu like this, instead of adding it to the root page?

@Beanow Beanow self-assigned this Mar 28, 2022
@heppix
Copy link

heppix commented Mar 28, 2022

Yeah, I'm fine with it.
It's much better approach than listing it on root page.
About that window. I assume it's a custom dialog, right?
If so, we can make it look kind of better, even trying to match the current window from official app.

#edit
Oh, I see. It's not a custom dialog. My bad :)

@Beanow
Copy link
Collaborator Author

Beanow commented Mar 28, 2022

I assume it's a custom dialog, right? If so, we can make it look kind of better, even trying to match the current window from official app.

#edit Oh, I see. It's not a custom dialog. My bad :)

It's a https://romanvm.github.io/Kodistubs/_autosummary/xbmcgui.html#xbmcgui.Dialog.select
https://github.com/Mariusz89B/kodi-viaplay/blob/d3a51dbd4aaf19d10a23475e9fe48a0192080a02/resources/lib/kodihelper.py#L92

Maybe xbmcgui.ListItem.setArt would allow the avatars to display here? 🤷

@zuzia-dev zuzia-dev requested a review from heppix March 28, 2022 15:45
@Beanow
Copy link
Collaborator Author

Beanow commented Mar 28, 2022

Looks like we could use the icons, if we use useDetail=True for the dialog.
That means we get list items about 3 lines high, a second label option, and icon support.

image

@heppix
Copy link

heppix commented Mar 28, 2022

Looks like we could use the icons, if we use useDetail=True for the dialog. That means we get list items about 3 lines high, a second label option, and icon support.

image

Ok, I see. This looks good. The second line could be last login if possible.
I just checked and we have "updated" key in persistentLogin endpoint. Maybe it will be better than ID in my opinion.

@Beanow
Copy link
Collaborator Author

Beanow commented Mar 28, 2022

@heppen-dev yeah I just tried out the type: adult|child field, and that looks useful. The updated field though is when the profile itself was last updated. Mine shows March 18th for example, while I used that profile multiple times this weekend.

Ones that look interesting to me are:

{
  "isOwner": true,
  "type": "adult",
  "restricted": false,
  "language": "nl",
}

@heppix
Copy link

heppix commented Mar 28, 2022

@heppen-dev yeah I just tried out the type: adult|child field, and that looks useful. The updated field though is when the profile itself was last updated. Mine shows March 18th for example, while I used that profile multiple times this weekend.

Ones that look interesting to me are:

{
  "isOwner": true,
  "type": "adult",
  "restricted": false,
  "language": "nl",
}

Basically this 3 infos are usefull:

  • isOwner
  • type
  • lang
    We can combine it somehow.

Just say when you are ready and we can merge :)

@Mariusz89B
Copy link
Owner

Looks like we could use the icons, if we use useDetail=True for the dialog. That means we get list items about 3 lines high, a second label option, and icon support.

image

This would be the simplest way to implement a profile screen, you could also use the preselect method if there is a main account that you would like to highlight.

@Beanow
Copy link
Collaborator Author

Beanow commented Mar 28, 2022

@heppen-dev like so?

image

Only downside to this is, all of these values are internal ones from the API (not labels). We'd have to translate them ourselves.

@Mariusz89B Yeah I spotted the preselect, I've got that already implemented as, the current profile or none preselected.
(That's the green highlight)

@Beanow
Copy link
Collaborator Author

Beanow commented Mar 28, 2022

@heppix
Copy link

heppix commented Mar 28, 2022

It seems, profile does not works well with different Viaplay versions. We need more test before merge.

@Beanow
Copy link
Collaborator Author

Beanow commented Mar 29, 2022

@heppen-dev do you mean on other countries' sites? Which ones are you trying and what does it do? Silently ignore profiles or errors?

@Mariusz89B
Copy link
Owner

Mariusz89B commented Mar 29, 2022

@heppen-dev do you mean on other countries' sites? Which ones are you trying and what does it do? Silently ignore profiles or errors?

A solution would be to set a single user as an exception if there is missing information in the json dictionary and it's silently ignoring profiles.

@Mariusz89B Mariusz89B marked this pull request as ready for review April 23, 2022 06:18
@heppix
Copy link

heppix commented Apr 24, 2022

Hey @Beanow we have added profiles support to our repository with @zuzia-dev
Since I was based kind of on your solution, added you to providers and changelog. Hope you are fine with it.
Also, I solved the issue with different sites versions. Profiles should works everywhere.
You can find our version of Viaplay in https://heppen-dev.github.io repository.

@Beanow
Copy link
Collaborator Author

Beanow commented Apr 24, 2022

@heppen-dev your version seems to work with NL as well.
What did the issue end up being for other sites?

Closing this, as your fixes seem preferable.

@Beanow Beanow closed this Apr 24, 2022
@heppix
Copy link

heppix commented Apr 24, 2022

There was a KeyError related to language key in response. It's not exist in some sites.

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.

Profile support
3 participants