Hi, A few review comments for this commit are inline.
CellML Automated Notifications wrote: > Author: agarny > Date: 2007-09-19 05:47:09 +1200 (Wed, 19 Sep 2007) > New Revision: 1755 > > Modified: > pce/trunk/chrome/content/controls/graph.xml > pce/trunk/chrome/content/controls/model.xml > pce/trunk/chrome/content/ui/MainWindow.js > pce/trunk/chrome/content/util/GraphSupport.js > pce/trunk/chrome/content/util/Metadata.js > pce/trunk/chrome/content/util/ModelURLLoader.js > pce/trunk/chrome/content/util/VariablePath.js > Log: > Made some 'alert' messages more meaningful. > > > Modified: pce/trunk/chrome/content/controls/graph.xml > =================================================================== > --- pce/trunk/chrome/content/controls/graph.xml 2007-09-18 05:56:23 UTC > (rev 1754) > +++ pce/trunk/chrome/content/controls/graph.xml 2007-09-18 17:47:09 UTC > (rev 1755) > @@ -267,7 +267,7 @@ > const > XUL="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";; > var tcs = e.element.getElementsByTagNameNS(XUL, "treecell"); > tcs.item(1).setAttribute("label", m.name); > - > + > I agree that lines with only spaces are bad - I will make a note of this when we come to writing up our style guidelines. > try > { > var typeT = ds.GetTarget(first, typeR, true); > @@ -1531,7 +1531,7 @@ > try > { > this.gcontext.moveTo(xe, y); > - } catch (e) { alert("Got problem with xe=" + xe + " and y=" + y); } > + } catch (e) { alert("There is a problem with xe=" + xe + " and y=" > + y + "."); } > this.gcontext.lineTo(x, y); > this.gcontext.stroke(); > } > > Modified: pce/trunk/chrome/content/controls/model.xml > =================================================================== > --- pce/trunk/chrome/content/controls/model.xml 2007-09-18 05:56:23 UTC > (rev 1754) > +++ pce/trunk/chrome/content/controls/model.xml 2007-09-18 17:47:09 UTC > (rev 1755) > @@ -305,12 +305,11 @@ > > // Now go through the code information, and find initial values to set... > var ciit = ci.iterateTargets(); > - var v; > while ((ct = ciit.nextComputationTarget()) != null) > { > if (ct.degree == 0 && ct.type == 2 /* STATE_VARIABLE */) > { > - var cv = FindElementByPath(MakeVariablePath(ct.variable), m); > + var cv = FindElementByPath(MakeVariablePath(ct.variable), > ct.variable, m); > See my comments about the changes to FindElementByPath on VariablePath.js > cv.initialValue = dc.getRawData(ct.assignedIndex, guess); > } > } > > Modified: pce/trunk/chrome/content/ui/MainWindow.js > =================================================================== > --- pce/trunk/chrome/content/ui/MainWindow.js 2007-09-18 05:56:23 UTC (rev > 1754) > +++ pce/trunk/chrome/content/ui/MainWindow.js 2007-09-18 17:47:09 UTC (rev > 1755) > @@ -65,7 +65,7 @@ > window.ccgs = > cc["@cellml.org/ccgs-bootstrap;1"]. > > getService(ci.cellml_servicesICodeGeneratorBootstrap).createCodeGenerator(); > - > + > // Get the CellML Integration Service... > window.cis = > cc["@cellml.org/cis-bootstrap;1"]. > @@ -81,7 +81,7 @@ > } > catch (e) > { > - alert("Caught exception: " + e); > + alert("A problem occurred while registering the different services > required by PCEnv: " + e + "."); > } > } > > @@ -142,8 +142,8 @@ > } > catch (e) > { > - alert("Invalid URL on command line: " + ns.getArgument(i) + " (" + > - NormaliseInputURL(ns.getArgument(i)) + "): " + e); > + alert("An invalid URL was provided to PCEnv: " + ns.getArgument(i) + " > (" + > + NormaliseInputURL(ns.getArgument(i)) + "): " + e + "."); > I have two concerns with the new message: 1) We don't say where the invalid URL came from. This error would presumably usually arise when deliberately invoking the program from with arguments on the command line, and so the user would be aware that they had done this, and it would be good to draw their attention to the fact that the URL they passed is invalid. 2) I am not sure about whether we should just write 'PCEnv' in alerts like this - we can obviously search for all instances and replace them if we ever change the name, although it will make life harder, e.g., for someone creating a differently branded fork for inclusion in a distribution or for some other purpose. Having a 'branding string' might be one way around it, or we could just avoid naming the product in such messages since it is probably not necessary here. > } > } > > > Modified: pce/trunk/chrome/content/util/GraphSupport.js > =================================================================== > --- pce/trunk/chrome/content/util/GraphSupport.js 2007-09-18 05:56:23 UTC > (rev 1754) > +++ pce/trunk/chrome/content/util/GraphSupport.js 2007-09-18 17:47:09 UTC > (rev 1755) > @@ -9,6 +9,7 @@ > return function() > { > var idcr = Components.interfaces.IDataToCanvasRequest; > + > if (obj.drawType == 'L') > return idcr.GLYPH_LINES; > else if (obj.drawType == 'SQ') > @@ -19,8 +20,10 @@ > return idcr.GLYPH_DIAMONDS; > else if (obj.drawType == 'D') > return idcr.GLYPH_DOTS; > + > // Shouldn't be reached... > - alert("Unknown glyph string; this is a bug."); > + alert("Unknown glyph string: '" + obj.drawType + "'."); > Can we keep the 'this is a bug' because this is an internal error which should never happen, and if it does, we would like to know about it (having drawType as well is a good idea, of course)? > + > return -1; > } > } > @@ -84,8 +87,8 @@ > var model = SearchModelsForURI(uri, window.context.loadedModels); > > if (model == null) > - alert("Failed to find a model with URI " + uri + " as referenced in > RDF."); > - > + alert("The model with URI '" + uri + "' couldn't be found."); > + > I think it is probably an idea to make it clearer where the URI came from and so keep some reference to RDF / graphing metadata or something like that. As it is now, it sounds like there was a 404 HTTP error, which is not the case, instead this means that a session refers to a variable in a model, but it doesn't also load the corresponding model into memory. > var cci = model.allComponents.iterateComponents(); > var c; > > @@ -98,7 +101,8 @@ > return v; > } > > - alert("Warning: No match for variable cmeta:id referenced from RDF."); > + alert("The variable with a cmeta:id of '" + ident + "' couldn't be > found."); > Again, it would be good to make it clear that the cmeta:id we are looking for is from the graphing metadata ("from RDF" is not very clear either, of course) so they know what the actual problem is. > + > return null; > } > > @@ -750,7 +754,7 @@ > return; > } > } > - alert("Warning: No match for session axis found."); > + alert("The session axis '" + resource + "' couldn't be found."); > Note that resource is not a string, it is a resource node. Javascript will convert it to a string, but probably not the most useful one. The resource URL might be more useful (i.e. + resource.ValueUTF8 +). > }, > > lockAxis: function(axid, lockedBy, oldAxid) { > @@ -881,7 +885,7 @@ > var axis = GraphAxisFromRDF(ds, n); > this.mLastAxid = axis.axid = this.mLastAxid + 1; > this.mAxes[axis.axid] = axis; > - > + > try > { > tl = ds.GetTarget(tl, restR, true); > > Modified: pce/trunk/chrome/content/util/Metadata.js > =================================================================== > --- pce/trunk/chrome/content/util/Metadata.js 2007-09-18 05:56:23 UTC (rev > 1754) > +++ pce/trunk/chrome/content/util/Metadata.js 2007-09-18 17:47:09 UTC (rev > 1755) > @@ -5,7 +5,7 @@ > var sd = > model.getRDFRepresentation("http://www.cellml.org/RDFXML/string";). > QueryInterface(Components.interfaces. > cellml_apiIRDFXMLStringRepresentation).serialisedData; > - > + > // Put it into the Mozilla RDF implementation... > var p = Components.classes["@mozilla.org/rdf/xml-parser;1"]. > createInstance(Components.interfaces.nsIRDFXMLParser); > > Modified: pce/trunk/chrome/content/util/ModelURLLoader.js > =================================================================== > --- pce/trunk/chrome/content/util/ModelURLLoader.js 2007-09-18 05:56:23 UTC > (rev 1754) > +++ pce/trunk/chrome/content/util/ModelURLLoader.js 2007-09-18 17:47:09 UTC > (rev 1755) > @@ -310,7 +310,7 @@ > else > { > alert("Sorry, the loaded document does not contain a CellML model > (namespace " + > - ns + ")"); > + ns + ")."); > } > LoadCompleted(); > return; > > Modified: pce/trunk/chrome/content/util/VariablePath.js > =================================================================== > --- pce/trunk/chrome/content/util/VariablePath.js 2007-09-18 05:56:23 UTC > (rev 1754) > +++ pce/trunk/chrome/content/util/VariablePath.js 2007-09-18 17:47:09 UTC > (rev 1755) > @@ -34,7 +34,7 @@ > return BuildModelPath(imp.modelElement); > } > > -function FindElementByPath(path, m) > +function FindElementByPath(path, v, m) > FindElementByPath is supposed to be a generic API which we might want to call from other places too - it is not just for finding variables let alone computation targets. This is really an internal 'there is a bug' error if the path is invalid (it shouldn't happen if the rest of the code is working), and so I don't think we should change a clean, generic interface to something more specific just to make such a message display a different diagnostic. Also, given this is an error which would only be interpreted by someone programming PCEnv, it would probably be more useful just to put path in the error message anyway. > { > var operators = > { > @@ -64,7 +64,10 @@ > operand2 = operators[operator](operand1, operand2); > } > if (path.length != 0) > - alert("Warning: junk at end of an element path."); > + { > + var c = > v.parentElement.QueryInterface(Components.interfaces.cellml_apiICellMLComponent); > + alert("The element path for variable '" + v.name + "' of component '" + > c.name + "' is malformed."); > + } > > return operand2; > } > > _______________________________________________ > automated-notifications mailing list > [EMAIL PROTECTED] > http://www.cellml.org/mailman/listinfo/automated-notifications > _______________________________________________ cellml-discussion mailing list cellml-discussion@cellml.org http://www.cellml.org/mailman/listinfo/cellml-discussion
