Talk:Io.js: Difference between revisions

From MozillaZine Knowledge Base
Jump to navigationJump to search
(reponse to asqueella)
Line 117: Line 117:
:[[User:Asqueella|asqueella]]
:[[User:Asqueella|asqueella]]


::There are a few reasons why both forward and backslashes should be supported: (1) Some extension developers work on operating systems where they are accustomed to using forward slashes instead of backslashes for paths; we should be sensitive to that; (2) I think one of the main reasons the <code>nsIFile</code> interface prevents you from entering full paths (i.e., <code>nsiFile.append()</code> to drill down directories) is because [http://www.xulplanet.com/references/xpcomref/ifaces/nsIFile.html#method_append "it is the only correct cross-platform way to specify a file. Strings are not such a way. If you grew up on windows or unix, you may think they are. Welcome to reality."] With that in mind, we might try to accomodate *nix and Mac developers by permitting forward slashes.
::You are right, <code>p in paths</code> would also append <code>paths[0]</code> to the file path. As I wrote above, <b>this is untested code</b> -- I wrote the function on-the-fly in the MediaWiki editor.
::Returning <code>f</code> instead of <code>this.read(f, charset)</code> is fine with me, but I would like either (1) an option for this function to return the file contents or (2) another function called something like <code>getFileContents()</code> which returns <code>this.read(f, charset)</code>. Come to think of it, if we have both of these functions, why do we need the original <code>read()</code> function at all? Similarly, we can get rid of the original <code>write()</code> function in place of the new one (which could take a full path/filename as a function argument, <b>OR</b> an <code>nsILocalFile</code> -- we can use <code>typeof</code> or some other means to determine the argument's type).
::[[User:grimholtz|grimholtz]]


Now here are the changes I would make to existing functions:
Now here are the changes I would make to existing functions:
Line 174: Line 181:
:From my reading of the code though, calling close is not necessarily at all (the streams are closed in destructor). See nsFileStreams.cpp.
:From my reading of the code though, calling close is not necessarily at all (the streams are closed in destructor). See nsFileStreams.cpp.
:[[User:Asqueella|asqueella]]
:[[User:Asqueella|asqueella]]
::The changes are not merely stylistic. They result in more efficient algorithms because there are less assignments, comparisons, and object allocations than the original code (although since I don't know anything about Mozilla's memory management, I don't know if the change in object allocations really matter; however, the change in assignments and comparisons certainly makes for a more efficient algorithm).
::[[User:grimholtz|grimholtz]]




Line 211: Line 221:
:I'm not sure if it is a suitable addition to File I/O module. I think this functionality needs to be in the Extension manager service itself.
:I'm not sure if it is a suitable addition to File I/O module. I think this functionality needs to be in the Extension manager service itself.
:[[User:Asqueella|asqueella]]
:[[User:Asqueella|asqueella]]
::Of course I agree with you, but it's not there and who knows when it will be? Maybe with QuickNote you don't want to delete the notes if someone uninstalls the extension, but in many other cases, any file which is created using <code>write()</code> should be deleted with the extension. We can leave it out if you like, and I'll just make it a KB article instead.
::[[User:Grimholtz|grimholtz]]

Revision as of 08:44, 14 March 2005

asqueella's suggestions

I'd like to get point-by-point agree/disagree answer below from interested parties (grimholtz?), add long answers as subsections.

  • rename top-level ids -> FileIO2, DirIO2 to avoid conflicts with older code.
  • change Components.classes[this.dirservCID].createInstance(this.propsIID) -> getService (it is a service).
  • uncomment this ((?) I don't really see why that's commented): // data += uniConv.Finish();
  • remove DirIO.join/DirIO.split. File IO article has example for converting native path to URI and vice versa. I don't know how join/split can be used also, and this is not very clean XP code (hard-coding '\' for windows only doesn't seem smart).
  • rewrite FileIO.path()
  • move return false statements out of catch blocks
  • add dump() statements instead of // foobar!
  • personal: I would like the *CID/*IID members to be removed. I like the idea of creating wrappers for getService/createInstance, see below. This makes code much more readable IMO.
{
  createInstance: function(aCID, aIID) {
    return Components.classes["@mozilla.org/"+aCID]
          .createInstance(Components.interfaces[aIID]);
    },

  getService: function(aCID, aIID) {
    return Components.classes["@mozilla.org/"+aCID]
          .getService(Components.interfaces[aIID]);
  }
}
  • replace tab symbols with two-spaces. It is too wide for my 1024x768 screen now.
  • move examples from code to a separate section, add a link to io.js to the code.

me thinks about creating his own thin wrapper (like this for prefs) for file io...


@Nick - Why don't you add prefutils to as an article to KB like is.js? Also, do you want to use it for QuickNote? Might make things nicer and more module. --Jed 00:25, 13 Mar 2005 (PST)

Will think about it and open a bug or mail you about prefs in QN.
I wanted to write about that wrapper and about other wrappers too, but I haven't had time for that yet. asqueella

They're all great ideas, in my opinion, except "move return false statements out of catch blocks." I want to know when the operation has failed (but not through an exception). Also, don't forget to increment the version counter from 0.1 to 0.2, as well as placing your name in the header comments. In fact, it might be a good idea to have a version history section in the header. Up to you. I have a few other suggestions that I'll type here tonight (code fixes and enhancements).
--grimholtz 13:54, 10 Mar 2005 (EST).
I don't think the changelog should be in the reusable js file itself. We can create a changelog section/subpage though. asqueella


OK, but please include authors and contributors in the js file. --grimholtz 15:13 10 Mar 2005 (EST).

regarding returning false, I meant, change

try {
  return "useful value"
}
catch(e) {
  return false;
}

to

try {
  return "useful value"
} catch(e) {}
return false;
Sounds good! --grimholtz 15:14 10 Mar 2005 (EST).

Also I think returning false on failure isn't smart, null would be better. asqueella


hm, why? I like minimizing the number of types that can be returned because it simplifies code which uses the class --grimholtz 15:15 10 Mar 2005 (EST).
I mean return null in functions that return a string in case of success.
returning false for a function that returns a boolean value is of course correct.
asqueella
Sounds good --grimholtz 16:52 10 Mar 2005 (EST).

grimholtz's suggestions

These are the functions I would change and add to FileIO.


First, I would add read() and write() function overloads which take a path/filename as a string. That way you don't have to continually write code this like:

var fileContents = DirIO.get("ProfD");
fileContents.append('extensions');
fileContents.append('{5872365E-67D1-4AFD-9480-FD293BEBD20D}');
fileContents.append('defaults');
fileContents.append(some-file.xml);
fileContents = FileIO.read(fileContents);

you could just write:

var fileContents = FileIO.read("ProfD/extensions/{5872365E-67D1-4AFD-9480-FD293BEBD20D}/defaults/some-file.xml");

Here's an example (this is untested):

  /**
   * readFiles takes a full path/filename as string and returns its contents.
   * First part of the path must be one of the special path names; e.g., ProfD.
   * You can separate paths with forward slash or backslash; it doesn't matter.
  readFile : function(fileAsString, charset) {

    fileAsString = fileAsString.replace("\\", "/", "g");
    var paths = fileAsString.split("/");
    // first part must be one of the "special" path names, like ProfD or TmpD
    // else DirIO.get() will fail.
    var f = DirIO.get(paths[0]);
    for (p in paths) {
      f.append(paths[p]);
    }
    return this.read(f, charset);
  },
Sounds great, but why do you want to include the support for forward slashes? Is it really useful to anybody? Also, wouldn't p in paths also append paths[0] to the file path?
Also any reason this have to be readFile, not getFile (ie. change that return with return f;)
asqueella
There are a few reasons why both forward and backslashes should be supported: (1) Some extension developers work on operating systems where they are accustomed to using forward slashes instead of backslashes for paths; we should be sensitive to that; (2) I think one of the main reasons the nsIFile interface prevents you from entering full paths (i.e., nsiFile.append() to drill down directories) is because "it is the only correct cross-platform way to specify a file. Strings are not such a way. If you grew up on windows or unix, you may think they are. Welcome to reality." With that in mind, we might try to accomodate *nix and Mac developers by permitting forward slashes.
You are right, p in paths would also append paths[0] to the file path. As I wrote above, this is untested code -- I wrote the function on-the-fly in the MediaWiki editor.
Returning f instead of this.read(f, charset) is fine with me, but I would like either (1) an option for this function to return the file contents or (2) another function called something like getFileContents() which returns this.read(f, charset). Come to think of it, if we have both of these functions, why do we need the original read() function at all? Similarly, we can get rid of the original write() function in place of the new one (which could take a full path/filename as a function argument, OR an nsILocalFile -- we can use typeof or some other means to determine the argument's type).
grimholtz

Now here are the changes I would make to existing functions:


  read   : function(file, charset) {
    var fiStream = null;
    var siStream = null;
    try {
      fiStream = Components.classes[this.finstreamCID]
                .createInstance(this.finstreamIID);
      siStream = Components.classes[this.sinstreamCID]
		.createInstance(this.sinstreamIID);
      fiStream.init(file, 1, 0, false);
      siStream.init(fiStream);
      var data = new String(siStream.read(-1));
      return charset ? this.toUnicode(charset, data) : data
    } 
    catch(e) {
      return false;
    }
    finally {
      if (siStream != null)
        siStream.close();
      if (fiStream != null)
        fiStream.close();
    }
},

write  : function(file, data, mode, charset) {
  var foStream = null;
  try {
    foStream = Components.classes[this.foutstreamCID]
	      .createInstance(this.foutstreamIID);
    if (charset) {
      data = this.fromUnicode(charset, data);
    }
    var flags = mode == 'a' ? 0x02 | 0x10 : // wronly | append
                              0x02 | 0x08 | 0x20; // wronly | create | truncate
				
    foStream.init(file, flags, 0664, 0);
    foStream.write(data, data.length);
    return true;
  }
  catch(e) {
    return false;
  }
  finally {
    if (foStream != null)
      foStream.close();
  }
},
In case anybody else is following this, the changes are stylistic plus added finally statement.
From my reading of the code though, calling close is not necessarily at all (the streams are closed in destructor). See nsFileStreams.cpp.
asqueella
The changes are not merely stylistic. They result in more efficient algorithms because there are less assignments, comparisons, and object allocations than the original code (although since I don't know anything about Mozilla's memory management, I don't know if the change in object allocations really matter; however, the change in assignments and comparisons certainly makes for a more efficient algorithm).
grimholtz


And here's another function I would add:

/**
 * Adds the given file to an extension's uninstall list
 * so the file doesn't linger after extension uninstallation.
 * The regular expression isn't correct (yet) so the function
 * doesn't prevent duplicate entries in the Uninstall log.
 * 
 */
addToUninstallList : function(file, extensionGUID) {

  var uninstallFile = DirIO.get("ProfD"); // %Profile% dir
  uninstallFile.append("extensions");
  uninstallFile.append(extensionGUID);
  uninstallFile.append("uninstall");
  uninstallFile.append("Uninstall");
  var data = FileIO.read(uninstallFile);
  // file.leafName actually isn't a sufficient check; we
  // should check the full path after 
  if (!data || new RegExp("^add\\s.*" + file.path, "m").test(data)) {
    // Uninstall file doesn't exist or the given file is
    // already listed in it.
    return;
  }
  // Add new file to the Uninstall list.
  // Note we're not using the FileIO.path() function
  // because it returns a URL-encoded path; e.g., ' ' = %20
  var newEntry = "add\t" + file.path + "\n";
  this.write(uninstallFile, newEntry, "a");
},
I'm not sure if it is a suitable addition to File I/O module. I think this functionality needs to be in the Extension manager service itself.
asqueella
Of course I agree with you, but it's not there and who knows when it will be? Maybe with QuickNote you don't want to delete the notes if someone uninstalls the extension, but in many other cases, any file which is created using write() should be deleted with the extension. We can leave it out if you like, and I'll just make it a KB article instead.
grimholtz