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 p5.Vector in localStorags(#3990) #4034

Merged
merged 4 commits into from
Sep 27, 2019
Merged

Added p5.Vector in localStorags(#3990) #4034

merged 4 commits into from
Sep 27, 2019

Conversation

singhvisha
Copy link
Contributor

@singhvisha singhvisha commented Sep 23, 2019

Added a new feature in which vector can be stored/retrieved in/from Local Storage(#3990 ).

closes #3990

@singhvisha singhvisha changed the title Adding p5.Vector in localStorags(#3990) Added p5.Vector in localStorags(#3990) Sep 23, 2019
@singhvisha
Copy link
Contributor Author

Hi @stalgiag, Could you please review it?

Copy link
Contributor

@stalgiag stalgiag left a comment

Choose a reason for hiding this comment

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

Good start!

One typo that I pointed out inline. Also since you are editing functions now, we should add unit tests. The new unit test goes here. You can follow the pattern for testing color. You can find more info on writing unit tests here.

@@ -71,6 +71,8 @@ p5.prototype.storeItem = (key, value) => {
case 'object':
if (value instanceof p5.Color) {
type = 'p5.Color';
} else if (value instanceof p5.Vector) {
type = 'p5.Color';
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here I believe

Suggested change
type = 'p5.Color';
type = 'p5.Vector';

@singhvisha
Copy link
Contributor Author

hi @stalgiag when I am adding the unit test and running npm run grunt It showing an error.

10 pending
1 failing
1338 passing (23s)

  1. local storage
    "before each" hook for "boolean storage retrieval should work":
    TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'p5'
    | property '_curElement' -> object with constructor '_main.default.Renderer2D'
    --- property '_pixelsState' closes the circle
    at JSON.stringify ()
    at p5._main.default.storeItem (http://localhost:9001/lib/p5-test.js:86298:30)
    at p5.p.setup (unit/data/local_storage.js:23:14)
    at p5._setup (http://localhost:9001/lib/p5-test.js:66825:27)
    at p5._start (http://localhost:9001/lib/p5-test.js:66688:25)
    at new p5 (http://localhost:9001/lib/p5-test.js:67253:22)
    at Context. (unit/data/local_storage.js:13:5)

Warning: [object Object] Use --force to continue.

Aborted due to warnings.
npm ERR! code ELIFECYCLE
npm ERR! errno 3
npm ERR! [email protected] grunt: grunt
npm ERR! Exit status 3
npm ERR!
npm ERR! Failed at the [email protected] grunt script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR! /home/vishal/.npm/_logs/2019-09-24T17_38_14_940Z-debug.log

@singhvisha
Copy link
Contributor Author

singhvisha commented Sep 24, 2019

And the Edited file is.

suite('local storage', function() {
    var myp5;
    var myBoolean = false;
    var myObject = { one: 1, two: { nested: true } };
    var myNumber = 46;
    var myString = 'coolio';
    var myColor;
    var myVector;
  
    var hardCodedTypeID = 'p5TypeID';
  
    setup(function(done) {
      new p5(function(p) {
        p.setup = function() {
          myp5 = p;
          myColor = myp5.color(40, 100, 70);
          myVector = myp5.createVector(10, 20, 30);
          myp5.storeItem('myBoolean', myBoolean);
          myp5.storeItem('myObject', myObject);
          myp5.storeItem('myNumber', myNumber);
          myp5.storeItem('myString', myString);
          myp5.storeItem('myColor', myColor);
          myp5.storeItem('myVector', myVector);
          done();
        };
      });
    });
  
    teardown(function() {
      myp5.remove();
    });
  
    suite('all keys and type keys should exist in local storage', function() {
      test('boolean storage retrieval should work', function() {
        assert.isTrue(myp5.getItem('myBoolean') === false);
      });
      test('boolean storage should store the correct type ID', function() {
        assert.isTrue(
          localStorage.getItem('myBoolean' + hardCodedTypeID) === 'boolean'
        );
      });
      test('object storage should work', function() {
        assert.deepEqual(myp5.getItem('myObject'), {
          one: 1,
          two: { nested: true }
        });
      });
      test('object storage retrieval should store the correct type ID', function() {
        assert.isTrue(
          localStorage.getItem('myObject' + hardCodedTypeID) === 'object'
        );
      });
      test('number storage retrieval should work', function() {
        assert.isTrue(myp5.getItem('myNumber') === 46);
      });
      test('number storage should store the correct type ID', function() {
        assert.isTrue(
          localStorage.getItem('myNumber' + hardCodedTypeID) === 'number'
        );
      });
      test('string storage retrieval should work', function() {
        assert.isTrue(myp5.getItem('myString') === 'coolio');
      });
      test('string storage should store the correct type ID', function() {
        assert.isTrue(
          localStorage.getItem('myString' + hardCodedTypeID) === 'string'
        );
      });
      test('p5 Color should retrieve as p5 Color', function() {
        assert.isTrue(myp5.getItem('myColor') instanceof p5.Color);
      });
      test('p5 Vector should retrieve as p5 Vector', function() {
        assert.isTrue(myp5.getItem('myVector') instanceof p5.Vector);
      });
    });
  
    var checkRemoval = function(key) {
      myp5.removeItem(key);
      assert.deepEqual(myp5.getItem(key), null);
      assert.deepEqual(myp5.getItem(key + hardCodedTypeID), null);
    };
  
    suite('should be able to remove all items', function() {
      test('boolean should be removable', function() {
        checkRemoval('myBoolean');
      });
  
      test('number should be removable', function() {
        checkRemoval('myNumber');
      });
  
      test('object should be removable', function() {
        checkRemoval('myObject');
      });
  
      test('string should be removable', function() {
        checkRemoval('myString');
      });
  
      test('color should be removable', function() {
        checkRemoval('myColor');
      });
  
      test('color should be removable', function() {
        checkRemoval('myVector');
      });
    });
  });

@stalgiag
Copy link
Contributor

Hi @singhvisha! So it is important to be able to do some troubleshooting and testing on your own. If you take the main error message 'TypeError: Converting circular structure to JSON' and search you will find that an object with a circular structure cannot be given as an argument to JSON.stringify. 'Circular structure' is when an object has a reference to itself in one of its properties. In the case of a p5.Vector this is due to to the p5 property. Since we don't need to store that value, you can make a clone of the object that only has the x, y, and z properties and store that whenever you have a p5.Vector.

It is important to always test to make sure your changes work in a manual example before moving onto unit testing. You can use manual-test-examples/empty-example/sketch.js for this purpose. Just make sure to not commit your changes to this file.

Once you have your manual tests working with all of your added features, your unit tests should work as well.

Good luck!

@singhvisha singhvisha requested a review from stalgiag September 26, 2019 06:37
@singhvisha
Copy link
Contributor Author

Hi @stalgiag , Could you please review it?

@@ -71,6 +71,10 @@ p5.prototype.storeItem = (key, value) => {
case 'object':
if (value instanceof p5.Color) {
type = 'p5.Color';
} else if (value instanceof p5.Vector) {
type = 'p5.Vector';
let coord = [value.x, value.y, value.z];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let coord = [value.x, value.y, value.z];
const coord = [value.x, value.y, value.z];

small request, to try to use const whenever something doesn't need to be reassigned. Otherwise looks great!

@singhvisha singhvisha requested a review from stalgiag September 27, 2019 14:54
@singhvisha
Copy link
Contributor Author

Hi @stalgiag , Could you please review it?

@stalgiag stalgiag merged commit dc34474 into processing:master Sep 27, 2019
@singhvisha
Copy link
Contributor Author

Hi @stalgiag,could please assign me a new issue (#3973)?

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.

p5.Vector in LocalStorage
2 participants