Handle LocationStateButton saved state using Bundle

[Isira-Seneviratne]

No description provided.

[westnordost]

Why is this better?

Did you test this?

[Isira-Seneviratne]

Why is this better?

It removes the need for an extra class to store the additional state. Alternatively, the Parcelize plugin could be used.

Did you test this?

Yes, on an Android 5.0 emulator. There were no issues.

[westnordost]

Hm, well, it's not less code at least. And it is a different API that I know, so to understand this code I'd need to look into that, too.

So, from my point of view, it is not an improvement because it replaces something that works with something that is mostly just different. I understand that if other custom views in this project were to save their instance state in that "old way" too, this API or even the Parcelize plugin may be an improvement, but I think there are on other custom views that do that, right?

I really do appreciate that you groom and modernize the codebase with your contributions. Did you read #5421, though? We plan to migrate the Android view code step by step to Jetpack Compose. So, any improvements on the current Android view code will be somewhat moot or short-lived. I don't mind merging it if the improvement is apparent, but this is the reason why I am somewhat reluctant to concern myself with it to understand it.

And maybe it is also not worth your time, if you consider how relatively short lived your improvements will be. Now, on the Jetpack Compose stuff, if you want to and can help there, your work would really make a difference.
Compose is somewhat of a paradigm shift, so if you wanted to help and don't know it yet, it might take some time to get acquainted with it. On the plus side, the whole industry is shifting to this kind of declarative UI paradigm, so it's a good idea to learn it too, anyway.

[westnordost]

Oh by the way, I noticed you are a contributor to NewPipe! Cool stuff, thank you! Love it!

[Isira-Seneviratne]

Yeah, I think making contributions in Compose would be better. I've gone through the tutorials, but hands-on experience would be very useful.

[westnordost]

Cool! I am also going through the documentation currently... it is quite.. well, as I wrote, paradigm shift... it's a lot. I am also having a cold right now, so that may contribute towards why I am a bit overwhelmed with it. I started with setting it up (#5447) though I am not sure if it wouldn't be better (=cleaner, more efficient) to first migrate to ViewModels everywhere before (#5530).

On the other hand, if I want to encourage people contributing to this, I should enable it as soon as possible. And contributions by contributors are vital to the whole undertaking. So, I'll probably set it up end of this week.

Anyway. I've looked through the PR again and I have to admit that it is actually better readable even if it is not shorter. But well, just wanted to let you know that (future) improvements on Android view stuff will be quite short-lived.