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

Remove unused resources #41

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Remove unused resources #41

merged 1 commit into from
Sep 3, 2024

Conversation

bramborman
Copy link
Contributor

@bramborman bramborman commented Aug 18, 2024

This removes unused AndroidX and other libraries. As AndroidX is removed, I had to manually reimplement the compat functionality it provides by overriding the registerReceiver method.
Also, removes tests that were the default samples. As NFC functionality is not available in emulators and considering the nature of this app I believe tests don't make much sense here/are impossible.

Also, unused app resources, i.e., colors and themes are removed here as they don't appear anywhere.

When creating an app bundle locally this reduced its size by more than 17 times, from 1234 kB to 72 kB.
This might be considered a micro-optimization, but I believe the lower the space an app consumes, the better 😁

I haven't yet had time to deploy this to phone to test this, so marking as draft.

@bramborman bramborman marked this pull request as ready for review August 18, 2024 18:28
@bramborman bramborman marked this pull request as draft August 18, 2024 18:28
@bramborman bramborman marked this pull request as ready for review August 25, 2024 14:19
@bramborman
Copy link
Contributor Author

bramborman commented Aug 25, 2024

Just tested on my phone (Android 14) and both basic and advanced mode works fine! ☺️

@bramborman
Copy link
Contributor Author

Hmm, thinking about it, the override approach might not work on older Android versions... Lemme check there...

@bramborman bramborman marked this pull request as draft August 25, 2024 14:29
@pcolby
Copy link
Owner

pcolby commented Aug 29, 2024

Thanks @bramborman, I'm hoping to have a play these updates this this weekend 🙂

@pcolby
Copy link
Owner

pcolby commented Sep 1, 2024

Thanks @bramborman, as this PR boils down to three different changes, I have different thoughts on each one:

  1. Removing the unused theme and assets: this one is easy 🙂 I hadn't realised that those assets could just be removed, but it makes sense now that I see it, and its great to clean that up. Of course we might want to bring them back if/when we implement a UI for features like Getting Started after install #44 and/or Automatic turn off in advanced mode #48, but that's easy enough to do then.

  2. Removing the unit tests: while I completely agree that the unit tests are, currently pretty useless, I had left them there with the intention of trying to actually implement a limited set sometime. I realise that the NFC functionality itself is not going to be testable (without live hardware, anyway), but I will explore testing as much (or little 😅) code as possible sometime - but that's really just a matter of self-learning through personal challenge 😊 I guess if I don't get time to play with the tests in the next couple of weeks, I might delete them from the main branch, and just play in a different branch.

  3. Removing androidx.ContextCompat: this one feels a bit counter-productive to me. While I can see the impressive size savings, its still relatively quite small, and that has to be balanced against the extra code required to be tested and maintained. I've had a quick look at the code inside ContextCompat.registerListener(), and it has differing code paths for at least three groupings of SDK/Android versions, and as I don't have devices with most of those versions, and the Android emulators don't emulate NFC, testing would be impractical for me. So, personally, I reckon it probably makes sense to stick with the androidx.ContextCompat implementation for now, to save myself some probably support headaches. (I currently have active users going all the way back to Android 7 😳)

So, if you'd like to split out the theme changes into a separate PR, I can merge that one pretty easily. Or I can just apply those changes myself, crediting you in the commit message - whichever you prefer. Add totally happy to continue discussing the other two changes 😄

Thanks!

@bramborman
Copy link
Contributor Author

Hi, thanks for having a look! 😊

Lemme comment on each of them:

  1. Completely agree. Also, using Jetpack Compose is a preferable approach as per current Google guidelines so I believe implementing the UI using itself is a better choice compared to the classic XMLs, and while I have no experience with it, I believe those assets wouldn't be used for Jetpack Compose anyway. But if they were, it's simple to reintroduce them.

  2. Agree. Also, as other features get implemented eventually, the tests might gradually make more sense and provide a value.

  3. Actually the broadcast receiver registration is testable even on the emulators, just with different broadcast, ie, not NFC-related. Also, while there are more code paths, I believe this app only needs these two. But anyway, as per current state, it seems more UI and features might get implemented where AndroidX would preferably be reintroduced anyway as it would make more sense, so this size-saving would be just temporary, so I'll revert this.

I'll just remove the changes from this PR as they are simple enough to reproduce eventually and it's low probability they ever need to be reintroduced anyway 😅

Copy link

sonarcloud bot commented Sep 1, 2024

@bramborman bramborman changed the title Remove unused libraries, resources, tests Remove unused resources Sep 1, 2024
@bramborman bramborman marked this pull request as ready for review September 1, 2024 12:50
@pcolby pcolby merged commit d68d08d into pcolby:main Sep 3, 2024
9 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.

2 participants