-
Notifications
You must be signed in to change notification settings - Fork 73
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.
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
Implemented a basic json logger #9
base: master
Are you sure you want to change the base?
Conversation
Hey Jonathan, thanks for your contribution! I'll look into this on the weekend and get back to you. Regarding the deprecated |
Hey Stephan,
I tried to update the libraries but to be honest I am not an expert on Gradle or the Android Build ecosystem. So I naturally ran into lots of problems and gave up after ca. 2 hours of trying. Instead I opted to run the old build as described. But most likely someone with more experience can sort it out easily ... or not (as I cannot know). 😉 Cheers, Jonathan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a few suggestions. Please don't feel obliged to implement them all, it's just what came to my mind looking at the PR.
Again, thanks for your contribution!
if (ContextCompat.checkSelfPermission(getApplicationContext(), | ||
Manifest.permission.WRITE_EXTERNAL_STORAGE) | ||
!= PackageManager.PERMISSION_GRANTED) { | ||
if (ActivityCompat.shouldShowRequestPermissionRationale(app.getContextActivity(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Negate this to remove the empty if branch or actually show a rationale
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the empty branch by removing the permission handling. 👍
@Override | ||
public void onClick(View v) { | ||
// request runtime permission | ||
if (ContextCompat.checkSelfPermission(getApplicationContext(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the minimum SDK version is 21, you can probably avoid requesting this permission. According to the documentation:
Starting in API level 19, this permission is not required to read/write files in your application-specific directories returned by Context.getExternalFilesDir(String)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(1/3) Removed the permission checking, good to know that it was not required. 👍
} | ||
|
||
// if user did not grant permission, notify by Toast | ||
if (ContextCompat.checkSelfPermission(getApplicationContext(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this is checked again here. Note that requesting permissions is an asynchronous process, all permission request results will be delivered via a callback method that you have to override (onRequestPermissionsResult
). See requesting runtime permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(2/3) Removed the permission checking, good to know that it was not required. 👍
if (ActivityCompat.shouldShowRequestPermissionRationale(app.getContextActivity(), | ||
Manifest.permission.WRITE_EXTERNAL_STORAGE)) { | ||
} else { | ||
ActivityCompat.requestPermissions(app.getContextActivity(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're currently in the PhoneActivity
, so every time you need a Context
you can simply use this
instead of getApplicationContext()
or app.getContextActivity()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manifest.permission.WRITE_EXTERNAL_STORAGE)) { | ||
} else { | ||
ActivityCompat.requestPermissions(app.getContextActivity(), | ||
new String[]{Manifest.permission.WRITE_EXTERNAL_STORAGE},23 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 23
is the request code that you would get back in the result callback. Although the value itself doesn't matter too much, this looks like a magic number. Usually you would define a constant for that, like private static final int STORAGE_PERMISSION_REQUEST_CODE = 1;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(3/3) Removed the permission checking, good to know that it was not required. 👍
filewriter.write(","); | ||
} | ||
firstEntryWasWritten = true; | ||
filewriter.write(dataBatch.toJson()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing every DataBatch
to a file as soon as its received sounds like a very big overhead. The file will bloat up a lot because of the JSON format and IO operations are quite slow and will block the current thread.
I would recommend collecting multiple of these data batches, merging them together and then flushing them to the file writer occasionally (and when stopping the recording).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented a Queue that is read by a runnable (thread) in the background and flushes the contents to file. 👍
this.filewriter = filewriter; | ||
} | ||
|
||
public String stopRecording () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to startRecording
, I would return the created File
here. Or make it void
and add a getter for the most recently created file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 (Included both suggestions)
private boolean firstEntryWasWritten; | ||
|
||
public void logDataBatch(DataBatch dataBatch, String sourceNodeId) { | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This style of block comment (used for method documentation) needs to be placed right above the method, not in the method body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, wrote to much Python in the past apparently where it is flipped around. 👍
// start a new recording if no recording is going on. | ||
// pass current timestamp as a filename and raise a Toast to notify user of start | ||
// and stop action. Also assign a new icon to the second floating action button. | ||
if (! dataHandler.isRecording()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (! dataHandler.isRecording()) { | |
if (!dataHandler.isRecording()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
public String stopRecording () { | ||
// terminate early if recording was not started beforehand | ||
// (should not happen.) | ||
if (! isRecording) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (! isRecording) | |
if (!isRecording) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Hey, thanks for the feedback, I find it valuable and I will incorporate it once I find the time (probably around the weekend). Thanks! Cheers, Jonathan |
Hi, thanks for the feedback. I have included all of it and pushed a new commit to my branch. 🙂 If you like, take a look. Thanks in advance. PS: I have tested the new implementation on a LG LM-K510 with a Moto 360. |
Hi @Steppschuh,
first of all, thank you for your work with this fantastic app. I took a quick look at your thesis and I have to tell that we share the same goal. In my PhD I focus on getting rid of passwords as well. And for that reason I intend to log some spatial movement through an Android Wear Device (Moto 360 in my case), but I need that data logged to file for a post-hoc analysis. Whilst your app already does an excellent job of visualization, connection etc., I am glad to add this missing feature (#8). 🙂 🚀
Please find a pull request for some basic logging functionality that adds a second floating action button to start and stop the logging of the selected sensors to a JSON file. The file then is stored in Android's external storage in a directory resembling the package name. I also added some example data. It is only a very basic implementation, so please do not judge me (I did some Android in the past, but that's quite some years ago), but I think it does the job and it might be better than nothing. Also I tried to comment/document the most important things in the code.
If you have any comments I am open for feedback. I would be very happy if you could review the changes and in case of approval, please push an update to Google Play.
On a side note, I also had some troubles getting Gradle and the wear-build to work. Reason lies within some wear-library on version
1.4.0
being removed from the mirrors. So I had to fetch the files from an unofficial mirror and place them at this location:%appdata%\..\Local\Android\Sdk\extras\m2repository\com\google\android\support\wearable\1.4.0
(Win10). After that I built the app.Everything was tested on a Sony Xperia XZ2 Compact and a Moto 360 smart watch.
Cheers!