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.

Sign up for GitHub

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

Fix #285 #295

Merged
merged 29 commits into from
Sep 21, 2016
Merged

Fix #285 #295

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
697e1ff
Start working on Reminders.
paolorotolo Sep 3, 2016
e47e12c
Add Reminders in Realm. Add migration to schema 4.
paolorotolo Sep 4, 2016
06984ab
Add ArrayAdapter for reminders.
paolorotolo Sep 5, 2016
749ea74
Continue work on Reminders.
paolorotolo Sep 5, 2016
89e6543
Add method to get all active reminders
paolorotolo Sep 8, 2016
c5b4218
Fix notes export, abstract to a pseudo MVP architecture
JoaquimLey Sep 9, 2016
2a20536
Merge remote-tracking branch 'upstream/develop' into develop
JoaquimLey Sep 9, 2016
99b510b
Continue work on Reminders.
paolorotolo Sep 9, 2016
ae0f1cc
Simple note tests
JoaquimLey Sep 10, 2016
826e4ca
Fix notes export, abstract to a pseudo MVP architecture
JoaquimLey Sep 9, 2016
1514998
Simple note tests
JoaquimLey Sep 10, 2016
afb6d1a
Merge branch 'develop' of github.com:JoaquimLey/glucosio-android into…
JoaquimLey Sep 10, 2016
545af91
Improve quality from codacy report
JoaquimLey Sep 10, 2016
7a305cc
Fix migration.
paolorotolo Sep 10, 2016
c79c10e
Class ReadingToCSV is not final, removed the redundant Sys.currentTMl…
JoaquimLey Sep 10, 2016
7556aa9
Add reminders notification.
paolorotolo Sep 11, 2016
05e12ad
Improvements #2 on PR comments
JoaquimLey Sep 11, 2016
8b51d72
Fix some issues.
paolorotolo Sep 12, 2016
b5eb81d
Codacy warnings fixed
emartynov Sep 12, 2016
10d731d
Automatic translation import (build 937).
glucat Sep 12, 2016
6c1895b
Fix reminder triggered on boot, update Gradle version to stable.
paolorotolo Sep 19, 2016
acacf1d
Merge pull request #296 from Glucosio/reminders
emartynov Sep 20, 2016
976f522
Update dependencies.
paolorotolo Sep 20, 2016
76c6073
Fix notes export, abstract to a pseudo MVP architecture
JoaquimLey Sep 9, 2016
5f7725a
Simple note tests
JoaquimLey Sep 10, 2016
c1a098d
Improve quality from codacy report
JoaquimLey Sep 10, 2016
4b07fff
Class ReadingToCSV is not final, removed the redundant Sys.currentTMl…
JoaquimLey Sep 10, 2016
6653e10
Improvements #2 on PR comments
JoaquimLey Sep 11, 2016
59db67f
Merge with remote counterpart
JoaquimLey Sep 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/src/androidTest/java/org/glucosio/android/MainActivityTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,6 @@ public void check_001_checkIfToolbarIsDisplayed() throws InterruptedException {
goThroughHelloActivity();
onView(withId(R.id.activity_main_toolbar)).check(matches(isDisplayed()));
}

// TODO: 09/09/16 Test the responses in UI -> Show ui notice when export started, is empty or error
}
12 changes: 11 additions & 1 deletion app/src/main/java/org/glucosio/android/GlucosioApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@
import uk.co.chrisjenx.calligraphy.CalligraphyConfig;

public class GlucosioApplication extends Application {

private static GlucosioApplication sInstance;

@Nullable
private Analytics analytics;

Expand All @@ -55,7 +58,7 @@ public class GlucosioApplication extends Application {
@Override
public void onCreate() {
super.onCreate();

sInstance = this;
initFont();
initLanguage();
}
Expand Down Expand Up @@ -153,6 +156,13 @@ public Preferences getPreferences() {
return preferences;
}

public static GlucosioApplication getInstance() {
if (sInstance == null) {
sInstance = new GlucosioApplication();
}
return sInstance;
}

@NonNull
public HelloPresenter createHelloPresenter(@NonNull final HelloActivity activity) {
return new HelloPresenter(activity, getDBHandler());
Expand Down
4 changes: 4 additions & 0 deletions app/src/main/java/org/glucosio/android/activity/AboutActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.glucosio.android.GlucosioApplication;
import org.glucosio.android.R;
import org.glucosio.android.analytics.Analytics;
import org.glucosio.android.presenter.ExportPresenter;

import java.util.Locale;

Expand All @@ -48,6 +49,9 @@ protected void onCreate(Bundle savedInstanceState) {
getFragmentManager().beginTransaction()
.replace(R.id.aboutPreferencesFrame, new MyPreferenceFragment()).commit();

ExportPresenter exportPresenter = new ExportPresenter(this);
exportPresenter.getFromDay();

getSupportActionBar().setDisplayHomeAsUpEnabled(true);
getSupportActionBar().setTitle(getString(R.string.preferences_about_glucosio));
}
Expand Down
42 changes: 34 additions & 8 deletions app/src/main/java/org/glucosio/android/activity/MainActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,14 @@
import org.glucosio.android.db.DatabaseHandler;
import org.glucosio.android.presenter.ExportPresenter;
import org.glucosio.android.presenter.MainPresenter;
import org.glucosio.android.view.ExportView;

import java.util.Calendar;

import uk.co.chrisjenx.calligraphy.CalligraphyContextWrapper;


public class MainActivity extends AppCompatActivity implements DatePickerDialog.OnDateSetListener {
public class MainActivity extends AppCompatActivity implements DatePickerDialog.OnDateSetListener, ExportView {

private static final int REQUEST_INVITE = 1;
private static final String INTENT_EXTRA_PAGER = "pager";
Expand Down Expand Up @@ -258,7 +259,37 @@ public boolean onItemClick(View view, int position, IDrawerItem drawerItem) {
private void initPresenters(GlucosioApplication application) {
final DatabaseHandler dbHandler = application.getDBHandler();
presenter = new MainPresenter(this, dbHandler);
exportPresenter = new ExportPresenter(this, dbHandler);
exportPresenter = new ExportPresenter(this);
}

@Override
protected void attachBaseContext(Context newBase) {
super.attachBaseContext(CalligraphyContextWrapper.wrap(newBase));
}


@Override
public void onExportStarted(int numberOfItemsToExport) {
showExportedSnackBar(numberOfItemsToExport); // TODO: 09/09/16 Instead of calling this method, move logic to this callback ?
Log.d("Activity", "onExportStarted(): you might want to track this event");
}

@Override
public void onNoItemsToExport() {
showNoReadingsSnackBar(); // TODO: 09/09/16 Instead of calling this method, move logic to this callback ?
Log.e("Activity", "onNoItemsToExport(): you might want to track this event");
}

@Override
public void onExportFinish(Uri uri) {
showShareDialog(uri); // TODO: 09/09/16 Instead of calling this method, move logic to this callback ?
Log.e("Activity", "onExportFinish(): you might want to track this event");
}

@Override
public void onExportError() {
showExportError(); // TODO: 09/09/16 Instead of calling this method, move logic to this callback ?
Log.e("Activity", "onExportError(): you might want to track this event");
}

private void openA1CCalculator() {
Expand Down Expand Up @@ -292,7 +323,7 @@ public void openPreferences() {
finishActivity();
}

public void finishActivity(){
public void finishActivity() {
// dismiss dialog if still expanded
bottomSheetAddDialog.dismiss();
// then close activity
Expand Down Expand Up @@ -711,9 +742,4 @@ private boolean checkPlayServices() {
private void showErrorDialogPlayServices() {
Toast.makeText(getApplicationContext(), R.string.activity_main_error_play_services, Toast.LENGTH_SHORT).show();
}

@Override
protected void attachBaseContext(Context newBase) {
super.attachBaseContext(CalligraphyContextWrapper.wrap(newBase));
}
}
1 change: 0 additions & 1 deletion app/src/main/java/org/glucosio/android/activity/PreferencesActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.preferences);


getFragmentManager().beginTransaction()
.replace(R.id.preferencesFrame, new MyPreferenceFragment()).commit();

Expand Down
138 changes: 66 additions & 72 deletions app/src/main/java/org/glucosio/android/presenter/ExportPresenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@
import android.support.v4.content.FileProvider;
import android.util.Log;

import org.glucosio.android.activity.MainActivity;
import org.glucosio.android.GlucosioApplication;
import org.glucosio.android.db.DatabaseHandler;
import org.glucosio.android.db.GlucoseReading;
import org.glucosio.android.tools.ReadingToCSV;
import org.glucosio.android.view.ExportView;

import java.io.File;
import java.util.ArrayList;
import java.util.Calendar;
import java.util.List;

Expand All @@ -56,84 +56,78 @@ public class ExportPresenter {
private int toDay;
private int toMonth;
private int toYear;
private DatabaseHandler dB;
private MainActivity activity;

public ExportPresenter(MainActivity exportActivity, DatabaseHandler dbHandler) {
this.activity = exportActivity;
dB = dbHandler;
}

public void onExportClicked(final Boolean all) {
final List<GlucoseReading> readings;
private Activity mActivity;
private ExportView mExportView;
private DatabaseHandler dB;

if (all) {
readings = dB.getGlucoseReadings();
public ExportPresenter(Activity activity) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not to have parameter for constructor only ExportView type? Why do we need to cast it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you need the Activity since you are handling the permissions in the presenter (it wouldn't be really my choice but see line 140) which requires to have an activity, the cast is to ensure said activity is implementing the required View interface.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear! It was there already. As for me, a presenter should not have any dependency to Android API, but this is the discussion for the android team.

As the quick solution I would have two parameters - one view and one activity. And remove casting!

Copy link
Contributor Author

@JoaquimLey JoaquimLey Sep 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it would be easy to abstract the Android apis from the Presenter if there was no Permission handling there (and it shouldn't).
With the current setup you'll be instantiating something like this:
new ExportPresenter(this, this); not pretty, but refactoring this wouldn't make the PR with to many differences.

mActivity = activity;
if (activity instanceof ExportView) {
mExportView = (ExportView) activity;
} else {
Calendar fromDate = Calendar.getInstance();
fromDate.set(Calendar.YEAR, fromYear);
fromDate.set(Calendar.MONTH, fromMonth);
fromDate.set(Calendar.DAY_OF_MONTH, fromDay);

Calendar toDate = Calendar.getInstance();
toDate.set(Calendar.YEAR, toYear);
toDate.set(Calendar.MONTH, toMonth);
toDate.set(Calendar.DAY_OF_MONTH, toDay);
readings = dB.getGlucoseReadings(fromDate.getTime(), toDate.getTime());
throw new RuntimeException("ExportPresenter Activity must implement ExportView interface");
}
dB = GlucosioApplication.getInstance().getDBHandler();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why singleton preferred instead of parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way you don't have to pass the databaseHandler as a parameter since you are always using the one residing in the application context anyway. Using a singleton you ensure you are accessing the same reference.

Copy link
Contributor

@emartynov emartynov Sep 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually breaking testability. If you have parameter then you can easy mock it in unit tests. You have to make static setters or some hooks with the singleton pattern.

It is probably a good thing if you want to keep the single reference to the DB (I'm not sure if required). But all scope logic should be done outside of consumer class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again I've gone along the lines of what was already present. I think the constructor should remain with the ExportView as the only param, and have a setter for the dbh. Once again I've gone along the lines of what was present.

But all scope logic should be done outside of consumer class

You're right.

}

public void onExportClicked(final boolean isExportAll) {

if (hasStoragePermissions(mActivity)) {
// final ReadingToCSV csv = new ReadingToCSV(GlucosioApplication.getInstance());
final String preferredUnit = dB.getUser(1).getPreferred_unit();
final boolean[] isEmpty = {false};
Copy link
Contributor

@emartynov emartynov Sep 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to create simple structure {String //filename, boolean //isEmpty} instead of having several states to update?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you can handle all three cases: When export process was successful, export process was successful but the database is empty or export process was unsuccessful.


new AsyncTask<Void, Void, String>() {
@Override
protected String doInBackground(Void... params) {

Realm realm = dB.getNewRealmInstance();
final List<GlucoseReading> readings;
if (isExportAll) {
readings = dB.getGlucoseReadings(realm);
} else {
Calendar fromDate = Calendar.getInstance();
fromDate.set(Calendar.YEAR, fromYear);
fromDate.set(Calendar.MONTH, fromMonth);
fromDate.set(Calendar.DAY_OF_MONTH, fromDay);

Calendar toDate = Calendar.getInstance();
toDate.set(Calendar.YEAR, toYear);
toDate.set(Calendar.MONTH, toMonth);
toDate.set(Calendar.DAY_OF_MONTH, toDay);
readings = dB.getGlucoseReadings(realm, fromDate.getTime(), toDate.getTime());
}

if (readings.size() != 0) {
if (hasStoragePermissions(activity)) {
activity.showExportedSnackBar(readings.size());
final ReadingToCSV csv = new ReadingToCSV(activity.getApplicationContext());
final String preferredUnit = dB.getUser(1).getPreferred_unit();

new AsyncTask<Void, Void, String>() {
@Override
protected String doInBackground(Void... params) {
final ArrayList<GlucoseReading> readingsToExport;
Realm realm = dB.getNewRealmInstance();

if (all) {
readingsToExport = dB.getGlucoseReadings(realm);
} else {
Calendar fromDate = Calendar.getInstance();
fromDate.set(Calendar.YEAR, fromYear);
fromDate.set(Calendar.MONTH, fromMonth);
fromDate.set(Calendar.DAY_OF_MONTH, fromDay);

Calendar toDate = Calendar.getInstance();
toDate.set(Calendar.YEAR, toYear);
toDate.set(Calendar.MONTH, toMonth);
toDate.set(Calendar.DAY_OF_MONTH, toDay);
readingsToExport = dB.getGlucoseReadings(realm, fromDate.getTime(), toDate.getTime());
}

if (dirExists()) {
Log.i("glucosio", "Dir exists");
return csv.createCSVFile(realm, readingsToExport, preferredUnit);
} else {
Log.i("glucosio", "Dir NOT exists");
return null;
}
mExportView.onExportStarted(readings.size());
if (readings.isEmpty()) {
isEmpty[0] = true;
return null;
}

@Override
protected void onPostExecute(String filename) {
super.onPostExecute(filename);
if (filename != null) {
Uri uri = FileProvider.getUriForFile(activity.getApplicationContext(),
activity.getApplicationContext().getPackageName() + ".provider.fileprovider", new File(filename));
activity.showShareDialog(uri);
} else {
//TODO: Show error SnackBar
activity.showExportError();
}
if (dirExists()) {
Log.i("glucosio", "Dir exists");
return ReadingToCSV.createCSVFile(mActivity, realm, readings, preferredUnit);
} else {
Log.i("glucosio", "Dir NOT exists");
return null;
}
}.execute();
}
} else {
activity.showNoReadingsSnackBar();
}

@Override
protected void onPostExecute(String filename) {
super.onPostExecute(filename);
if (filename != null) {
Uri uri = FileProvider.getUriForFile(mActivity.getApplicationContext(),
mActivity.getApplicationContext().getPackageName() + ".provider.fileprovider", new File(filename));
mExportView.onExportFinish(uri);
} else if (isEmpty[0]) {
mExportView.onNoItemsToExport();
} else {
mExportView.onExportError();
}
}
}.execute();
}
}

Expand Down
18 changes: 5 additions & 13 deletions app/src/main/java/org/glucosio/android/tools/ReadingToCSV.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,15 @@
import java.io.File;
import java.io.FileOutputStream;
import java.io.OutputStreamWriter;
import java.util.ArrayList;
import java.util.List;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!


import io.realm.Realm;

public class ReadingToCSV {

private Context context;

public ReadingToCSV(Context mContext) {
this.context = mContext;
}


public String createCSVFile(Realm realm, final ArrayList<GlucoseReading> readings, String um) {
public static String createCSVFile(Context context, Realm realm, final List<GlucoseReading> readings, String um) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why static method instead of class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not required to instantiate the class/object for the current usage, you only wish to "consume" code residing in it, which really makes it more of a helper class.
That being said I should've declared the class final, will do.
This way you ensure fewer resources since the system will create the reference in compile time instead of runtime.

http://stackoverflow.com/a/218761/3374428
http://stackoverflow.com/a/5181618/3374428

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear!

I would not bother about performance unless you show me it is the bottle neck or some problem here. Because this again breaks testability. With this solution, I can not mock this behaviour in unit tests. So if I test any class that uses this functionality, I will also test real exporting as well.

An article about "love" to statics http://www.endran.nl/blog/death-to-statics/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always worry about performance, this can of course be tested if we change the AsyncTask to a Factory class/public method (as it should be).
With the current setup, no you can't because you are receiving the clicked event and create the AsyncTask there, not a good pattern. And the fix per se would get verbose, this would be my first contribution I don't think is a good atitude and change a lot, it may confuse/upset current contributors.

Every kitten grows up to be a cat. They seem so harmless at first--small, quiet, lapping up their saucer of milk. But once their claws get long enough, they draw blood, sometimes from the hand that feeds them.

That being said, static elements are awesome when used right, they go both ways, if we ignore performance we'll have the same problems ;).

try {
final File file = new File(Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS) + "/glucosio", "glucosio_export_ " + System.currentTimeMillis()/1000 + ".csv");
final File file = new File(Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS) + "/glucosio", "glucosio_export_ " + System.currentTimeMillis() / 1000 + ".csv");
final File sd = Environment.getExternalStorageDirectory();
if (sd.canWrite()) {
FileOutputStream fileOutputStream = new FileOutputStream(file);
Expand Down Expand Up @@ -84,13 +77,13 @@ public String createCSVFile(Realm realm, final ArrayList<GlucoseReading> reading
osw.append(dateTool.convertRawTime(reading.getCreated() + ""));
osw.append(',');

osw.append(reading.getReading() + "");
osw.append(String.valueOf(reading.getReading()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

osw.append(',');

osw.append("mg/dL");
osw.append(',');

osw.append(reading.getReading_type() + "");
osw.append(String.valueOf(reading.getReading_type()));
osw.append(',');

osw.append(reading.getNotes());
Expand Down Expand Up @@ -122,7 +115,6 @@ public String createCSVFile(Realm realm, final ArrayList<GlucoseReading> reading

}
}

osw.flush();
osw.close();
Log.i("Glucosio", "Done exporting readings");
Expand Down
17 changes: 17 additions & 0 deletions app/src/main/java/org/glucosio/android/view/ExportView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package org.glucosio.android.view;

import android.net.Uri;

/**
* Created by joaquimley on 08/09/16.
*/
public interface ExportView {

void onExportStarted(int numberOfItemsToExport);

void onNoItemsToExport();

void onExportFinish(Uri fileUri);

void onExportError();
}
Loading