ebidel / idb.filesystem.js

HTML5 Filesystem API polyfill using IndexedDB
https://www.npmjs.com/package/idb.filesystem.js
Other
487 stars 46 forks source link

Added support for seek() and truncate() #6

Closed piranna closed 11 years ago

piranna commented 11 years ago

Added support for seek() and truncate() methods on FileWriter and also improved write() support according to the File API specification based on code from FileWriter polyfill (https://github.com/piranna/ShareIt/blob/master/js/webp2p/polyfills/FileWriter.js) at ShareIt! project (also developed by myself). Since I started the polyfill using your implementation as basis, I think it's fair to return my improved and fixed version... :-)

ebidel commented 11 years ago

Haven't looked closely at the code yet, but can you use Google's JS style guide for spacing, brackets, etc. http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml.

piranna commented 11 years ago

Fixed :-) I'm too much used to the BSD and Pep-8 styles... :-P

shacharz commented 11 years ago

I'd like to see this pull request accepted, is it inefficient or anything?

ebidel commented 11 years ago

Thanks for this. Sorry about taking to long to review. LGTM

ebidel commented 11 years ago

This isn't passing the tests. Can you fire them up in FF (start a web serve in the root dir and hit http://localhost/tests/)?

piranna commented 11 years ago

Don't worry, to be honest I forgot about it X-D I'll check it now.

piranna commented 11 years ago

I've started it and got the next error:

Uncaught ReferenceError: module is not defined tests.js:6
(anonymous function) tests.js:6

The line is:

module('window methods', {

Seems qUnit is not available, I've added it by hand and now it's working. I'm going to look on the bug.

piranna commented 11 years ago

I can't be able to fix it, the bug is happening when you create an empty FileEntry, so it don't have a blob object. I've tried to fix it using a mock file:

var blob_ = fileEntry.file_ ? fileEntry.file_.blob_ : {size: 0};

but now test doesn't work, it only show the first one as running. Any idea?

ebidel commented 11 years ago

That's one change I've made ad well. Currently having other issues with this business:

// Calc the head and tail fragments
var head = blob_.slice(0, position_);
var tail = blob_.slice(position_ + data.length);

I'm looking into it, don't sweat it.

piranna commented 11 years ago

Yeah, I did it to be able to do a 'data' write in the middle of a file at current 'position_' just like the specification (and POSIX, by the way) say.

Ok, I'll keep me code here if you need that I review something :-)

ebidel commented 11 years ago

Got the tests working. You had a data.length instead of data.size that tripped me up for far too long :(

Here's what write() looks like now:

this.write = function(data) { if (!data) { throw Error('Expected blob argument to write.'); }

// Call onwritestart if it was defined.
if (this.onwritestart) {
  this.onwritestart();
}

// TODO: not handling onprogress, onwrite, onabort. Throw an error if
// they're defined.

if (!blob_) {
  blob_ = new Blob([data], {type: data.type});
} else {
  // Calc the head and tail fragments
  var head = blob_.slice(0, position_);
  var tail = blob_.slice(position_ + data.size);

  // Calc the padding
  var padding = position_ - head.size;
  if (padding < 0) {
    padding = 0;
  }
  // Do the "write". In fact, a full overwrite of the Blob.
  // TODO: figure out if data.type should overwrite the exist blob's type.
  blob_ = new Blob([head, new Uint8Array(padding), data, tail],
                   {type: blob_.type});
}

// Set the blob we're writing on this file entry so we can recall it later.
fileEntry.file_.blob_ = blob_;

idb_.put(fileEntry, function(entry) {
  // Add size of data written to writer.position.
  position_ += data.size;

  if (this.onwriteend) {
    this.onwriteend();
  }
}.bind(this), this.onerror);

};

I'm going to add tests for seek() and truncate().

piranna commented 11 years ago

Don't know why I was using .length instead of .size, maybe they are equivalent for other objects? Code looks good anyway, thank you for your time looking it :-)

ebidel commented 11 years ago

Blob doesn't have a .length but ArrayBuffer's do and the FileWriter interface does. It's easy to get things mixed up!!

piranna commented 11 years ago

Maybe that's the reason, in my original code (ShareIt!) I was using ArrayBuffers...

ebidel commented 11 years ago

Everything should be right as mud now. Give v0.0.3 a test and see if it breaks anything. Thanks again!

https://github.com/ebidel/idb.filesystem.js/commit/e92d5959ba109ee7cd076f7f20ad63b229f67f3f

piranna commented 11 years ago

Tests are being passed on the latest version, good job! :-)