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(ui): show account name and image on main settings page #1001

Merged
merged 7 commits into from
Apr 22, 2024

Conversation

IndusAryan
Copy link
Contributor

image

${VideoDownloadManager.downloadProgressEvent.size}") **/

// Check login status for each OAuth2API
val accountProfile = accountManagers.firstOrNull { it.loginInfo() != null }
Copy link
Collaborator

Choose a reason for hiding this comment

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

edge case where you have a login but not pfp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this case it will load pfp of the local account. and name also ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

no, lets say you have a non null loginInfo, but that loginInfo does not have a pfp then we are fucked

Copy link
Contributor Author

@IndusAryan IndusAryan Mar 23, 2024

Choose a reason for hiding this comment

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

i am not understanding

image

the loginInfo() returns null when NOT logged in, and we are already using that form

image

?

Copy link
Collaborator

Choose a reason for hiding this comment

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

assume that accountmanagers is a list of

listof(LoginInfo(
pfp = null,
name = "Hello",
accountIndex = 0
),
LoginInfo(
pfp = null,
name = "World",
accountIndex = 1
))

What would your code do? It would NOT work as it passes the accountProfile check (because it has more than one account), but does not in fact have an image, and because of this val pic = login?.profilePicture ?: continue will be trigged for both and you will get NOTHING.

app/src/main/res/layout/main_settings.xml Outdated Show resolved Hide resolved
@IndusAryan
Copy link
Contributor Author

pushed the changes and tested by creating a anilist account, before login and after logout :

image

after anilist login

image

when i created account there is a default avatar image if we dont have a pfp , and i think that will be same in simkl and mal, so not much of an issue.

@Luna712
Copy link
Contributor

Luna712 commented Mar 25, 2024

I personally think it is better to be safe, and consider there may not be one, amd if there isn't maybe use one of the default avatars from local accounts? If I'm understanding the issue anyway...

@IndusAryan
Copy link
Contributor Author

I personally think it is better to be safe, and consider there may not be one, amd if there isn't maybe use one of the default avatars from local accounts? If I'm understanding the issue anyway...

already it loads ?: getdefaultaccount, in worst case it will be empty like it is now

@M-BOUAB
Copy link

M-BOUAB commented Apr 1, 2024

Don't we already have this since forever? Am I missing something?

@IndusAryan
Copy link
Contributor Author

Don't we already have this since forever? Am I missing something?

this will show local ac if not using any sync provider

<stroke
android:width="2dp"
android:color="?attr/white" />
<corners android:radius="@dimen/rounded_image_radius" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is wrong, the cardCornerRadius is set to 25dp and while this MIGHT look fine, but rounded_image_radius may be subject to change which makes this a hard to track bug. (Also android on lower vers have difficulties with corners in some edge cases so this must match exactly)

@fire-light42 fire-light42 merged commit 0744189 into recloudstream:master Apr 22, 2024
2 checks passed
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.

None yet

4 participants