SimonHalvdansson / Harmonic-HN

Modern Android client for Hacker News
https://play.google.com/store/apps/details?id=com.simon.harmonichackernews
Apache License 2.0
611 stars 40 forks source link

Add more auto themes #155

Open flofriday opened 6 months ago

flofriday commented 6 months ago

I added two more automatic themes for dark/light and black/light, which should address issue #118.

While this works as expected I have a couple of questions about some code pieces I stumbled upon.

API level checks

In ThemeUtil.java there is the following api level check just for the material theme:

 switch (theme) {
            case "material_daynight":
                //below 30, default on comments is swipeBack so if we don't use it we need to change to a normal theme
                if (Build.VERSION.SDK_INT < 30) {
                    if (!swipeBack) {
                        activity.setTheme(R.style.AppThemeMaterialDayNight);
                    }
                } else {
                    //at and above 30, the default is AppTheme so if we use swipeBack we need to change
                    if (swipeBack) {
                        activity.setTheme(R.style.ThemeSwipeBackNoActionBarMaterialDayNight);
                    }
                }
                break;

I don't quite get why that logic would only be necessary for that theme. Moreover, it seems faulty to me, like if we are on build level over 30 we would just not update the theme if swipeBack is disabled? Maybe I am overlooking something here.

Change auto theme naming

I currently kept your naming convention of {theme}_daynight for the automatic themes, however since they are not really dependent on the time but often on the system settings I think dynamic or automatic would be a better term.

As always feedback is welcome.

SimonHalvdansson commented 5 months ago

Just want to say I have not forgotten about this, just have a lot going on research-wise at the moments but should get this merged soon! As always, thanks for your work :)

flofriday commented 5 months ago

No problem, I already assumed something like that. Also this is just a side project, no need to get stressed out about it 😉.

Best of luck with your research and if you need help with anything, feel free to reach out.

PS: feel free to ignore #157 for a while, it's definitely not worth your time right now, as it doesn't add any features, is a lot of code and probably will result in a few discussions about how code look.