yourPISD / yourpisd

Android app to access myPISD (Plano Independent School District grades portal)
MIT License
2 stars 4 forks source link

Get rid of global state? #3

Open sid-kap opened 8 years ago

sid-kap commented 8 years ago

We use android.app.Application to store the user's credentials and the grade information that's been downloaded (see YPApplication.java). If I remember correctly, this is bad because Android sometimes deletes everything in android.app.Application when the app is backgrounded to reduce memory usage.

Back in 2013-2014, we received lots of bug reports saying that the app would crash upon opening it. We noticed on our own phones that this tended to happen if we backgrounded the app and then opened it again a few hours later. The YPApplication state getting cleared (set to null) might be the cause of this problem. We haven't tested this hypothesis though.

sid-kap commented 8 years ago

TLDR: We didn't property understand the Application life cycle, and therefore the app crashes a lot. It could probably be fixed by saving the contents of YPApplication in the activities' onDestroy() method.

theKidOfArcrania commented 8 years ago

Could we just relogin each time (and store the username and password in SharedPrefrences) instead of storing the Session object in the Application?

sid-kap commented 8 years ago

First, to clarify, I don't think storing the Session object in the Application is always bad. The point of Application is to store global state that can be accessed by any Activity. Since our app has many activities (MainActivity, ClassSwipeActivity) that access the same data (list of classes, etc.), it makes sense to keep this data in a global place.

Now, to answer your question: Yeah, relogin is the right thing to do (over crashing), although we want to relogin only when necessary (i.e. when Android has killed our data).

The solution might be as simple as the following: In onResume(), check if YPApplication.session is null. If it is null (suggesting that the system deleted our state), go back to LoginActivity.

sid-kap commented 8 years ago

This StackOverflow post is a good reference on the activity lifecycle. I think you can even detect (using onDestroy and onPause) when the app gets destroyed, so you could back up important things at that time. However, this may not be necessary; we could just store nothing and relogin every time the app gets destroyed.

ritu99 commented 8 years ago

I actually think Henry's method of SharedPreferences might work better because it opens the possibility of having an "offline" mode for the app.

sid-kap commented 8 years ago

Can you elaborate a bit?

I don't think we can get rid of YPApplication yet, since we have multiple activities (MainActivity and ClassSwipeActivity) accessing data stored in Session. Extending Application is the only way I know of to have a shared state between activities. Do you see a way around this?

sid-kap commented 8 years ago

Regarding an offline mode: If you want to store grade information, I think SQLite is the way to go, rather than storing a list of classes and grades in SharedPrefs.

pyrito commented 8 years ago

Wouldn't it be more cumbersome to use SQLite than just store the list on the system?

sid-kap commented 8 years ago

If I remember correctly, doesn't SharedPrefs only allow you to store strings?

However, I agree with you that SQLite on Android is a pain in the butt. Maybe there are some helper libraries that make it easier to work with?

sid-kap commented 8 years ago

Maybe Sugar ORM fits the bill? There are probably others though. The main thing that turned me off from SQLite on Android was handwriting SQL queries. So something like this would get rid of that problem.

ritu99 commented 8 years ago

Instead of SQL Lite you might want to look into Realm - it's pretty easy to implement for local storage.

And SharedPrefs allow you to store strings but in a key-value pair. You could save the session state in there as <"Multiple Users", "True"> type pairs, but storing grades might become annoying.

sid-kap commented 8 years ago

Yeah, Realm looks great! Thanks for the link.

I agree with what you said: for the simple things, use SharedPrefs, but for lists and nested structures (classes, grades, etc.) a database makes more sense.

sid-kap commented 8 years ago

I still think caching things in memory (a la YPApplication) is necessary though. I would guess that SharedPrefs and SQLite/Realm require reading from disk, which is slower than keeping the data in memory.

theKidOfArcrania commented 8 years ago

I attempted to fix the null pointer errors by logging out when the app gets destroyed and after restarting (in case the destroying procedure does not get called). The implications would be that you would have to login when you start up the app every time, (or it will log you in if you checked auto-login), which should be the typical behavior for apps. Only way to find out if I solved it is by testing it.

EDIT: currently working on trying to incorporate Realm in order to cache grades (when without internet access). Also considering removing the "auto-login" and "remember my password" check-boxes, since it's no longer necessary.

sid-kap commented 8 years ago

Nice! I'm curious to see if this solves the null pointer errors.

I like the idea of getting rid of the "auto-login" and "remember my password" fields, in lieu of a "Logout" action. The only downside is that we would always store peoples' username and passwords in plaintext on their phones, without getting their permission. I wonder if people would have a problem with that.

ritu99 commented 8 years ago

We could add a settings option that simply says "always log in" that forces the user to log in on refresh

sid-kap commented 8 years ago

or "always prompt for password"

theKidOfArcrania commented 8 years ago

Technically, the password is not stored "plain text", but it is encoded in base64. I do propose maybe encrypting the password in AES and storing the key throughout the entire program such as suggested in this stack-overflow answer. Besides, even storing it in plaintext will be secured enough unless the device is rooted, and we also do list that in the license agreement.

sid-kap commented 8 years ago

Ah, I see. What we have is probably fine then.