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

Improve synopsis/description display on phone and emulator #967

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

Luna712
Copy link
Contributor

@Luna712 Luna712 commented Mar 7, 2024

Instead of using a BottomSheetDialog, which can be buggy (IE issues like #816 and not looking quite right for extremely large descriptions) and modifies the UI more then we need to, which is just a little bit of poor UI, this way it just expands the description/synopsis if it is clicked, which is nicer UI. It only only this for phone and emulator layouts though, on TV having the same popup is nicer so it puts the text in a bit bigger view also.

This method is also nice, because if there is nothing to expand it doesn't created an unneeded dialog or do anything at all, and if you accidentally click it it doesn't end up getting in your way.

This also switches result_description to use maxLines as that UI looks nicer for very long descriptions (IE some YouTube descriptions from Invidious) and is also is more reliable.

Instead of using a BottomSheetDialog, which can be buggy (recloudstream#816) and modifies the UI more then we need to, which is just a little bit of poor UI, this way it just expands the description/synopsis if it is clicked, which is nicer UI. It only only this for phone and emulator layouts though, on TV having the same popup is nicer so it puts the text in a bit bigger view also.

This method is also nice, because if there is nothing to expand it doesn't created an unneeded dialog or do anything at all, and if you accidentally click it it doesn't end up getting in your way.

showBottomDialogText in SingleSelectionHelper could probably be removed after this if you want as it is now unused again, however I decided to leave it as I figured maybe it would be useful in the future but not sure if you want to or not...
Looks nicer in UI and is more reliable
@fire-light42
Copy link
Collaborator

While this PR is very good, I do have to say that I prefer the UI rn, and would much rather have #816 fixed then this expanding text. This is for 2 reasons, 1. CS3 Did have expanding text, but it caused crashes on low end devices for no reason, 2. This moves the UI too much, and I really dont want any UI to pop even on click like this, and not especially on episodes.

I can merge this, but I want some other ppl opinions on this.

@fire-light42
Copy link
Collaborator

But from a pure code quality perspective, you are doing it wrong. The state of expanded episodes/description ect should not be in the local variable but in the viewmodel. This is because rn when the UI is recreated it, isExpanded will be reset.

To understand this, recreation will happend under a few conditions like: 1. starting the app after not using it for a few min, 2. when some metadata is added (eg malsync), 3. when episodes becomes watched and updates the progress. 4. when switching episode ranges or sub+dub

@Luna712
Copy link
Contributor Author

Luna712 commented Mar 8, 2024

While this PR is very good, I do have to say that I prefer the UI rn, and would much rather have #816 fixed then this expanding text. This is for 2 reasons, 1. CS3 Did have expanding text, but it caused crashes on low end devices for no reason, 2. This moves the UI too much, and I really dont want any UI to pop even on click like this, and not especially on episodes.

I can merge this, but I want some other ppl opinions on this.

@fire-light42

One of the reasons I like this better is because on very large descriptions it makes the bottom dialog so large it covers the entire screen, with expanding description (which my method for doing this was actually inspired my Prime Video), it makes it look nicer and also it becomes more reliable and keepts the descriptions in place rather than it covering other content as well, leading to it becoming annoying if for example you accidentally click the description, even if #816 was not an issue, becomes quite annoying.

But from a pure code quality perspective, you are doing it wrong. The state of expanded episodes/description ect should not be in the local variable but in the viewmodel. This is because rn when the UI is recreated it, isExpanded will be reset.

To understand this, recreation will happend under a few conditions like: 1. starting the app after not using it for a few min, 2. when some metadata is added (eg malsync), 3. when episodes becomes watched and updates the progress. 4. when switching episode ranges or sub+dub

@fire-light42

This was actually intentional, I can do it in the view model if you prefer but I chose the local variable so that it never does persist, as it seems it persisting caused (albeit pretty minor) performance degradation, and there is not much reason it'd really need to persist when UI is recreated, at least how I figured but again if you prefer, I can change that.

@Luna712
Copy link
Contributor Author

Luna712 commented Mar 8, 2024

Also I don't think even when using dialogs description was never part of the view model and I figured synopsis only was because it needed to send it from EpisodeAdaptar to the result fragment via the callback, but maybe I'm completely wrong about that also.

@fire-light42
Copy link
Collaborator

I fixed the og issue of #816 at least.

@fire-light42
Copy link
Collaborator

This was actually intentional

Makes sense

@Luna712
Copy link
Contributor Author

Luna712 commented Mar 8, 2024

I fixed the og issue of #816 at least.

@fire-light42

Awesome! Thanks! Can this still be merged though since you approved, or would you like to wait for other feedback is why it isn't merged? That is fine with me, I'm just wondering...

@fire-light42
Copy link
Collaborator

Will be merged after testing on android 6 for crashing

@Luna712
Copy link
Contributor Author

Luna712 commented Mar 9, 2024

Will be merged after testing on android 6 for crashing

@fire-light42

Thanks! Just to note though I did test this on an Android 5.1.1 using an emulator and it did seem to work. Although, side-note I did notice some very weird UI discrepancies on 5.1.1 that doesn't happen in later Android versions, a lot of buttons end up red:
Screenshot_20240309_161941
Screenshot_20240309_161918

@KingLucius
Copy link
Contributor

Will be merged after testing on android 6 for crashing

@fire-light42

Thanks! Just to note though I did test this on an Android 5.1.1 using an emulator and it did seem to work. Although, side-note I did notice some very weird UI discrepancies on 5.1.1 that doesn't happen in later Android versions, a lot of buttons end up red:
Screenshot_20240309_161941
Screenshot_20240309_161918

It's a fixed color this red or theme color?

@Luna712
Copy link
Contributor Author

Luna712 commented Mar 11, 2024

@KingLucius it's fixed 100%, meaning it happens on all themes, with any primary color set it always ends up red on lower Android devices, but it doesn't happen on later ones.

@Luna712
Copy link
Contributor Author

Luna712 commented Mar 12, 2024

@fire-light42 I tested this on Android 5 (virtual), 7 (physical), 9 (virtual), and 14 (physical) with no problems, but for some reason I couldn't get Android 6 to even load up (with or without this patch) so if that is the version that has had problems in the past I could confirm on that one.

@fire-light42
Copy link
Collaborator

Tested on a real android 6 device, works as expected, no crashing.

@fire-light42 fire-light42 merged commit adc6539 into recloudstream:master Mar 13, 2024
2 checks passed
@Luna712
Copy link
Contributor Author

Luna712 commented Mar 13, 2024

Thanks a ton!

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

3 participants