Merge lp:~vbkaisetsu/tomdroid/search-without-xml into lp:tomdroid/beta

Proposed by Koichi Akabe
Status: Merged
Merged at revision: 460
Proposed branch: lp:~vbkaisetsu/tomdroid/search-without-xml
Merge into: lp:tomdroid/beta
Diff against target: 225 lines (+107/-8)
4 files modified
src/org/tomdroid/Note.java (+1/-0)
src/org/tomdroid/NoteManager.java (+12/-6)
src/org/tomdroid/NoteProvider.java (+57/-2)
src/org/tomdroid/util/StringConverter.java (+37/-0)
To merge this branch: bzr merge lp:~vbkaisetsu/tomdroid/search-without-xml
Reviewer Review Type Date Requested Status
Tomdroid Maintainers Pending
Review via email: mp+123400@code.launchpad.net

This proposal supersedes a proposal from 2011-10-27.

Description of the change

This branch fixes following things:
    - Bug #562089: the search-bar search also XML tags Remove
    - Can't search words which contains "%" or "_"

The old branch(*) also fix the following bugs, but they are already fixed on beta:

    - The list always shows system:template notes
    - Bug #880322: SQL query should use "?" and selectionArgs Remove

Please consider merging it.

(*) old branch: lp:~vbkaisetsu/tomdroid/sql-args

To post a comment you must log in.
Revision history for this message
Piotr Adamski (mcveat) wrote : Posted in a previous version of this proposal

Please consider using StringBuilder for constructing where clause and wrapping it in separate function to increase readability and divide responsibilities a bit.

Revision history for this message
Koichi Akabe (vbkaisetsu) wrote : Posted in a previous version of this proposal

Hello Piotr,

Thanks your opinion.
I changed the code to use StringBuilder.

Regards,
Koichi Akabe.

Revision history for this message
Piotr Adamski (mcveat) wrote : Posted in a previous version of this proposal

Great, Koichi! But your fix needs one more change in my opinion. The point of using StringBuilder is to avoid explicit string concatenation, as each produces new String in strings pool. It may strike back as memory issue, so it is worth to avoid them in cases like this, where there is a lot string operations. Consider replacing each concatenation with append method call. I know it doesn't look so concise, but in that situation i think it is worth it.

Regards
Piotr

Revision history for this message
Koichi Akabe (vbkaisetsu) wrote : Posted in a previous version of this proposal

I think constants' normal operation is faster than using StringBuffer.
Because they are calculated in compiling.

Note.XXXXXs are defined with final, so they are constant.

Revision history for this message
Koichi Akabe (vbkaisetsu) wrote : Posted in a previous version of this proposal

The commit message is wrong: s/restore/store/

Sorry.

Revision history for this message
Stefan Hammer (j-4-deactivatedaccount) wrote : Posted in a previous version of this proposal

Hi! The bug where searching for tags shows all notes is finally gone - great!
But somehow, the test notebook templates still show up in the notes list and in the search on my AVDevices. (even after complete reinstall.)
Do you have any idea?

Revision history for this message
Koichi Akabe (vbkaisetsu) wrote : Posted in a previous version of this proposal

In my environment, templates have "system:template" tag and they are not shown on the list.
Please debug it and tell me more information.

Revision history for this message
Stefan Hammer (j-4-deactivatedaccount) wrote : Posted in a previous version of this proposal

Hi! I use Android 1.6 AVD, the sdcard image (sdcard.img) from our sources and exactly your code branch, with additional log entries. I still have both two test notebook templates in the list.

In this templates the tags look like:
  <tags>

    <tag>system:template</tag>

    <tag>system:notebook:test</tag>

  </tags>

  <tags>
    <tag>system:notebook:Hipo</tag>
    <tag>system:template</tag>
  </tags>

My log gives:
10-30 11:04:27.107: I/Tomdroid(486): Tomdroid is first run.
10-30 11:04:27.227: V/FirstNote(486): Creating first note
10-30 11:04:27.537: V/NoteManager(486): A new note has been detected (not yet in db)
10-30 11:04:27.547: V/NoteManager(486): Note inserted in content provider. ID: content://org.tomdroid.notes/notes/1 TITLE:Tomdroid's first note GUID:8f837a99-c920-4501-b303-6a39af57a714
10-30 11:04:27.626: V/NoteManager(486): where is: tags NOT LIKE '%system:template%'
10-30 11:04:27.626: V/NoteManager(486): TAGS is: tags
10-30 11:04:52.287: V/FirstNote(486): Creating first note
10-30 11:04:52.556: V/NoteManager(486): A new note has been detected (not yet in db)
10-30 11:04:52.566: V/NoteManager(486): Note inserted in content provider. ID: content://org.tomdroid.notes/notes/1 TITLE:Tomdroid's first note GUID:8f837a99-c920-4501-b303-6a39af57a714
10-30 11:04:55.497: V/SdCardSyncService(486): Loading local notes
10-30 11:04:55.497: I/SdCardSyncService(486): Path /sdcard/tomdroid exists: true

It works when syncing the AVD with Ubuntu One, using my own notes. One difference is, that there i do not use notebooks.
So, the tags look like:

  <tags>
    <tag>system:template</tag>
  </tags>

10-30 11:34:44.367: V/FirstNote(652): Creating first note
10-30 11:34:44.786: V/NoteManager(652): A new note has been detected (not yet in db)
10-30 11:34:44.797: V/NoteManager(652): Note inserted in content provider. ID: content://org.tomdroid.notes/notes/1 TITLE:Tomdroid's first note GUID:8f837a99-c920-4501-b303-6a39af57a714
10-30 11:35:30.927: V/NoteManager(652): where is: tags NOT LIKE '%system:template%'
10-30 11:35:30.927: V/NoteManager(652): query is: null

Does this help?

Revision history for this message
Koichi Akabe (vbkaisetsu) wrote : Posted in a previous version of this proposal

Notes list always shows templates on SD card, because Tomdroid doesn't check "tags" tag.

I registered another branch to solve it.
https://code.launchpad.net/~vbkaisetsu/tomdroid/sdcard-tags

Thanks for reporting!

Revision history for this message
Stefan Hammer (j-4-deactivatedaccount) wrote : Posted in a previous version of this proposal

Thanks for your work. I already know the code very well and I think it's awesome. This is something we definitively have to include in the next release.
The new sql query system is much simpler/smarter and that notebook templates are in the database, but not shown - just as it should be ;-)

Problem is... I have never merged any important code and I am not sure, what Olivier's reviewing checklist/merging criteria contains. Therefore I suggest to wait for his opinion. If he is fine with it, I will merge immediately.

There was a idea coming up, when testing this stuff: We now store all tags, including the "system:notebook:****" tag. If you include Note.TAGS in the search query, it would be very easy to display all notes of one notebooks, just with the query "notebook:mynotebookname". Of course, we would have to think about a way to exclude queries like notebook, system or template from searching in Note.TAGS... But hey, intelligent searching! Further... how about title:mynotetitle?? :-D ... but this would be something for a new project, if you feel like it!

Revision history for this message
Koichi Akabe (vbkaisetsu) wrote : Posted in a previous version of this proposal

Hello Stefan,

It's wonderful but there are many problems for notebooks.
We have to register another branch to solve them.

Revision history for this message
Koichi Akabe (vbkaisetsu) wrote : Posted in a previous version of this proposal

I added notebook support to getListAdapter(), and you can search "%" and "_" now.
This code encodes some symbols of notes before store them.

I think this code is dirty. Please review it.

Revision history for this message
Olivier Bilodeau (plaxx) wrote : Posted in a previous version of this proposal

Doesn't automatically upgrade the SQL database. This can't be merged as-is.

Also, I would like to see the query arguments to be migrated into ? and selectionArgs as query() supports. docs/reference/android/database/sqlite/SQLiteDatabase.html#query(java.lang.String, java.lang.String[], java.lang.String, java.lang.String[], java.lang.String, java.lang.String, java.lang.String, java.lang.String)

review: Needs Fixing
Revision history for this message
Koichi Akabe (vbkaisetsu) wrote : Posted in a previous version of this proposal

changed onUpgrade to support update from old versions 2 or 3. But version 1 dosn't have GUID and note contents (title only) so it doesn't support.

Revision history for this message
Olivier Bilodeau (plaxx) wrote : Posted in a previous version of this proposal

w/o looking at the code, I would say not upgrading from 1 is ok.

What I'm really concerned about is the 0.4.1 users on the android market. I want them to upgrade cleanly. Pretty sure this fixes it.

Revision history for this message
Stefan Hammer (j-4-deactivatedaccount) wrote : Posted in a previous version of this proposal

Hi!

Since our new beta, a lot of things changed. However, we still need to fix parts of the things treated in this branch.
    - Bug #562089: the search-bar search also XML tags Remove
    - Bug #880322: SQL query should use "?" and selectionArgs Remove
    - Can't search words which contains "%" or "_"

All the other features are already implemented (i think)
As I assume that this branch won't merge with the new beta it is probably best to rewrite these things too (or copy/paste the neccesary stuff into the new beta code).

@Koichi: do you want to fix this three issues in a new branch, or do you prefere if somebody else does it?

Revision history for this message
Koichi Akabe (vbkaisetsu) wrote : Posted in a previous version of this proposal

I hope to fix this branch, but the other hand, it's hard work.
I'll appreciate someone's help.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/org/tomdroid/Note.java'
2--- src/org/tomdroid/Note.java 2012-09-07 04:55:06 +0000
3+++ src/org/tomdroid/Note.java 2012-09-08 03:43:19 +0000
4@@ -48,6 +48,7 @@
5 public static final String FILE = "file";
6 public static final String TAGS = "tags";
7 public static final String NOTE_CONTENT = "content";
8+ public static final String NOTE_CONTENT_PLAIN = "content_plain";
9
10 // Notes constants
11 public static final int NOTE_HIGHLIGHT_COLOR = 0x99FFFF00; // lowered alpha to show cursor
12
13=== modified file 'src/org/tomdroid/NoteManager.java'
14--- src/org/tomdroid/NoteManager.java 2012-09-07 08:55:07 +0000
15+++ src/org/tomdroid/NoteManager.java 2012-09-08 03:43:19 +0000
16@@ -34,6 +34,7 @@
17 import android.database.Cursor;
18 import android.net.Uri;
19 import android.os.Bundle;
20+import android.text.Html;
21 import android.text.format.Time;
22 import android.text.util.Linkify.TransformFilter;
23 import android.widget.ListAdapter;
24@@ -44,6 +45,7 @@
25 import org.tomdroid.ui.Tomdroid;
26 import org.tomdroid.util.NoteListCursorAdapter;
27 import org.tomdroid.util.Preferences;
28+import org.tomdroid.util.StringConverter;
29 import org.tomdroid.util.TLog;
30 import org.tomdroid.util.XmlUtils;
31
32@@ -174,16 +176,21 @@
33 whereArgs,
34 null);
35 activity.startManagingCursor(managedCursor);
36+
37+ String title = note.getTitle();
38+ String xmlContent = note.getXmlContent();
39+ String plainContent = StringConverter.encode(Html.fromHtml(title + "\n" + xmlContent).toString());
40
41 // Preparing the values to be either inserted or updated
42 // depending on the result of the previous query
43 ContentValues values = new ContentValues();
44- values.put(Note.TITLE, note.getTitle());
45+ values.put(Note.TITLE, title);
46 values.put(Note.FILE, note.getFileName());
47 values.put(Note.GUID, note.getGuid().toString());
48 // Notice that we store the date in UTC because sqlite doesn't handle RFC3339 timezone information
49 values.put(Note.MODIFIED_DATE, note.getLastChangeDate().format3339(false));
50- values.put(Note.NOTE_CONTENT, note.getXmlContent());
51+ values.put(Note.NOTE_CONTENT, xmlContent);
52+ values.put(Note.NOTE_CONTENT_PLAIN, plainContent);
53 values.put(Note.TAGS, note.getTags());
54
55 Uri uri = null;
56@@ -347,11 +354,10 @@
57 if (querys != null ) {
58 // sql statements to search notes
59 String[] query = querys.split(" ");
60- qargs = new String[query.length*2+optionalQueries];
61+ qargs = new String[query.length+optionalQueries];
62 for (String string : query) {
63- qargs[count++] = "%"+string+"%";
64- qargs[count++] = "%"+string+"%";
65- where = where + (where.length() > 0? " AND ":"")+"("+Note.TITLE+" LIKE ? OR "+Note.NOTE_CONTENT+" LIKE ?)";
66+ qargs[count++] = "%"+StringConverter.encode(string)+"%";
67+ where = where + (where.length() > 0? " AND ":"")+"("+Note.NOTE_CONTENT_PLAIN+" LIKE ?)";
68 }
69 }
70 else
71
72=== modified file 'src/org/tomdroid/NoteProvider.java'
73--- src/org/tomdroid/NoteProvider.java 2012-07-17 16:15:05 +0000
74+++ src/org/tomdroid/NoteProvider.java 2012-09-08 03:43:19 +0000
75@@ -51,11 +51,15 @@
76 import android.database.sqlite.SQLiteOpenHelper;
77 import android.database.sqlite.SQLiteQueryBuilder;
78 import android.net.Uri;
79+import android.text.Html;
80 import android.text.TextUtils;
81 import org.tomdroid.ui.Tomdroid;
82+import org.tomdroid.util.StringConverter;
83 import org.tomdroid.util.TLog;
84
85+import java.util.ArrayList;
86 import java.util.HashMap;
87+import java.util.Map;
88 import java.util.UUID;
89
90 public class NoteProvider extends ContentProvider {
91@@ -64,7 +68,7 @@
92 // --
93 private static final String DATABASE_NAME = "tomdroid-notes.db";
94 private static final String DB_TABLE_NOTES = "notes";
95- private static final int DB_VERSION = 3;
96+ private static final int DB_VERSION = 4;
97 private static final String DEFAULT_SORT_ORDER = Note.MODIFIED_DATE + " DESC";
98
99 private static HashMap<String, String> notesProjectionMap;
100@@ -77,6 +81,14 @@
101
102 // Logging info
103 private static final String TAG = "NoteProvider";
104+
105+ // List of each version's columns
106+ private static final String[][] COLUMNS_VERSION = {
107+ { Note.TITLE, Note.FILE, Note.MODIFIED_DATE },
108+ { Note.GUID, Note.TITLE, Note.FILE, Note.NOTE_CONTENT, Note.MODIFIED_DATE },
109+ { Note.GUID, Note.TITLE, Note.FILE, Note.NOTE_CONTENT, Note.MODIFIED_DATE, Note.TAGS },
110+ { Note.GUID, Note.TITLE, Note.FILE, Note.NOTE_CONTENT, Note.NOTE_CONTENT_PLAIN, Note.MODIFIED_DATE, Note.TAGS }
111+ };
112
113 /**
114 * This class helps open, create, and upgrade the database file.
115@@ -95,6 +107,7 @@
116 + Note.TITLE + " TEXT,"
117 + Note.FILE + " TEXT,"
118 + Note.NOTE_CONTENT + " TEXT,"
119+ + Note.NOTE_CONTENT_PLAIN + " TEXT,"
120 + Note.MODIFIED_DATE + " STRING,"
121 + Note.TAGS + " STRING"
122 + ");");
123@@ -102,10 +115,51 @@
124
125 @Override
126 public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {
127- TLog.d(TAG, "Upgrading database from version {0} to {1}, which will destroy all old data",
128+ TLog.d(TAG, "Upgrading database from version {0} to {1}",
129 oldVersion, newVersion);
130+ Cursor notesCursor;
131+ ArrayList<Map<String, String>> db_list = new ArrayList<Map<String, String>>();
132+ notesCursor = db.query(DB_TABLE_NOTES, COLUMNS_VERSION[oldVersion - 1], null, null, null, null, null);
133+ notesCursor.moveToFirst();
134+
135+ if (oldVersion == 1) {
136+ // GUID and NOTE_CONTENT are not saved.
137+ TLog.d(TAG, "Database version {0} is not supported to update, all old datas will be destroyed", oldVersion);
138+ db.execSQL("DROP TABLE IF EXISTS notes");
139+ onCreate(db);
140+ return;
141+ }
142+
143+ // Get old datas from the SQL
144+ while(!notesCursor.isAfterLast()) {
145+ Map<String, String> row = new HashMap<String, String>();
146+ for(int i = 0; i < COLUMNS_VERSION[oldVersion - 1].length; i++) {
147+ row.put(COLUMNS_VERSION[oldVersion - 1][i], notesCursor.getString(i));
148+ }
149+
150+ // create new columns
151+ if (oldVersion <= 2) {
152+ row.put(Note.TAGS, "");
153+ }
154+ if (oldVersion <= 3) {
155+ row.put(Note.NOTE_CONTENT_PLAIN, StringConverter.encode(Html.fromHtml(row.get(Note.TITLE) + "\n" + row.get(Note.NOTE_CONTENT)).toString()));
156+ }
157+
158+ db_list.add(row);
159+ notesCursor.moveToNext();
160+ }
161+
162 db.execSQL("DROP TABLE IF EXISTS notes");
163 onCreate(db);
164+
165+ // put rows to the database
166+ ContentValues row = new ContentValues();
167+ for(int i = 0; i < db_list.size(); i++) {
168+ for(int j = 0; j < COLUMNS_VERSION[newVersion - 1].length; j++) {
169+ row.put(COLUMNS_VERSION[newVersion - 1][j], db_list.get(i).get(COLUMNS_VERSION[newVersion - 1][j]));
170+ }
171+ db.insert(DB_TABLE_NOTES, null, row);
172+ }
173 }
174 }
175
176@@ -291,6 +345,7 @@
177 notesProjectionMap.put(Note.TITLE, Note.TITLE);
178 notesProjectionMap.put(Note.FILE, Note.FILE);
179 notesProjectionMap.put(Note.NOTE_CONTENT, Note.NOTE_CONTENT);
180+ notesProjectionMap.put(Note.NOTE_CONTENT_PLAIN, Note.NOTE_CONTENT_PLAIN);
181 notesProjectionMap.put(Note.TAGS, Note.TAGS);
182 notesProjectionMap.put(Note.MODIFIED_DATE, Note.MODIFIED_DATE);
183 }
184
185=== added file 'src/org/tomdroid/util/StringConverter.java'
186--- src/org/tomdroid/util/StringConverter.java 1970-01-01 00:00:00 +0000
187+++ src/org/tomdroid/util/StringConverter.java 2012-09-08 03:43:19 +0000
188@@ -0,0 +1,37 @@
189+/*
190+ * Tomdroid
191+ * Tomboy on Android
192+ * http://www.launchpad.net/tomdroid
193+ *
194+ * Copyright 2011 Koichi Akabe <vbkaisetsu@gmail.com>
195+ *
196+ * This file is part of Tomdroid.
197+ *
198+ * Tomdroid is free software: you can redistribute it and/or modify
199+ * it under the terms of the GNU General Public License as published by
200+ * the Free Software Foundation, either version 3 of the License, or
201+ * (at your option) any later version.
202+ *
203+ * Tomdroid is distributed in the hope that it will be useful,
204+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
205+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
206+ * GNU General Public License for more details.
207+ *
208+ * You should have received a copy of the GNU General Public License
209+ * along with Tomdroid. If not, see <http://www.gnu.org/licenses/>.
210+ */
211+package org.tomdroid.util;
212+
213+public class StringConverter {
214+ public static String encode(String text) {
215+ return text.replace("&", "&amp;")
216+ .replace("%", "&pct;")
217+ .replace("_", "&und;");
218+ }
219+
220+ public static String decode(String text) {
221+ return text.replace("&und;", "_")
222+ .replace("&pct;", "%")
223+ .replace("&amp;", "&");
224+ }
225+}

Subscribers

People subscribed via source and target branches