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

Jump to bottom

Add import data #1333

Merged
merged 11 commits into from
Jul 16, 2021
Merged

Add import data #1333

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
61f44af
Add import data
Feb 25, 2021
f0ccfa1
Import changes
Feb 26, 2021
21c6bdf
Changing noteFromJson to return Note
Feb 26, 2021
c5e2bdd
Import changes
Mar 1, 2021
4857d46
Import markdown check
Mar 2, 2021
cdab029
WIP: Add import functionality into app
dmsnell Mar 16, 2021
5f8a42e
Remove unused references
dmsnell Mar 16, 2021
cb158b0
Importer changes
Mar 19, 2021
ade7ae0
Revert "Importer changes"
Mar 20, 2021
0ea324a
Added import file validation
Mar 21, 2021
7d93384
Change import error messages
Mar 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
120 changes: 120 additions & 0 deletions Simplenote/src/main/java/com/automattic/simplenote/PreferencesFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import android.os.AsyncTask;
import android.os.Bundle;
import android.os.ParcelFileDescriptor;
import android.webkit.MimeTypeMap;
import android.widget.Toast;

import androidx.appcompat.app.AlertDialog;
Expand All @@ -26,6 +27,7 @@
import com.automattic.simplenote.utils.AuthUtils;
import com.automattic.simplenote.utils.BrowserUtils;
import com.automattic.simplenote.utils.CrashUtils;
import com.automattic.simplenote.utils.DateTimeUtils;
import com.automattic.simplenote.utils.HtmlCompat;
import com.automattic.simplenote.utils.PrefUtils;
import com.simperium.Simperium;
Expand All @@ -35,10 +37,17 @@
import com.simperium.client.User;

import org.json.JSONArray;
import org.json.JSONException;
import org.json.JSONObject;

import java.io.BufferedReader;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.Calendar;
import java.util.Collections;
import java.util.Comparator;
import java.util.List;
Expand Down Expand Up @@ -66,6 +75,7 @@ public class PreferencesFragment extends PreferenceFragmentCompat implements Use

private static final int REQUEST_EXPORT_DATA = 9001;
private static final int REQUEST_EXPORT_UNSYNCED = 9002;
private static final int REQUEST_IMPORT_DATA = 9003;

private Bucket<Preferences> mPreferencesBucket;
private SwitchPreferenceCompat mAnalyticsSwitch;
Expand Down Expand Up @@ -147,6 +157,18 @@ public boolean onPreferenceClick(Preference preference) {
}
});

findPreference("pref_key_import").setOnPreferenceClickListener(new Preference.OnPreferenceClickListener() {
@Override
public boolean onPreferenceClick(Preference preference) {
Intent intent = new Intent(Intent.ACTION_OPEN_DOCUMENT);
intent.addCategory(Intent.CATEGORY_OPENABLE);
intent.setType("*/*");
intent.putExtra(Intent.EXTRA_MIME_TYPES, new String[]{"text/*", "application/json"});
startActivityForResult(intent, REQUEST_IMPORT_DATA);
return true;
}
});

findPreference("pref_key_export").setOnPreferenceClickListener(new Preference.OnPreferenceClickListener() {
@Override
public boolean onPreferenceClick(Preference preference) {
Expand Down Expand Up @@ -321,6 +343,9 @@ public void onActivityResult(int requestCode, int resultCode, Intent resultData)
case REQUEST_EXPORT_UNSYNCED:
exportData(resultData.getData(), true);
break;
case REQUEST_IMPORT_DATA:
importData(resultData.getData());
break;
}
}

Expand Down Expand Up @@ -456,6 +481,101 @@ public int compare(String text1, String text2) {
}
}

private void importData(Uri uri) {
try {
String exportContents = readFile(uri);
switch (getFileExtension(uri)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried confirming the return values for getType(uri) and didn't see anything helpful. did you see a list? I saw that it does chop off the first part of the mime type.

we probably want to add unit tests to cover this function for the expected file types we will support importing. can it really suggest md as the extension? I guess if it detects plaintext and has .md?

this is a nice update from before though. I like that there's some leeway if someone has a file with a missing or wrong file extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I misunderstood, but I guess it works a little differently. It seems to me that it makes no difference to us what extension the file is, except for json, of course. All the rest will definitely be text (this is exactly what mimes are for), because I set mime types application/json and text/*, which includes txt, md, html and other, but there's no difference for us between them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's a list (section "common examples"): en.wikipedia.org/wiki/Media_type

Copy link
Contributor

Choose a reason for hiding this comment

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

my point was that I don't see how it's possible here to ever match on "md", unless Android is making a guess on that which I don't think it would?

there is a distinction that's possible, though we could handle all text/plain files the same way, but that distinction is whether or not we toggle markdown support in the note. if we happen to know it's a .md file and it's plaintext then why not go ahead and import it with markdown enabled. otherwise, if it's plaintext import it just as raw text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I understand, I apologize for what you had to explain. However, unfortunately, I still did not understand what the problem was with the written code, I added the method getFileExtension(Context context, Uri uri) that gets the file extension from uri. getType(uri) returns not an extension, but a mimetype, which I'm using for getExtensionFromMimeType(mimeType)

Copy link
Contributor

Choose a reason for hiding this comment

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

here's the central question: can getExtensionFromMimeType(mimeType) ever return "md"?

the code is written as if it can, but I want to make sure that's the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, of course I checked!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, should I add a parameter boolean isMarkdownEnabled to importPlaintextFile(String exportContents) method?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think so, or possibly better a string or enum WithMarkdown | WithoutMarkdown to avoid dangling and curious true values. I don't foresee any other options we'd want to pass in there such as tags or share urls so it should be good to introduce this one flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good, did! It seemed to me, that enum will be better than a string

case "json":
importJsonExport(exportContents);
break;
case "txt":
importPlaintextFile(exportContents);
break;
case "md":
importPlaintextFile(exportContents);
break;
default:
Toast.makeText(requireContext(), getString(R.string.import_unknown), Toast.LENGTH_SHORT).show();
return;
}
Toast.makeText(requireContext(), getString(R.string.import_message_success), Toast.LENGTH_SHORT).show();
} catch (Exception e) {
Toast.makeText(requireContext(), getString(R.string.import_message_failure), Toast.LENGTH_SHORT).show();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this catch is uncomfortably broad. can we separate the exceptions from readFile from the exceptions from the importers? additionally, we have an AppLog that would be useful here for debugging issues that appear during the import. if you aren't sure how to add that we can do that. let's add a log when starting the import, indicating which type, and when it succeeds and fails. it would either fit as type ACTION or we might create a new type, IMPORT

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 add exception handling. I think it's better to add the logs to you, because I don't know how to do it most correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

we'll take care of them, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good, thank you too!

}

private void importPlaintextFile(String exportContents) {
Simplenote currentApp = (Simplenote) requireActivity().getApplication();
Bucket<Note> noteBucket = currentApp.getNotesBucket();
Note note = noteBucket.newObject();
note.setContent(exportContents);
note.setCreationDate(Calendar.getInstance());
note.setModificationDate(note.getCreationDate());
note.save();
}

private void importJsonExport(String exportContents) throws Exception {
JSONObject jsonData = new JSONObject(exportContents);
JSONArray activeNotes = jsonData.getJSONArray("activeNotes");
JSONArray trashedNotes = jsonData.getJSONArray("trashedNotes");
List<String> trashedIds = new ArrayList<>();
JSONArray notesArray = new JSONArray();
for (int i = 0; i < activeNotes.length(); i++) {
notesArray.put(activeNotes.getJSONObject(i));
}
for (int j = 0; j < trashedNotes.length(); j++) {
notesArray.put(trashedNotes.getJSONObject(j));
trashedIds.add(trashedNotes.getJSONObject(j).getString("id"));
}
for (int k = 0; k < notesArray.length(); k++) {
JSONObject noteJson = notesArray.getJSONObject(k);
noteFromJson(noteJson, trashedIds).save();
}
}

private Note noteFromJson(JSONObject noteJson, List<String> trashedIds) throws Exception {
Simplenote currentApp = (Simplenote) requireActivity().getApplication();
Bucket<Note> noteBucket = currentApp.getNotesBucket();
Note note = noteBucket.newObject();
note.setContent(noteJson.has("content") ? noteJson.getString("content") : "");
note.setCreationDate(noteJson.has("creationDate") ? DateTimeUtils.getDateCalendar(noteJson.getString("creationDate")) : Calendar.getInstance());
note.setModificationDate(noteJson.has("lastModified") ? DateTimeUtils.getDateCalendar(noteJson.getString("lastModified")) : Calendar.getInstance());
note.setTags(noteJson.has("tags") ? jsonToList(noteJson.getJSONArray("tags")) : new ArrayList<String>());
note.setPinned(noteJson.has("pinned") && noteJson.getBoolean("pinned"));
note.setMarkdownEnabled(noteJson.has("markdown") && noteJson.getBoolean("markdown"));
note.setDeleted(noteJson.has("id") && trashedIds.contains(noteJson.getString("id")));
Copy link
Contributor

Choose a reason for hiding this comment

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

not going to import any chords, tracks, or photos? 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😁 Thank you for your attention to my project! I really like Simplenote, that's why I took it as a basis. I use it every day, thank you very much for your work, I am very grateful to you!

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, what about merge? 😀

Copy link
Contributor

@dmsnell dmsnell Mar 1, 2021 •
edited

Choose a reason for hiding this comment

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

this is going to have some issues as I don't know that we can or should rely on imported notes containing an id. I believe that it's not really necessary anyway, more a byproduct of the way we've chosen to build an intermediate list of notes on import.

instead we could traverse each category of notes once and avoid needing to pass in List<String> trashedIds. there are a couple ways we can convey the trashed status but one easy way to do it would having to think about it much is to manually alter the JSON or the output note.

for (int i = 0; i < activeNotes.length(); i++) {
	noteFromJson(activeNotes.getJSONObject(i)).save();
}

for (int i = 0; i < trashedNotes.length(); i++) {
	Note note = noteFromJson(trashedNotes.getJSONObject(i));
	note.setDeleted(true);
	note.save();
}

(edit: corrected typo from notesArray to activeNotes/trashedNotes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I had doubts about this piece of code, so much better, thanks!

return note;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

looking at this now it does feel like it belongs in Note.java more than it does here. would you be willing to move it into that module and then import it? if now we can do that before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is much better, but I want to note that I move it there as a static method.


private List<String> jsonToList(JSONArray json) {
List<String> list = new ArrayList<>();
for (int i = 0; i < json.length(); i++) {
try {
list.add(json.getString(i));
} catch (JSONException e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

we'll want to match our error logging with the rest of the app vs. dumping a stack trace.

in addition, it looks like the Note class stores its list of tags as a JSONArray so we could expose a new method for setTags(JSONArray tags) and skip the encode/decode loop.

on this point, are we able to directly instantiate a note with the existing Note class, without conversion? I doubt it works 100% but the constructor can take a JSON object. it may be the case that we can perform minimal transformation and/or slightly modify Note.java so we can go straight from the imported JSON object to a note.

new Note(activeNotes.getObject(i)).save();

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 changed tags adding, but I doubt about the note. In theory, we can use the following code:

Simplenote currentApp = (Simplenote) activity.getApplication();
Bucket<Note> noteBucket = currentApp.getNotesBucket();
Note note = new Note("note - " + UUID.randomUUID(), activeNotes.getJSONObject(i));
noteBucket.add(note);
note.save();

It looks much better, but I doubt this is a good idea, since I am not sure how correctly the Ыimperium alogyrythms will work in this case. It seems to me that it is preferable to use noteBucket.newObject();

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'll leave this as it was for now, and then we'll decide!

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks. I might need to consult with a few people on this. please pardon the delay, but expect a few days gap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, no problem!

}
}
return list;
}

private String readFile(Uri uri) throws IOException {
StringBuilder stringBuilder = new StringBuilder();
InputStream inputStream = requireContext().getContentResolver().openInputStream(uri);
BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream));
String line;
while ((line = reader.readLine()) != null) {
stringBuilder.append(line);
stringBuilder.append("\n");
}
inputStream.close();
return stringBuilder.toString();
}

public String getFileExtension(Uri uri) {
return MimeTypeMap.getSingleton().getExtensionFromMimeType(requireContext().getContentResolver().getType(uri));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

these two might be great in a FileUtils class in the utils folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Wanted to do it but was afraid to create new files.

Copy link
Contributor

Choose a reason for hiding this comment

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

all good. I know that fear, especially entering an open-source repo for new contributions.

we'll let you know if we want something to change, or we'll just end up changing it on merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, thank you! 😀


private void trackSortOrder(String label) {
AnalyticsTracker.track(
AnalyticsTracker.Stat.SETTINGS_SEARCH_SORT_MODE,
Expand Down
9 changes: 9 additions & 0 deletions Simplenote/src/main/java/com/automattic/simplenote/utils/DateTimeUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import android.text.format.DateFormat;
import android.text.format.DateUtils;

import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.util.Calendar;
import java.util.Locale;
Expand Down Expand Up @@ -37,4 +38,12 @@ public static String getDateTextString(Context context, Calendar calendar) {
);
return new SimpleDateFormat(pattern, Locale.getDefault()).format(calendar.getTime());
}

public static Calendar getDateCalendar(String json) throws ParseException {
String pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like an ISO8601 string. I see parseOffsetISO8601 in SDK 24 which I guess means we can't use that here. I looked briefly through androidx and didn't see one. do you know if there's an existing ISO8601 parser available in one of the compatibility or other libraries we already use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll look for solutions!

SimpleDateFormat dateFormat = new SimpleDateFormat(pattern, Locale.getDefault());
Calendar date = Calendar.getInstance();
date.setTime(dateFormat.parse(json));
return date;
}
}
4 changes: 4 additions & 0 deletions Simplenote/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,12 @@

<!-- Preferences -->
<string name="export_file">simplenote.json</string>
<string name="import_message_failure">Data could not be loaded</string>
<string name="import_message_success">Data loaded</string>
<string name="import_unknown">Unknown format</string>
<string name="export_message_failure">Data could not be saved</string>
<string name="export_message_success">Data saved to file</string>
<string name="import_setting">Import data</string>
<string name="export_setting">Export data</string>
<string name="editor">Editor</string>
<string name="condensed_note_list">Condensed note list</string>
Expand Down
5 changes: 5 additions & 0 deletions Simplenote/src/main/res/xml/preferences.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,11 @@
tools:title="@string/log_out">
</Preference>

<Preference
android:key="pref_key_import"
android:title="@string/import_setting">
</Preference>

<Preference
android:key="pref_key_export"
android:title="@string/export_setting">
Expand Down