Tech debt/Migrate to Androidx settings #240

SchoolGuy · 2020-11-24T19:25:54Z

Currently we are using our own settings. This is not desired as this is code we need to maintain. The Jetpack library gives us everything we need via AndroidX. Thus let's use that and reuse what people smarter then us invented. At least that is my proposal. Also this makes #210 stupidly easy to implement.

SchoolGuy · 2020-11-25T18:37:36Z

@bekuno If I tested correctly, I should be done with this. BUT we lost the summarys per default. Either I can implement some summary providers to preview settings or we have a small behavior change.

P.S.: Please test this manually before merging. I really would hate to be the one that breaks stuff in here but I feel like I am "Betriebsbild".

src/main/java/menion/android/whereyougo/MainApplication.java

bekuno · 2020-12-02T23:03:36Z

@SchoolGuy
Thanks for the big work so far.

I set DNM for now.

Please check your changes.
There are some subroutine liqidations, I think this is not necessary.
There is a lambda migration.
There are GUI style optimizations.
There is the switch to fragments with using appcompat mode.
Maybe more different points.

Please divide it in different PR which are logical clear.
Describe it in separate issue(s) if there is a bigger change.
And give them a clear headline.

By the way, the project was already migrated to AndroidX.
You have done mostly a switch to fragments using the appcompat library.

SchoolGuy · 2020-12-03T17:53:42Z

@bekuno Okay I will separate the code optimizations again and will submit them afterwards/separately. They always laugh at me and I can't resist to refactor stuff so it looks better - according to my knowledge and taste.

About the rest: These are the required changes. Nothing more. I looked over them again. Using the AndroidX Settings (which we are not using, although we are using AndroidX in general) requires me to fix things which were "working" before. Thus everything you see now can be seen as an atomic change. The two commits after the main one are technically not needed but would leave the application in a different (not so good) looking stage. Thus I would see them as required due to the fact that we want to change as little as possible for the users.

If you want to fiddle the changes into separate PRs you are welcome to do so but I then don't understand your pattern in this case. Thus I would love if you could spare us both the back and forth and do it yourself. I thought about splitting this for a full hour and the time addressing your wishes is starting to supersede the time which I need to implement the features...

SchoolGuy · 2020-12-03T18:01:00Z

Also what I did was the way the official guide is showing as one option: https://developer.android.com/guide/topics/ui/settings/organize-your-settings#split_your_hierarchy_into_multiple_screens

One Step further: What we did is no longer supported! https://developer.android.com/guide/topics/ui/settings/organize-your-settings#preferencescreens

bekuno · 2020-12-03T20:38:17Z

@SchoolGuy Sorry for the confusion. But I was confused - and misread your AndroidX plan. It is fine to migrate the settings management to a modernized state using some AndroidX jetpack functions. I hope that I can test the changes at weekend.

bekuno · 2020-12-03T20:38:59Z

Also what I did was the way the official guide is showing as one option: https://developer.android.com/guide/topics/ui/settings/organize-your-settings

One Step further: What we did is no longer supported! https://developer.android.com/guide/topics/ui/settings/organize-your-settings

both are the same links :-/

SchoolGuy · 2020-12-04T05:50:36Z

@bekuno I thought they had anchors... Anyway I will get you the quotes when I am in front of a computer and not on my mobile...

SchoolGuy · 2020-12-10T06:59:07Z

@bekuno Did a ninja edit to the original post

SchoolGuy · 2020-12-19T19:00:25Z

@bekuno How is the testing going?

bekuno · 2020-12-25T17:37:11Z

There is something broken:

    java.lang.RuntimeException: Unable to create application menion.android.whereyougo.MainApplication: java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
        at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6250)
        at android.app.ActivityThread.access$1200(ActivityThread.java:238)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1787)
        at android.os.Handler.dispatchMessage(Handler.java:106)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:7073)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:494)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:965)
     Caused by: java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
        at java.util.ArrayList.get(ArrayList.java:437)
        at menion.android.whereyougo.MainApplication.onCreate(MainApplication.java:251)
        at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1154)

in MainApplicationL242 is no result for lang,
so it crashes in L251 (loc.get(0))

bekuno · 2020-12-27T12:23:11Z

The initialization was done in L236 PreferenceFragment.setDefaultValues(this, R.xml.whereyougo_preferences, false);. but this is now devided in more whereyougo_preferences_*.xml, but not initilized from there.

SchoolGuy · 2020-12-27T15:53:16Z

@bekuno Since this is deprecated since API 29, I will try to fix it via replacing the functionallity of this method. We are using the "current" approach and thus we can't use the deprecated API. I was not able to reproduce your errormessage but I understand how it was produced...

SchoolGuy · 2020-12-27T16:14:01Z

@bekuno May I remove all Preference compability below 0.9.3. We have overtaken since that version and everything older was a "different" app. Thus I would love if we could cut that. That would enable me to cut out our custom "Preference" class. This would mean we would make the life easier when using SharedPreferences and would not need to criple the implementation of SharedPreferences for backward compatibility of something we don't even support anymore.

SchoolGuy · 2020-12-27T16:39:52Z

Nvm. I found that there is a 1:1 replacement. We can fix it via other means. Commit is in progress.

bekuno · 2020-12-27T17:58:22Z

@bekuno May I remove all Preference compability below 0.9.3. We have overtaken since that version and everything older was a "different" app. Thus I would love if we could cut that. That would enable me to cut out our custom "Preference" class. This would mean we would make the life easier when using SharedPreferences and would not need to criple the implementation of SharedPreferences for backward compatibility of something we don't even support anymore.

The app was not migrated to a new one.
The signing key for the Android play store is hold by the original author - https://github.com/menion .
In this way the app is the same in the play store.

If we implement a new system to store the user configuration then we need a way to migrate the settings from old to new.

bekuno · 2020-12-27T18:01:06Z

The app was not migrated to a new one.
The signing key for the Android play store is hold by the original author - https://github.com/menion .
In this way the app is the same in the play store.

For details see #52.

SchoolGuy · 2020-12-27T18:32:52Z

So now when we start the application for the very first time (that's why I never got your stacktrace), the application starts successfully. But the usage of settings is in my eyes a pure abomination of the Preferences Framework. I would do it like I did it in lines 250-254. But I guess this is something for another PR because this then requires real migration. The XML Files for the settings do not persist their settings currently. This means everything stays in a single file for now.

SchoolGuy · 2020-12-27T18:33:09Z

@bekuno Review request.

src/main/res/xml/whereyougo_preferences_appearance.xml

src/main/res/xml/whereyougo_preferences_directions.xml

src/main/java/menion/android/whereyougo/gui/activity/XmlSettingsActivity.java

SchoolGuy · 2021-01-30T09:31:08Z

@bekuno It appears that your Jenkins seems to hang? (at least for this PR)

SchoolGuy · 2021-01-30T10:08:44Z

@bekuno @moving-bits I think it is time for the (hopefully) final review.

Things that will be separated:

The Map Settings
Styling of the Settings

bekuno · 2021-01-30T15:59:21Z

@bekuno It appears that your Jenkins seems to hang? (at least for this PR)

@mucek4 Please have a look at cgeo-3. He is shown as offline, but needed for WhereYouGo.

Lineflyer · 2021-01-31T12:52:53Z

@mucek4 fired cgeo-3 up again.

bekuno · 2021-01-31T18:10:38Z

@bekuno @moving-bits I think it is time for the (hopefully) final review.

Things that will be separated:
* The Map Settings

* Styling of the Settings

Can you exclude the style changes from this PR or are they necessary for the function?
(src/main/res/values/styles_theme.xml, src/main/res/values-v11/styles_theme.xml, src/main/res/values-sw360dp-v13/values-preference.xml, src/main/res/values-v14/styles_theme.xml)

SchoolGuy · 2021-02-01T12:17:01Z

@bekuno Afaik this is required for AndroidX to work correctly. The reasoning I found is the following: https://developer.android.com/guide/topics/ui/look-and-feel/themes#Customize

Although this might not directly feel related to this PR, this is required because we now use Jetpack and they are the "new" Support Library. The other Android Apps I have worked with have followed the same principle.

SchoolGuy · 2021-02-13T12:29:33Z

Ping @bekuno @moving-bits
Is there something missing? I would like to finish my work! :)

moving-bits · 2021-02-14T07:34:34Z

Ping @bekuno @moving-bits
Is there something missing? I would like to finish my work! :)

sorry, wasn't actively monitoring this.

Did a quick glance & installed the PR locally. Generally LGTM, thanks for all the work you've put into this!

A few tiny observations regarding changes in behavior compared to the current implementation:

Nearly all summary provides do show current values now, which is fine. But in contrast to the current implementation they have lost their explanation text when there is some value to show. (E. g.: "Savegame slots: (5) Number of slots for saving game via menu button" or "Wherigo folder (... folder ...) Select folder for Wherigo files" etc.)
File selector (Settings => Wherigo folder): Why is "Select" in the middle? Standard Android-behavior would be to have the"positive reaction" at the right, the "negative" (="Cancel") right beside it to the left, and the "neutral" to the far left.
Settings => Login credentials & Settings => Appearance => Language do show an explanation but not their current value (intended?)
Am I right that you did not touch the map preferences in this PR? They look very different from the main screens preferences.

SchoolGuy · 2021-02-14T12:54:09Z

sorry, wasn't actively monitoring this.

Don't worry

Nearly all summary provides do show current values now, which is fine. But in contrast to the current implementation they have lost their explanation text when there is some value to show. (E. g.: "Savegame slots: (5) Number of slots for saving game via menu button" or "Wherigo folder (... folder ...) Select folder for Wherigo files" etc.)

Yes I did this on purpose to make it shorter. Of course with an additional commit this can be changed. Whatever you like better but most of the descriptions can't be read anyway because they are too long. I wanted to keep it simple, in this case this means a change of behavior.

File selector (Settings => Wherigo folder): Why is "Select" in the middle? Standard Android-behavior would be to have the"positive reaction" at the right, the "negative" (="Cancel") right beside it to the left, and the "neutral" to the far left.

Havn't changed that (at least to my knowledge). If you desire I can also change that.

Settings => Login credentials & Settings => Appearance => Language do show an explanation but not their current value (intended?)

I added as a fallback to all (maybe I have overseen one) settings that the description gets shown if no value is set. In the case of the language I am also showing this if the language is set to the default one. (see SettingsLocalizationFragment.java)

Am I right that you did not touch the map preferences in this PR? They look very different from the main screens preferences.

Yes you asked that before... ;)

moving-bits · 2021-02-14T20:57:45Z

@SchoolGuy
Thanks for your reply. Regarding the summaries I can live with both approaches. I propose to leave it as is in your PR for now, and if someone is really missing the description texts, it should be easy enough to add them later.

With respect to the button order: It would be nice to have this consistent, so I would ask you to update it, if possible.

And for the map's preferences: Maybe it's better to do this in a separate PR, to get this PR "out into the wild". (I only wasn't sure whether it was untouched yet or something was missing, that's why I asked.)

SchoolGuy · 2021-02-14T21:15:11Z

Will do the button order tomorrow.

SchoolGuy · 2021-02-20T10:38:28Z

@moving-bits @bekuno Ping. How does it look now? :)

moving-bits · 2021-02-20T10:41:24Z

ok from my side. @bekuno?

SchoolGuy · 2021-03-07T09:13:58Z

So any chance we merge this? I really would love to do the original task with adding a button to let the user check his credentials! Also I would love to modernize many more things. The mapsforge maps are also not done afaik.

moving-bits · 2021-03-27T08:47:55Z

@bekuno
Have you seen this PR waiting for your review?
Can the "Do not merge / WIP" label be removed?

moving-bits · 2021-03-27T08:48:46Z

(Note to the team: PR should be squashed on merging.)

bekuno · 2021-04-16T07:14:17Z

I did a short test the last days. I did not found any problems.
@Lineflyer Do you hav a chance to make deeper tests also on different devices?

bekuno · 2021-04-16T07:24:18Z

src/main/java/menion/android/whereyougo/gui/extension/activity/CustomActivity.java

+        // Set language
+        SharedPreferences sharedPreferences = PreferenceManager.getDefaultSharedPreferences(getApplicationContext());
+        String lang = sharedPreferences.getString(getString(R.string.pref_KEY_S_LANGUAGE), "");
+        setLocale(this, lang);


Why do we need the language management in different classes (here and in XmlSettingsActivity)?

Yes because the settings are not inheriting from the CustomActivity anymore but instead are inheriting from AppCompatActivity. Otherwise we need to change the implementation of the CustomActivity and this would have drastic effects for the whole application. That' why I duplicated this part. If you desire I can try out if it is possible to use the AppCompatActivity as a base class for the CustomActivity.

SchoolGuy · 2021-05-07T08:57:14Z

@bekuno @moving-bits What is missing to merge this?

moving-bits · 2021-05-07T19:33:04Z

Let's merge this one here, has been waiting in line for quite some time already. If there's anything to fix left, we can still open a new issue.

Thanks @SchoolGuy for your patience!

SchoolGuy marked this pull request as ready for review November 25, 2020 18:37

SchoolGuy mentioned this pull request Nov 25, 2020

Provide possibility to verify login data in settings #210

Merged

bekuno reviewed Dec 2, 2020

View reviewed changes

src/main/java/menion/android/whereyougo/MainApplication.java Outdated Show resolved Hide resolved

bekuno added the Do not merge / WIP Not for merging and/or work still in progress label Dec 2, 2020

bekuno requested changes Dec 27, 2020

View reviewed changes

src/main/res/xml/whereyougo_preferences_appearance.xml Outdated Show resolved Hide resolved

src/main/res/xml/whereyougo_preferences_directions.xml Outdated Show resolved Hide resolved

bekuno reviewed Dec 29, 2020

View reviewed changes

src/main/java/menion/android/whereyougo/gui/activity/XmlSettingsActivity.java Show resolved Hide resolved

SchoolGuy added 6 commits January 1, 2021 15:09

Migrate to AndroidX settings

c7aeae8

Fix padding of the icons for the settings

c10b2bc

Add header to match look and feel of initial screen

6ae3500

Interim fix for the First Startup Error

e1cf799

Fix missing keys of the categories in the settings

bad9650

Fix persisting the values of the settings

9550282

bekuno mentioned this pull request Jan 22, 2021

cannot pass a negative number to startActivtyForResult #269

Open

SchoolGuy added 2 commits January 29, 2021 18:50

Fix negative Intent Caller-ID

a3ef37e

Add default label to switch

87eb56f

Implement remaining Summary Providers

d694341

Settings: Change button order

e0a6f9d

Lineflyer mentioned this pull request Apr 2, 2021

Next release WhereYouGo #251

Closed

bekuno removed the Do not merge / WIP Not for merging and/or work still in progress label Apr 16, 2021

bekuno reviewed Apr 16, 2021

View reviewed changes

moving-bits merged commit 34c775d into cgeo:master May 7, 2021

bekuno mentioned this pull request May 9, 2021

Refactor settings to use PreferenceFragments instead of PreferenceActivity cgeo/cgeo#10579

Closed

SchoolGuy mentioned this pull request Jul 22, 2021

Refactor settings to AndroidX cgeo/cgeo#11298

Merged

6 tasks

Tech debt/Migrate to Androidx settings #240

Tech debt/Migrate to Androidx settings #240

Conversation

SchoolGuy commented Nov 24, 2020 • edited Loading

SchoolGuy commented Nov 25, 2020

bekuno commented Dec 2, 2020

SchoolGuy commented Dec 3, 2020

SchoolGuy commented Dec 3, 2020 • edited Loading

bekuno commented Dec 3, 2020

bekuno commented Dec 3, 2020

SchoolGuy commented Dec 4, 2020

SchoolGuy commented Dec 10, 2020

SchoolGuy commented Dec 19, 2020

bekuno commented Dec 25, 2020

bekuno commented Dec 27, 2020

SchoolGuy commented Dec 27, 2020 • edited Loading

SchoolGuy commented Dec 27, 2020

SchoolGuy commented Dec 27, 2020

bekuno commented Dec 27, 2020

bekuno commented Dec 27, 2020

SchoolGuy commented Dec 27, 2020

SchoolGuy commented Dec 27, 2020

SchoolGuy commented Jan 30, 2021 • edited Loading

SchoolGuy commented Jan 30, 2021

bekuno commented Jan 30, 2021

Lineflyer commented Jan 31, 2021

bekuno commented Jan 31, 2021

SchoolGuy commented Feb 1, 2021

SchoolGuy commented Feb 13, 2021

moving-bits commented Feb 14, 2021

SchoolGuy commented Feb 14, 2021

moving-bits commented Feb 14, 2021

SchoolGuy commented Feb 14, 2021

SchoolGuy commented Feb 20, 2021

moving-bits commented Feb 20, 2021

SchoolGuy commented Mar 7, 2021

moving-bits commented Mar 27, 2021

moving-bits commented Mar 27, 2021

bekuno commented Apr 16, 2021

bekuno Apr 16, 2021

Choose a reason for hiding this comment

SchoolGuy Apr 16, 2021

Choose a reason for hiding this comment

SchoolGuy commented May 7, 2021

moving-bits commented May 7, 2021

SchoolGuy commented Nov 24, 2020 •

edited

Loading

SchoolGuy commented Dec 3, 2020 •

edited

Loading

SchoolGuy commented Dec 27, 2020 •

edited

Loading

SchoolGuy commented Jan 30, 2021 •

edited

Loading