Wizcorp / Ejecta-X

A Fast, Open Source JavaScript, Canvas & Audio Implementation
http://wizcorp.github.io/Ejecta-X
185 stars 51 forks source link

localStorage mapped on android sharedpreferences (initialization) #59

Closed come closed 10 years ago

come commented 10 years ago

Hi,

The goal of this PR is to add a very first implementation for localStorage binding into Ejecta-X

It uses the SharedPreferences storage of Android (the basic key value storage for mobiles) (See: http://developer.android.com/reference/android/content/SharedPreferences.html)

Code to test it :

  var score = 42;
  var bestScore = localStorage.getItem("my.bestScore") || 0;

  if (score > bestScore) {
    bestScore = score;
    localStorage.setItem("my.bestScore", score);
  }

Note : with this pr, I just want to start a discussion about accessing key/value datas in javascript (ejectax) Note2 : There may be a better way to achieve that. Don't hesitate if you have some improvements / refactoring to do... i will be happy to see what a real c++ developper would have done

ronkorving commented 10 years ago

From your code example, it implies that getItem can return non-string values (an integer in this case). That's not true, is it?

come commented 10 years ago

in seems localstorage always return a string no ? (as mentionned in w3c http://www.w3.org/TR/webstorage/#storage-0)

javascript auto cast it in this case so it will work

var score = 12;
var bestscore = "20";
console.log(bestscore > score);
//return true
ronkorving commented 10 years ago

Correct. I just wanted to make sure that you understood that. As a JS developer myself, I'm not really used to doing math comparisons on strings. Anyway... yes, the input must be cast to string (not serialized as JSON for example), and so the output must always be a string.

come commented 10 years ago

sorry my bad !

aogilvie commented 10 years ago

@come it's looking good so far! I hope my comments help a little and save you duplicating a lot of code for each API ;) Looking forward to the next iteration! :pizza:

come commented 10 years ago

just updated !

@aogilvie thx for the review, very helpfull for me indeed !

aogilvie commented 10 years ago

Great! I checked it. Careful with the indentation, you can see below your code needs to match indentation: https://github.com/come/Ejecta-X/blob/develop/sources/ejecta/EJUtils/EJBindingLocalStorage.cpp#L44

Will you implement the remaining APIs? (removeItem(), clear(), length() - length will be a bit tricky. Try using SharedPrefs; getAll to return Map<String, ?> and count the length of that. Be sure to clean memory appropriately, the overhead on this could be huge..)

ronkorving commented 10 years ago

Map has a size() function you should be able to use after getAll: http://developer.android.com/reference/java/util/Map.html#size%28%29

aogilvie commented 10 years ago

@come Ah yes, one last thing: You are using a slightly generic access to SharedPrefs, this means that if you implement a count as suggested above you may return a count for other data that Ejecta did not store. Use SharedPrefs with an especially unique name such as mContext.getPackageName() + "ejectaLocalStorage" See: https://github.com/come/Ejecta-X/blob/develop/project/android/src/com/impactjs/ejecta/EjectaRenderer.java#L83. Points for using the right API though :)

come commented 10 years ago

@aogilvie sorry for indentation (sublime was configured for another framework codestyle) :$

@aogilvie & @ronkorving i will implement clear/remove/length later :)

aogilvie commented 10 years ago

@come how's it going, got busy? :) you'll need to pull develop again, this merge is out of sync now.

p.s. Your fix got a mention in the changelog :)

come commented 10 years ago

@aogilvie yes got busy! (on www.wanaplan.com... will probably need to be ported to ejecta/ejectaX one day :) ?)

i made the sync, but had strange issues :$ that i resolved with unsatisfying manner. i will push it if you have advice on them :)

aogilvie commented 10 years ago

@come WOW that is seriously cool!! Looking forward to the updated PR!

ronkorving commented 10 years ago

Wow.... that is truly kick-ass. Awesome job! I would love to see this on my tablet ;)