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

Add account selection activity and support profile locks #736

Merged
merged 14 commits into from
Oct 30, 2023

Conversation

Luna712
Copy link
Contributor

@Luna712 Luna712 commented Oct 29, 2023

This also includes things from #733, if that is merged before this, I can rebase this again.

Resolves #604

TODO:

  • Auto-focus on last-used account
  • Improve TV layout
  • Don't show account selection if there is only one account that exists

Follow-up patch(s) for account improvements:

  • Support a "master" PIN on default account that is needed to delete any other accounts to prevent the default account PIN being able to bypass it by creating and then deleting accounts — currently PIN can not be set on the default account at all
  • Allow account creation, editing, and deletion from startup
  • Support custom (or more preset?) profile images

@Luna712 Luna712 marked this pull request as draft October 29, 2023 22:36
@Luna712
Copy link
Contributor Author

Luna712 commented Oct 29, 2023

For the most part, this is done, or at least things I have noticed are done.

@Luna712 Luna712 changed the title [WiP] Add account selection page Add account selection page Oct 29, 2023
@Luna712 Luna712 marked this pull request as ready for review October 29, 2023 23:45
@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

The grid layout definitely still needs some improvement as the gap gets further and further apart for larger screen sizes.

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

The grid layout definitely still needs some improvement as the gap gets further and further apart for larger screen sizes.

This is fixed

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

Screenshot_20231029_214937_CloudStream Debug

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

On TV layout this is one vertical column on the far left. This was done so that you don't have to keep pressing side DPAD keys and can just scroll down. I thought that was better UX, but I can change it if needed.

@Luna712 Luna712 changed the title Add account selection page Add account selection activity Oct 30, 2023
@Luna712 Luna712 changed the title Add account selection activity Add account selection activity and support profile locks Oct 30, 2023
@IndusAryan
Copy link
Contributor

IndusAryan commented Oct 30, 2023

i was thinking to add a biometric lock to app itself but it is not needed anymore after this 👍

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

i was thinking to add a biometric lock to app itself but it is not needed anymore after this 👍

@IndusAryan

That gives me an idea. I wonder if we could support biometric locks as the master lock (default account lock + lock needed to delete accounts, and maybe to remove pins from any account if you forget them, but I'm not sure about that last bit)

@IndusAryan
Copy link
Contributor

it will be an overkill, this is great enough imo

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

it will be an overkill, this is great enough imo

@IndusAryan

Yeah, it would not be hard to support device password/biometrics, but yeah might not be needed.

@fire-light42
Copy link
Collaborator

it will be an overkill, this is great enough imo

@IndusAryan

Yeah, it would not be hard to support device password/biometrics, but yeah might not be needed.

Biometrics is pain in the as that requires us to add that permission and make some sort of "safe" storage for that data, all for a 4 pin password

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

it will be an overkill, this is great enough imo

@IndusAryan
Yeah, it would not be hard to support device password/biometrics, but yeah might not be needed.

Biometrics is pain in the as that requires us to add that permission and make some sort of "safe" storage for that data, all for a 4 pin password

It is not needed, it was just an option to reset pin if you forgot it but don't need it.

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

I think I fixed every issue I know exists with this now (except maybe auto-focus on PIN input when shown), maybe missed something, but at least UI seems to be much improved and works better now.

@fire-light42
Copy link
Collaborator

While I love what you have done, it suffers from one major flaw. you have to reselect the account on every restart of the app and not every session, aka when you press the app icon. This leads to situations like this
image

Where PIP has the profile selection instead of a video.

This has to change to profile selection once per session not per app launch, that is just tedious and buggy. I would recommend on hijacking the main and pushing a fragment or dialog instead to remove this behavior.

@fire-light42
Copy link
Collaborator

This would also make extensions load in the background while you select account instead of offloading it entirely to after you select an account

@fire-light42
Copy link
Collaborator

Support a "master" PIN on default account that is needed to delete any other accounts to prevent the default account PIN being able to bypass it by creating and then deleting accounts — currently PIN can not be set on the default account at all

I think having a default account is necessary and it should not require any pin to remove accounts in case you forget it or smth. It should always be possible to remove all accounts even if you forget every password impo

@fire-light42
Copy link
Collaborator

Support custom (or more preset?) profile images

Account.customImage already exist for this exact thing, we simply do not have UI to let the user input the image url themself

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

Support custom (or more preset?) profile images

Account.customImage already exist for this exact thing, we simply do not have UI to let the user input the image url themself

@fire-light42

Yep, I know that is why I mentioned it, I will probably follow up later on adding this support

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

While I love what you have done, it suffers from one major flaw. you have to reselect the account on every restart of the app and not every session, aka when you press the app icon. This leads to situations like this image

Where PIP has the profile selection instead of a video.

This has to change to profile selection once per session not per app launch, that is just tedious and buggy. I would recommend on hijacking the main and pushing a fragment or dialog instead to remove this behavior.

@fire-light42

It does not ask again unless you click the app icon, rather than open from device recents. Netflix for example does the same thing. This is due to how android works, and there may be a way around it yeah, but for PINs for example it is more secure to ask again when you click the icon as that way if you leave the app open you are not always logged in. I did make a change so if you are already logged in and choose the same account again, it does not do a bunch of uneeded reloads though, and will instantly resume the former activity process where ever it was at the time after clicking account and/or entering PIN. The bug you mentioned with player overlay is indeed an issue that I will fix though. Additionally using an activity like this, rather than hijacking MainActivity is far more performant, always it to run faster, and is way more reliable, I originally did that way, and it was almost 10 full seconds faster to do this as a separate activity and load things there, I did just realize at some point I somehow removed my preloading code that loaded things while you create the account and sent things to MainActivity that way though. I'll see if I can re-add that, not sure how it was lost.

@fire-light42
Copy link
Collaborator

fire-light42 commented Oct 30, 2023

It does not ask again unless you click the app icon, rather than open from device recents. Netflix for example does the same thing.

Nope, netflix is once per session login, not once per app click. I tested right now on my own device

Additionally using an activity like this, rather than hijacking MainActivity is far more performant, always it to run faster, and is way more reliable

Ye, it might load faster, but the user will notice the 1s or so loading duration when clicking the account, you cant escape the loading time, one is not better than the other. Mainactivity loads stuff like extractors on runtime, and that is why I want you to be on mainactivity so they can load when you select account

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

It does not ask again unless you click the app icon, rather than open from device recents. Netflix for example does the same thing.

Nope, netflix is once per session login, not once per app click. I tested right now on my own device

Additionally using an activity like this, rather than hijacking MainActivity is far more performant, always it to run faster, and is way more reliable

Ye, it might load faster, but the user will notice the 1s or so loading duration when clicking the account, you cant escape the loading time, one is not better than the other. Mainactivity loads stuff like extractors on runtime, and that is why I want you to be on mainactivity so they can load when you select account

@fire-light42

I tested right now as well, I go to netlix, go to account, go to my device home, click the netflix icon again and it gives account selection again

Ye, it might load faster, but the user will notice the 1s or so loading duration when clicking the account, you cant escape the loading time, one is not better than the other. Mainactivity loads stuff like extractors on runtime, and that is why I want you to be on mainactivity so they can load when you select account

When you select account MainActivity loads, so if extractors are done on MainActivity that is still loaded, however, I was just testing something else, it seems to load perfectly if you simply extend MainActivity and override onCreate, but same behaver but it can fix the PiP issue anyway, thoughts on that?

@fire-light42
Copy link
Collaborator

fire-light42 commented Oct 30, 2023

I do not care how you solve it, but I need one login per session. Having to re login every time you open the app on a phone is very very bad and this wont get merged until you fix it.

This has the extended fix of not fucking up any pip or actions that are already on one account like watching a movie

@fire-light42
Copy link
Collaborator

fire-light42 commented Oct 30, 2023

What I am talking about when one session is that a login is keept throu this entire cycle, and is only released ondestroy
activity_lifecycle

It is ok to also do it in OnCreate, but then you need to wipe the current data that will automatically be restored

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

I do not care how you solve it, but I need one login per session. Having to re login every time you open the app on a phone is very very bad and this wont get merged until you fix it.

This has the extended fix of not fucking up any pip or actions that are already on one account like watching a movie

I figured out how, I just have to change a single line in AndroidManifest

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

@fire-light42 that should fix it
It seems like Netflix only requires it every single time if you are on an account requiring a PIN also, that is my bad.

@fire-light42
Copy link
Collaborator

fire-light42 commented Oct 30, 2023

@fire-light42 that should fix it It seems like Netflix only requires it every single time if you are on an account requiring a PIN also, that is my bad.

Amazing, now this update it amazing and ready to merge. 🔥 🔥 🔥 🔥 If you still feel like keeping the old behavior for locked account keep in mind that you need to clean ALL data when doing OnResume if we switch accounts.

That being said, this is a personal issue, and should be a small change to styles, but bottom nav is not the same color as bg. This is not necessary to fix for merge
image

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

@fire-light42 that should fix it It seems like Netflix only requires it every single time if you are on an account requiring a PIN also, that is my bad.

Amazing, now this update it amazing and ready to merge. 🔥 🔥 🔥 🔥 If you still feel like keeping the old behavior for locked account keep in mind that you need to clean ALL data when doing OnResume if we switch accounts.

That being said, this is a personal issue, and should be a small change to styles, but bottom nav is not the same color as bg. This is not necessary to fix for merge image

@fire-light42

Thanks a ton for the review. I'll think about doing that for locked accounts in a later patch, and I can do style update to fix navbar a little later in a seperate patch, unless you absolutely want it for this one?

@fire-light42
Copy link
Collaborator

You can pr the style fix or other account shit at a later time, if you want this merged now then reply with 👍 or ask for it.

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

You can pr the style fix or other account shit at a later time, if you want this merged now then reply with 👍 or ask for it.

@fire-light42

You can merge this. Thanks!

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

I went ahead and fixed the navigation bar color with this PR

@fire-light42 fire-light42 merged commit 65313b4 into recloudstream:master Oct 30, 2023
2 checks passed
@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

Thanks a ton @fire-light42

@fire-light42
Copy link
Collaborator

@Luna712 Your prs are always 🔥

btw you dont need to use View.VISIBLE, android has a built in getter and setter for isVisible.

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 30, 2023

@Luna712 Your prs are always 🔥

btw you dont need to use View.VISIBLE, android has a built in getter and setter for isVisible.

Thanks for the tip, setting View.VISIBLE was more of a personal preference for me over isVisible = true, but I'll take that into account in the future. I just personally like using constants when possible but in the future I'll use the other way, thanks!

@RokeJulianLockhart
Copy link

#736 (comment)

@Luna712, does it have the option for a list layout?

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 31, 2023

#736 (comment)

@Luna712, does it have the option for a list layout?

What do you mean by list layout?

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 1, 2023

#736 (comment)

@Luna712, does it have the option for a list layout?

@RokeJulianLockhart

If you mean in rows instead of a column, that is what #737 does to make TV layout nicer.

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.

Add support for profile locks in local accounts
4 participants