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.

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

Added Cordova 1.5 Support #23

Closed
wants to merge 6 commits into from
Closed

Added Cordova 1.5 Support #23

wants to merge 6 commits into from

Conversation

coomsie
Copy link

@coomsie coomsie commented Mar 27, 2012

Hi,

I've added Cordova 1.5 Support.

I've also renamed a few files as per other peoples suggestions ...

Cheers
Iain

@brodybits
Copy link
Contributor

@coomsie this looks like very good work and I definitely plan to try this later today. I do have a few more suggestions:

  • The name of the Javascript file to include should be like SQLitePlugin.js to be more like the other Cordova/PhoneGap plugins.

- What about people using "legacy" versions of "PhoneGap": do we try to keep a way to support them or do we just say "sorry"?

@brodybits
Copy link
Contributor

and also there is an Android plugin that I believe is following the HTML5 Web SQL API: https://github.com/apache/incubator-cordova-android/blob/master/framework/src/org/apache/cordova/Storage.java; I think it would be nice if we could return an object that follows the same API for the iOS version of Cordova/[PhoneGap].

@brodybits
Copy link
Contributor

I just added pull request #24 to propose a change to the API to come closer to the HTML5/W3 SQL API and the existing Android Storage module. I hope you will consider the changes for the Cordova version.

@coomsie
Copy link
Author

coomsie commented Mar 28, 2012

yeah, fine have added ... #24 changes to latest commit ...
coomsie/Cordova_SQLitePlugin@4b7b181

will add to this branch too ..

@brodybits
Copy link
Contributor

And one more patch for README.md to fix the CoffeeScript example:

$ git diff
diff --git a/README.md b/README.md
index 29ca527..696f2a7 100644
--- a/README.md
+++ b/README.md
@@ -59,7 +59,7 @@ General Usage

     db.transaction (tx) ->

-      tx.executeSql [ "INSERT INTO test_table (data, data_num) VALUES (?,?)", "test", 100], (res) -
+      tx.executeSql "INSERT INTO test_table (data, data_num) VALUES (?,?)", ["test", 100], (res) ->

         # success callback

@@ -67,7 +67,7 @@ General Usage
         console.log "rowsAffected: #{res.rowsAffected} -- should be 1"

         # check the count (not a part of the transaction)
-        db.executeSql "select count(id) as cnt from test_table;", (res) ->
+        db.executeSql "select count(id) as cnt from test_table;", [], (res) ->
           console.log "rows.length: #{res.rows.length} -- should be 1"
           console.log "rows[0].cnt: #{res.rows[0].cnt} -- should be 1"

Thanks @coomsie

@brodybits
Copy link
Contributor

@coomsie I looked again and you missed the changes to src/sqlite_plugin.coffee and src/lawnchair_sqlite_plugin_adapter.coffee

@coomsie
Copy link
Author

coomsie commented Mar 28, 2012

ta, done.

On Wed, Mar 28, 2012 at 9:25 PM, Chris Brody <
reply@reply.github.com

wrote:

And one more patch for README.md to fix the CoffeeScript example:

$ git diff
diff --git a/README.md b/README.md
index 29ca527..696f2a7 100644
--- a/README.md
+++ b/README.md
@@ -59,7 +59,7 @@ General Usage

    db.transaction (tx) ->
  •  tx.executeSql [ "INSERT INTO test_table (data, data_num) VALUES
    

    (?,?)", "test", 100], (res) -

  •  tx.executeSql "INSERT INTO test_table (data, data_num) VALUES
    

    (?,?)", ["test", 100], (res) ->

        # success callback
    

    @@ -67,7 +67,7 @@ General Usage
    console.log "rowsAffected: #{res.rowsAffected} -- should be 1"

        # check the count (not a part of the transaction)
    
  •    db.executeSql "select count(id) as cnt from test_table;",
    

    (res) ->

  •    db.executeSql "select count(id) as cnt from test_table;", [],
    

    (res) ->
    console.log "rows.length: #{res.rows.length} -- should be 1"
    console.log "rows[0].cnt: #{res.rows[0].cnt} -- should be 1"

Thanks @coomsie


Reply to this email directly or view it on GitHub:

#23 (comment)

Cheers
Coomsie

@brodybits
Copy link
Contributor

You still missed the changes to the source files src/sqlite_plugin.coffee and src/lawnchair_sqlite_plugin_adapter.coffee

@coomsie
Copy link
Author

coomsie commented Mar 28, 2012

Ok ta,

I'm not a coffee user. Will change tonight if I have time.

Cheers
Iain

Sent from my iPhone 4

On 29/03/2012, at 12:55 AM, Chris Brodyreply@reply.github.com wrote:

You still missed the changes to the source files src/sqlite_plugin.coffee and src/lawnchair_sqlite_plugin_adapter.coffee


Reply to this email directly or view it on GitHub:
#23 (comment)

@brodybits
Copy link
Contributor

@coomsie sorry to bother you but I got a comment from someone that a couple more changes are necessary to follow the WebSQL API. I expect to submit the changes sometime later today.

@coomsie
Copy link
Author

coomsie commented Apr 1, 2012

b.t.w.

HTML iOS 5.1 web sql changes will effect phonegap/cordova and make this a bit more important ....

check this out

@brodybits
Copy link
Contributor

Awesome. You know that they already have a solution that does backups and restores upon app startup and shutdown but I do not like this in case an app or even a device is crashing in the middle of running an application. I am working on a solution right now to provide a consistent API between PhoneGap 1.2/1.3, Cordova 1.5, and in the future for the Android.

@brodybits
Copy link
Contributor

I have reorganized my fork to contain versions for both PhoneGap (1.3-) and Cordova (1.5+), with plans to add an Android version in the future. It is in the following location: https://github.com/chbrody/Cordova-SQLiteNative

I made changes to db.transaction() and also to the callbacks in tx.executeSql() in the Legacy-PhoneGap-iPhone subdirectory only so far and have asked someone else to take a look. If they are OK then I plan to patch them into the Cordova-iOS version as well.

@davibe
Copy link
Owner

davibe commented Apr 3, 2012

I am thinking, does it make sense to maintain support for older phonegaps ? I think we should onle support "the current stable".

+1 for Android version.

@brodybits
Copy link
Contributor

I would agree however some people may be stuck with an older PhoneGap until almost all of the plugins are ported to Cordova 1.5+. In addition, this is a very nice fix for people with an older PhoneGap who are having problems with the iOS 5.1 Web SQL API changes (check this out from @coomsie)

@davibe
Copy link
Owner

davibe commented Apr 3, 2012

I still think people using older phonegap should just git reset to an older revision of this plugin. BTW I am finally working to port some projects to cordova so I will look at this patch and merge it soon.

@brodybits
Copy link
Contributor

Well someone is actually testing and using my API changes in an older PhoneGap so I do want to keep it in my branch for the time being. I have pushed quite a few API changes and I leave it up to you whether or not you want to take these.

@brodybits
Copy link
Contributor

Also I had an idea to drop db.executeSql() in favor of using the transaction, since db.executeSql() is not according to the HTML5/W3 SQL API and I don't really want to add this to the Android version. Any comments?

@coomsie
Copy link
Author

coomsie commented Apr 4, 2012

i suggest going with the latest supported stable version, people can go back via git.. to get the older version if need be.

also lets try to keep it as close the to the spec as possible ...

project might in future change again, when the new web db is common place .... chrome seems to be pushing ahead in that regard.

@marcucio
Copy link

marcucio commented Apr 4, 2012

We have a version of this that works well on 1.4 (thanks chbrody) so why not put it out there. There probably are many people like myself who cannot upgrade their projects yet to 1.5 because many of the plugins are not on 1.5. Here is the 1.4 list of plugins:

https://github.com/phonegap/phonegap-plugins/tree/master/iPhone

compared to the list of 1.5 plugins:

https://github.com/phonegap/phonegap-plugins/tree/master/iOS

@brodybits
Copy link
Contributor

I have renamed my project (again) to https://github.com/chbrody/Cordova-SQLitePlugin/ and added a DroidGap version, use it at your own risk!

@brodybits
Copy link
Contributor

I have made some changes to test the Android (DroidGap) version with the Lawnchair test suite, and I have now made a common Lawnchair adapter that works with both the iOS and Android versions. The Android and Cordova-iOS versions use sqlitePlugin.openDatabase() to open a database to work closer to the Web SQL API. I want to test and incorporate some fies from @marcucio then add a pull request if this is working OK.

@davibe
Copy link
Owner

davibe commented Apr 10, 2012

I decided to update the plugin to make it work with Cordova/Phonegap 1.5.0 with minimal modifications. I think we should keep it simple and just support the current release of Phonegap. Users can always use git to roll back to an older version. I am going to add git tags to help users decide. I did not change the name from Phonegap to Cordova yet, i think i will.

I love the idea of having an html5-like api, but this should not break the old one. Is there any test suite we can use to implement the api properly?

It's nice to see someone working out the android side too. I suggest to create a separate git project for that.

@davibe davibe closed this Apr 10, 2012
@brodybits
Copy link
Contributor

In that case, I will keep my changes separated but still as a "fork" so I hope you will give @coomsie the credit for the 1.5.0 fixes. I have been doing the testing by using an adapted version of the sample from your website together with the basic testsuite from Lawnchair. I do want to dump the old PhoneGap junk from my project but one of my goals was to keep the same API between iOS and Android and I want to continue in this direction. @davibe thanks again for this work, it has helped a number of people who were affected by the iOS 5.1 changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants