Provide possibility to verify login data in settings #210

SchoolGuy · 2020-08-29T17:46:26Z

This fixes #188

I will extend this once I have something presentable.

cgeo-ci-bot · 2020-08-29T17:46:28Z

Can one of the admins verify this patch?

SchoolGuy · 2020-08-29T17:48:08Z

@bekuno Can I somehow disable the CI Bot until this is out of WIP/Draft state. I think that would save you some resources.

Lineflyer · 2020-08-29T17:49:49Z

@SchoolGuy Not a big issue. It will do nothing until we give it the OK. So tell when ready, then we trigger the CI.

SchoolGuy · 2020-08-29T17:59:15Z

@Lineflyer Okay thanks for the info. I will remove the WIP progress and convert it to a "ready" PR when I am done.

SchoolGuy · 2020-10-27T10:43:33Z

Okay this was laying around. But I am starting to now dive into how this is done. The implementation of the Settings in WhereYouGo are so custom that I will just rewrite this. It works yes but it is ugly as hell. There are shared Preferences for a reason and then why not just use them. Currently we do this in custom classes etc. Also our Folder Picker is not updated and won't work on Fragments, thus this needs also to be changed. So this will take a while until I can fix all of that.

BUT all of this means that we will get a huge modernization of the app, allowing us to be more aligned with other changed from Android.

bekuno · 2020-10-27T11:17:38Z

@SchoolGuy This sounds very good. A modernization of the app is needed and welcome.

SchoolGuy · 2020-10-27T12:06:02Z

Lol. Just found out that we are not using AppCompatActivity anywhere and instead are using a CustomActivity. I thought this will be simple thing and now I am rewriting everything... Oh boy this PR will be fun when its done.

SchoolGuy · 2020-10-27T12:09:34Z

Okay new TODO list to get the button we actually want with this PR:

Rewrite the UI so we are using AppCompatActivty - this gives us the advantage so we don't need to take care of backward compability
Rewrite preferences so we are using the Android framework and not our own Preferences - even more alignment with Android
Add the actual button

SchoolGuy · 2020-10-27T18:21:19Z

Just to amuse the rest reading this...

clearPackageFromMemory(); // XXX not work on 2.2 and higher!!!

As far as I understand this comment we have code which is worrying about clearing up the memory on Android 2.2. I think We can safely remove this. Can't we? 😂

SchoolGuy · 2020-10-28T09:43:52Z

Commit 4e342f3 makes it possible to navigate through the settings again. BUT header bar is missing, edit is not tested, preview was deleted and I didn't check for any new bugs caused by changed to the AppCompat Themes.

SchoolGuy · 2020-10-28T09:55:23Z

@bekuno I think I am now deep enough in the code to understand what is vaguely done where... How much should I separate my efforts to fix tech-debt and fix the issue of checking the login data? Or should I just smash everything in here?

bekuno · 2020-10-28T14:01:17Z

Please divide the changes into parts as small as possible and reasonable. This facilitates the evaluation and a possibly necessary troubleshooting.
If the topic is in need of discussion then an issue should be created. Of course also if it is a topic that cannot be solved immediately.
Please always write to the PR what his task is and explain the solution if necessary.

I have noticed some sub areas of this PR, for example

Change of the minSdk - why is this necessary?
Abolition of the logger class - why?
Migration to androidx preferences
Adaptation of the theme styles

These are such typical single PR topics.

SchoolGuy · 2020-10-28T16:17:06Z

Okay I checked regarding the minSdk. Apparently the reasons why I did this had other root reasons, now this is working again.
Abolition of the logger class? - Well is was just tailcalling the Android framework and logging to a file partly. The rest is explained in the commit. For me this is just dead code. We really don't gain any advantages through this.
Migration to androidx preferences & Adaption of Theme Styles... Well this is just removing tech-debt and enabling my self to add that so much desired button.

So if we ignore the commits which are related to just removing literal dead code then all of this is directly related to this PR.

SchoolGuy · 2020-10-28T16:18:51Z

But I understand your point and will try to seperate as much as possible.

cgeo-ci-bot · 2020-10-28T16:20:14Z

Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 16
           

Complexity increasing per file
==============================
- src/main/java/menion/android/whereyougo/gui/fragments/settings/SettingsLocalizationFragment.java  1
- src/main/java/menion/android/whereyougo/gui/fragments/settings/SettingsLocationFragment.java  1
- src/main/java/menion/android/whereyougo/gui/fragments/settings/SettingsDirectionsFragment.java  1
- src/main/java/menion/android/whereyougo/gui/fragments/settings/SettingsGlobalFragment.java  1
- src/main/java/menion/android/whereyougo/gui/fragments/settings/SettingsCompassFragment.java  1
- src/main/java/menion/android/whereyougo/gui/fragments/settings/SettingsHiddenFragment.java  1
- src/main/java/menion/android/whereyougo/gui/fragments/settings/SettingsCredentialsFragment.java  1
- src/main/java/menion/android/whereyougo/gui/fragments/settings/SettingsAppearanceFragment.java  1
- src/main/java/menion/android/whereyougo/gui/fragments/settings/WhereYouGoSettingsFragment.java  1
         

Complexity decreasing per file
==============================
+ src/main/java/menion/android/whereyougo/gui/activity/XmlSettingsActivity.java  -49

See the complete overview on Codacy

bekuno · 2020-10-28T16:30:19Z

That would be fine.
At the end after rebasing this PR to master with the other changes it should only consist of changes to the initial plan - fix #188.

SchoolGuy · 2020-10-28T16:54:56Z

One last note: We generate a file called error.log. I know from experience that this file is only produced by WhereYouGo when Luacode in the Wherigo fails somehow. I see now that this is the reason for our custom logging class. I think through magic we also make use of this for normal Wherigo Logs. Thus I will drop this. I think I will instead add docs for this later...

SchoolGuy · 2020-11-25T18:44:18Z

I force pushed because now only #240 is missing before this one. This PR currently shows the button but it has no functionality because I need to check how we are going to find out if the data is correct.

SchoolGuy · 2021-05-08T05:24:20Z

I will continue to work on this starting tomorrow.

SchoolGuy · 2021-05-09T16:36:31Z

I did rebase this on master. The actual button is still there but my laptop has a crashing AVD, will test it tomorrow on my main computer.

SchoolGuy · 2021-05-17T06:21:53Z

@bekuno & @Lineflyer & other people of interest...

Okay so my rebase did mess up the code a bit. I am currently trying to fix the code again so the login check works. While I am doing this...

What is your opinion on having a round one of these boys fading one of those on success. On failure I would obviously do the some with a cross.

Second thought: Should I save the result of the check and display it the next time the user visits this page or do we want to display the result of the check only on button click?

SchoolGuy · 2021-05-17T06:23:04Z

^ For context: I am not using the Progress Dialog due to this Deprecation Warning: https://developer.android.com/reference/android/app/ProgressDialog

SchoolGuy · 2021-05-17T06:31:16Z

Okay that was quick... I didn't expect that. The code now works. But no user feedback is implemented on the button click yet.

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

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

src/main/java/menion/android/whereyougo/network/LoginTask.java

SchoolGuy · 2021-06-20T13:10:31Z

So due to some GC.com problems I can't test the positive side of the code but I think this is now read.y

SchoolGuy · 2021-07-01T06:55:51Z

@bekuno Ping again?

src/main/java/menion/android/whereyougo/network/LoginTask.java

bekuno · 2021-07-04T15:09:49Z

src/main/java/menion/android/whereyougo/network/LoginTask.java

+                        loginCheckdialog.setMessage("Init");
+                        break;
+                    case PING:
+                        loginCheckdialog.setMessage("Pinging");
+                        break;
+                    case LOGIN:
+                        loginCheckdialog.setMessage("Logging in");
+                        break;
+                    case LOGOUT:
+                        loginCheckdialog.setMessage("Logging out");
+                        break;
+                    default:
+                        loginCheckdialog.setMessage("Something unexpected happened. Please close the dialog!");
+                        break;
+                }
+                break;
+            default:
+                loginCheckdialog.setMessage("Something unexpected happened. Please close the dialog!");


This user visible string should be translated strings.

Is there a different behavior between the message "Logging in" (and other) and the defaults?
I dont understand, why "Please close the dialog!" is necessary.

In case for example that we receive a network disconnect or a networked changed event, the Task can't finish and the network possibly will throw a stacktrace, thus the error message for the user. Since we define the well known states inside the case, we want to catch all other situations to be safe on the usability side.

In the error situation you set the Button text to pref_gc_check_dialog_negative_button_close, the same as in the success situation.
The text "Please close the dialog!" is is therefore superfluous.

src/main/res/values/strings_pref.xml

src/main/java/menion/android/whereyougo/network/LoginTask.java

bekuno · 2021-07-04T17:00:09Z

The login check works really fine :-). I love it.
Thanks for your work.

SchoolGuy · 2021-07-05T08:19:13Z

@bekuno To keep the Git History vaguely clean I force pushed. Plelase re-review... :)

SchoolGuy · 2021-07-11T09:37:17Z

@bekuno Ping?

src/main/res/values/strings_pref.xml

SchoolGuy · 2021-07-12T09:40:18Z

Wanted to clean the Git History for that space and accidently removed your commits @bekuno
Could you rebase and push them again please?

bekuno · 2021-07-13T18:02:49Z

Wanted to clean the Git History for that space and accidently removed your commits @bekuno
Could you rebase and push them again please?

new changed with web editor.
Please rebase now.

SchoolGuy · 2021-07-14T06:27:02Z

@bekuno I think this is clean and ready now to be merged. If I forgot something, please point me to it.

bekuno · 2021-07-14T21:08:17Z

I will test it the next days.
There are already 2 follow up issues - #294 and #296.

SchoolGuy · 2021-07-21T18:49:17Z

@bekuno Ping?

SchoolGuy · 2021-07-28T15:12:04Z

@bekuno Ping again?

bekuno · 2021-07-28T20:20:11Z

I tested yesterday a bit.
The "Close dialog" is alredy set before "logging out", but should set after it, imho.
Please have a look at it.

SchoolGuy · 2021-07-31T12:16:42Z

@bekuno Fixed and tested

SchoolGuy · 2021-08-03T12:49:43Z

@bekuno Ping?

SchoolGuy · 2021-08-07T08:41:58Z

@bekuno I really would like to get this merged soon. It is open way too long for my liking...

SchoolGuy · 2021-08-13T13:21:43Z

@bekuno Ping?

Lineflyer · 2021-08-13T18:00:58Z

Lets merge to do further testing.

@bekuno Hope its OK for you...but indeed this PR is open to long and I guess @SchoolGuy will anyway continue to work on it if there are any deficiencies left.

bekuno · 2021-08-13T21:29:57Z

Lets merge to do further testing.

@bekuno Hope its OK for you...but indeed this PR is open to long and I guess @SchoolGuy will anyway continue to work on it if there are any deficiencies left.

@Lineflyer It' s fine for me.
@SchoolGuy Thank you very much for your work on WhereYouGo. I appreciate it very much.

I have had not much time, therefore I concentrated my work to c:geo.
I will test the change the next days. But I think the last requested change was realized.
For now we have already a bug #296 and a follow up FR #294.

SchoolGuy · 2021-08-14T08:32:04Z

@bekuno I am aware but I will first focus on cgeo/cgeo#11298 so I don't open to many PRs at once. A day has only 24h for as well.

bekuno · 2021-08-15T12:46:19Z

@bekuno I am aware but I will first focus on cgeo/cgeo#11298 so I don't open to many PRs at once. A day has only 24h for as well.

That's perfect.
And I hope that later the knowledge of SAF from c:geo will bring WYG forward to a new release.

This was referenced Oct 28, 2020

Auto improvements by Android Studio #227

Merged

Tech debt/remove custom abstract main #228

Merged

SchoolGuy mentioned this pull request Nov 25, 2020

Tech debt/Migrate to Androidx settings #240

Merged

Lineflyer mentioned this pull request Apr 2, 2021

Next release WhereYouGo #251

Closed

bekuno requested changes May 18, 2021

View reviewed changes

SchoolGuy marked this pull request as ready for review June 20, 2021 13:11

bekuno requested changes Jul 4, 2021

View reviewed changes

bekuno reviewed Jul 4, 2021

View reviewed changes

src/main/res/values/strings_pref.xml Outdated Show resolved Hide resolved

bekuno reviewed Jul 4, 2021

View reviewed changes

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

bekuno mentioned this pull request Jul 5, 2021

Generalize LoginTask and DownloadCartridgeTask #294

Open

bekuno reviewed Jul 11, 2021

View reviewed changes

src/main/res/values/strings_pref.xml Outdated Show resolved Hide resolved

SchoolGuy added 2 commits July 14, 2021 08:25

Implement button to check the login data

f1a1c2f

UI: Implement user feedback to the Settings

9d0a953

SchoolGuy changed the title ~~WIP: Provide possibility to verify login data in settings~~ Provide possibility to verify login data in settings Jul 14, 2021

Show Logincheck finished after the last Task

c6a7a3a

Lineflyer merged commit 1d555fe into cgeo:master Aug 13, 2021

SchoolGuy deleted the feature/add-login-verify branch August 13, 2021 20:04

Provide possibility to verify login data in settings #210

Provide possibility to verify login data in settings #210

Conversation

SchoolGuy commented Aug 29, 2020

cgeo-ci-bot commented Aug 29, 2020

SchoolGuy commented Aug 29, 2020

Lineflyer commented Aug 29, 2020

SchoolGuy commented Aug 29, 2020

SchoolGuy commented Oct 27, 2020

bekuno commented Oct 27, 2020

SchoolGuy commented Oct 27, 2020

SchoolGuy commented Oct 27, 2020 • edited Loading

SchoolGuy commented Oct 27, 2020

SchoolGuy commented Oct 28, 2020

SchoolGuy commented Oct 28, 2020

bekuno commented Oct 28, 2020

SchoolGuy commented Oct 28, 2020 • edited Loading

SchoolGuy commented Oct 28, 2020

cgeo-ci-bot commented Oct 28, 2020

bekuno commented Oct 28, 2020

SchoolGuy commented Oct 28, 2020

SchoolGuy commented Nov 25, 2020

SchoolGuy commented May 8, 2021

SchoolGuy commented May 9, 2021

SchoolGuy commented May 17, 2021

SchoolGuy commented May 17, 2021

SchoolGuy commented May 17, 2021

SchoolGuy commented Jun 20, 2021

SchoolGuy commented Jul 1, 2021

bekuno Jul 4, 2021

Choose a reason for hiding this comment

SchoolGuy Jul 5, 2021

Choose a reason for hiding this comment

bekuno Jul 11, 2021

Choose a reason for hiding this comment

bekuno commented Jul 4, 2021

SchoolGuy commented Jul 5, 2021

SchoolGuy commented Jul 11, 2021

SchoolGuy commented Jul 12, 2021

bekuno commented Jul 13, 2021

SchoolGuy commented Jul 14, 2021

bekuno commented Jul 14, 2021

SchoolGuy commented Jul 21, 2021

SchoolGuy commented Jul 28, 2021

bekuno commented Jul 28, 2021

SchoolGuy commented Jul 31, 2021

SchoolGuy commented Aug 3, 2021

SchoolGuy commented Aug 7, 2021

SchoolGuy commented Aug 13, 2021

Lineflyer commented Aug 13, 2021

bekuno commented Aug 13, 2021

SchoolGuy commented Aug 14, 2021

bekuno commented Aug 15, 2021

SchoolGuy commented Oct 27, 2020 •

edited

Loading

SchoolGuy commented Oct 28, 2020 •

edited

Loading