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

Massive changes to account flow #741

Merged
merged 30 commits into from
Nov 11, 2023

Conversation

Luna712
Copy link
Contributor

@Luna712 Luna712 commented Oct 31, 2023

The main purpose of this to to put all account stuff in one place and make it easier for TV users to manage accounts. If you don't want to go about it this way we can do some other way. This has numerous UX and focus improvements as well. Accounts are live updated and this fixes a few preexisting bugs.

I have additional plans for follow up PRs to have even more improvements such as making use of DrawerLayout for some things to make it easier to use. Inputs in dialogs are not always that convenient and I often accidentally dismiss them when I am not done, so may plan for DrawerLayout should improve things, but that will come in a follow-up PR eventually, unless you do not want it to.

Also adds a preference to turn off account selection at startup (refs #740)
If the preference is enabled, and the last used account has a PIN lock, it still asks for PIN at startup, however there is a button in PIN dialog in this case to "Use Default Account" so that of they don't know the PIN they don't get completely locked out.

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 31, 2023

Screenshot_20231031_173416_CloudStream Debug
Screenshot_20231031_173424_CloudStream Debug
This is the current version with more still to come, and a new version of the bottom dialog selector within MainActivity (so you don't always have to exit to this to do anything) It'll just take more work to finilize but I hope it works well enough, if not I'll do a few seperate PRs for the individual important components of this.

@IndusAryan
Copy link
Contributor

big pencil looking ugly on cards, honestly

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 1, 2023

big pencil looking ugly on cards, honestly

@IndusAryan

Indeed, I am working on design improvement

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 1, 2023

I might just split this into multiple PRs honestly, it is becoming hard to track things I want to do and that might be easier.

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 1, 2023

#737 and #742 are the most important parts of this so those can be merged first then I will be rebase this and continue working on perfecting it.

@Blatzar
Copy link
Contributor

Blatzar commented Nov 2, 2023

How about blurring the backgrounds when doing pencil stuff 🤔

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 2, 2023

How about blurring the backgrounds when doing pencil stuff 🤔

@Blatzar

This is exactly what I was thinking, I did dim the backgrounds, but it didn't seem to be enough, so blurring them seems like a good idea.

…into account-changes

# Conflicts:
#	app/src/main/java/com/lagradost/cloudstream3/ui/account/AccountAdapter.kt
#	app/src/main/java/com/lagradost/cloudstream3/ui/account/AccountDialog.kt
#	app/src/main/java/com/lagradost/cloudstream3/ui/account/AccountSelectActivity.kt
@Luna712
Copy link
Contributor Author

Luna712 commented Nov 3, 2023

@Blatzar @IndusAryan this is pretty much done, other than maybe a few bits of cleanup:
Screenshot_20231103_152228_CloudStream Debug
Screenshot_20231103_152023_CloudStream Debug
Screenshot_20231103_152422_CloudStream Debug

Things to note:

  • Long press to manage accounts still works if not on TV (it doesn't work great on TV 95% of the time (before, so it isn't enabled on TV, the way to edit accounts is made more clear with a button available also))
  • If you click "Manage Accounts" button in linear account management, it takes you to AccountSelectActivity to manage all accounts, and it runs it already in edit mode. Upon saving an edit to an account it will automatically exit back to MainActivity, resumed in the exact same state you left it, using the edited account. If you click the cancel editing button, and you came from MainActivity management, it will immediately return to MainActivity, rather than making you select an account again.

Those were the biggest things to mention, there was a lot of focus changes for TV with this, as well as some wording improvements, IE actually use something saying "Edit account" instead of "Create account" if actually editing an existing account, this also unifies account logic in one place rather than two adapters (partially identical) it allows us to unify it and manage the account logic a bit easier as well. Overall, in my personal opinion it is much improvement, especially for TV, if you don't want to do it like this, I'll find another way to satisfy any concerns. Thanks!

@Luna712 Luna712 marked this pull request as ready for review November 3, 2023 21:34
Copy link
Collaborator

@fire-light42 fire-light42 left a comment

Choose a reason for hiding this comment

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

  1. Optional: I really don't like that you don't do this in viewmodels, it is not a necessary change, but I think you should take a look at it at least. Looking at al this UI code just feels dirty
  2. Major: If you delete an account it wont register in the startup menu.
  3. Minor: The big edit symbol is not aligned with the text, make it aligned or in the top right
  4. Minor: The edit symbol on all accounts moves the lock to make space, that just looks bad
  5. Minor: The manage account button should be a fully horizontally filling BlackButton/WhiteButton, having it in the primary color goes against the design of the rest of the app
  6. Minor: The edit button should have a background ripple effect

@fire-light42
Copy link
Collaborator

By having a viewmodel you could share all the logic between the inapp edit account and the preapp edit account and make it really really easy to modify the UI without causing bugs

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 4, 2023

@fire-light42 thanks for the suggestions, view models confuse me, but I am working on implementing a view model for live updates at least so you don't have to reopen the bottom dialog if you are editing multiple, and it updates in a live context, but the rest of the UI code, I don't get view models as much. I can try to improve my knowledge of them to improve on that if absolutely needed though.

@fire-light42
Copy link
Collaborator

@fire-light42 thanks for the suggestions, view models confuse me, but I am working on implementing a view model for live updates at least so you don't have to reopen the bottom dialog if you are editing multiple, and it updates in a live context, but the rest of the UI code, I don't get view models as much. I can try to improve my knowledge of them to improve on that if absolutely needed though.

View models work by representing the state of the app in the viewmodel without any references to the view or context. Then observing the change in the fragment and updating the views. When a view is clicked it calls the viewmodel to tell that we have change something. By doing this the view is only a representation of the state and not the state itself, so if we wanted to change the UI or remove some part we can just change it without affecting the backed logic at all.

This makes the app fault resistant to app restarts and makes it hard to get into an invalid state.

For example a simple case would be a counter button

Viewmodel:

val count : MutableLiveData<Int> = MutableLiveData(0)

fun add() = count.postValue(count.value + 1)

Fragment:

observe(viewmodel.count) { count ->
    button.text = "I have been clicked $count times"
}
button.setOnClickListener {
    viewmodel.add()
}

no matter what you do here the button will always display the correct data, and if you wanted you could change this to a textview or some listview without any problems

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 4, 2023

Minor: The manage account button should be a fully horizontally filling BlackButton/WhiteButton, having it in the primary color goes against the design of the rest of the app

It did not look as nice when I tried that, if focused it looks nice, so I did a material TextButton, so it looks nicer, can change it if you want though, I can probably make it look nice the other way also if needed. Also the button style that I used does follow some design from the app, it is the same type of style as default action buttons in dialogs.

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 4, 2023

@fire-light42 I think I fixed some of your layout issues, though not 100% sure it is still great or not with that.

@IndusAryan
Copy link
Contributor

looks very good now

@Luna712 Luna712 force-pushed the account-changes branch 2 times, most recently from 484436c to 13bea95 Compare November 5, 2023 03:08
@Luna712
Copy link
Contributor Author

Luna712 commented Nov 5, 2023

This also now only completely reloads the home page if the API has changed, making switching accounts a lot faster if they use the same API. Bookmarks, and library are still refreshed regardless.

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 5, 2023

@fire-light42 I think this should be 100% done now, maybe a few things you might find in a review, but I made some more performance and UX improvements as well. This PR is quite large and I sorta wish I I did it in a few smaller ones, but it works fairly well at least.

Copy link
Collaborator

@fire-light42 fire-light42 left a comment

Choose a reason for hiding this comment

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

The app works really well, and I did not find any sort of bugs, however I have some questions about some design decisions like not placing accounts skip in the account settings and recreating the entire activity when rotation the screen.

That being said, I think this is my final review before merge as you should be able to fix all these in 10min as it is quite trivial changes. After that we can merge this

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 10, 2023

It should all be fixed now.

@fire-light42
Copy link
Collaborator

This is ready to merge, however I found a small bug that might need like 1 line to fix. If you go to homescreen->accounts->manage account->edit any account to change eg picture->apply ->home then it will not have updated in the home account switcher?

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 10, 2023

This is ready to merge, however I found a small bug that might need like 1 line to fix. If you go to homescreen->accounts->manage account->edit any account to change eg picture->apply ->home then it will not have updated in the home account switcher?

I just remembered, this is the real reason updateAccounts() was created in the view model, then it was used for deletions improperly but not why it existed. I'll see if I can fix it again another way, thanks!

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 11, 2023

@fire-light42 it should be fixed although maybe not the best way I could not seem to find another way to get it fixed.

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 11, 2023

This can be merged whenever now if you're okay with my method for fixing that last bug, if not I can try to fix it some other way I guess, if you have any suggestions. It might be something very simple also.

@Luna712 Luna712 mentioned this pull request Nov 11, 2023
3 tasks
seems I missed adding AndroidManifest.xml to commit
@fire-light42 fire-light42 merged commit 5bf2b4e into recloudstream:master Nov 11, 2023
2 checks passed
@fire-light42
Copy link
Collaborator

🔥 pr as always, this is a really nice addition to the ux and functionality of accounts. Glad to finally merge it!🥳 🥳 🥳

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 12, 2023

Awesome thanks for merging @fire-light42.

The last thing I want to do with accounts is what we discussed in my original PR adding AccountSelectActivity and PINs to make PIN accounts require them again when re-entering the account (unless skip account startup is enabled probably) if that still seems like a good idea to you?

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