ODF Toolkit
  1. ODF Toolkit
  2. ODFTOOLKIT-149

OdfPackag.save(..) should save as well all its open documents

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: odfdom-0.8.6
    • Fix Version/s: odfdom-0.9
    • Component/s: java
    • Labels:
      None
    • Environment:
      Operating System: All
      Platform: All

      Description

      After the design changes due to embedded documents (issue 166) the Package will be aware of all its open documents and may now save them all (securing all potential changes).

      Any document will therefore dispatch the save call in the future to its OdfPackage.

      In addition the method OdfPackage.save() without parameter will be added.

      This new method will distinct the save of changes into the original document/package from a save into a new document/package.

      The reason herefore is that JDK ZIP implementation is read-only and the save into the original document always requires the copy of all resources (taking time).

      The new method gives the user the change to trigger this extra work of copying only when required.

      The existing OdfPackage.save(InputStream in) could not distinct any earlier, if the stream will be written into the original or a new file and copying was always being done.

      NOTE: Still a fallback for save(..) into original Package will be provided using exception handling.

        Issue Links

          Activity

          Svante Schubert created issue -
          Hide
          Svante Schubert added a comment -

          In addition to saving all open documents, which are now known by the OdfPackage as the OdfPackageDocument base class is from the package layer as well, all open OdfFileDom should be saved as well.

          To archive this OdfFileDom should be moved to the same Java package as OdfPackage and every OdfFileDom should register itself during creation (via constructor) at the OdfPackage (similar as the OdfPackageDocument) is doing it.

          Show
          Svante Schubert added a comment - In addition to saving all open documents, which are now known by the OdfPackage as the OdfPackageDocument base class is from the package layer as well, all open OdfFileDom should be saved as well. To archive this OdfFileDom should be moved to the same Java package as OdfPackage and every OdfFileDom should register itself during creation (via constructor) at the OdfPackage (similar as the OdfPackageDocument) is doing it.
          Hide
          Svante Schubert added a comment -

          During striving to this goal, I realized the misdesign of XPath and embedded document feature, which only was designed for the XML layer and not made available for the underlying Package layer.

          The basic idea was to move XPath and Embedded feature to package level.
          All XML handling is done in the package layer and embedded methods (now called subdocuments, as embedded require a real reference in the content.xml) will be dispatched from Document to Package.
          The difference, the path on Document are relative to the document, while the call on the package is absolute to the package layer.
          Some further improvements are done as the usage of Pattern and resolving changing directories (ie. /../ ) in the normalize function OdfPackage function.

          Show
          Svante Schubert added a comment - During striving to this goal, I realized the misdesign of XPath and embedded document feature, which only was designed for the XML layer and not made available for the underlying Package layer. The basic idea was to move XPath and Embedded feature to package level. All XML handling is done in the package layer and embedded methods (now called subdocuments, as embedded require a real reference in the content.xml) will be dispatched from Document to Package. The difference, the path on Document are relative to the document, while the call on the package is absolute to the package layer. Some further improvements are done as the usage of Pattern and resolving changing directories (ie. /../ ) in the normalize function OdfPackage function.
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=397)
          ZIP - INTERMEDIATE PATCH WITH ERRORS

          The status:
          I was able to synch this patch and work myself into it, but I would like to show where I am standing.
          org.odftoolkit.odfdom.doc.DocumentCreationTest:

          embDoc.insertDocument(OdfTextDocument.newTextDocument(), pathToDoc);
          Assert.assertNotNull(embDoc.getPackage().getFileEntry(embDoc.getDocumentPath() + pathToDoc));
          OdfFileDom contentDom = embDoc.getContentDom();

          XPath xpath = contentDom.getXPath();
          TextPElement lastPara = (TextPElement) xpath.evaluate("//text[last()]", contentDom, XPathConstants.NODE);
          addFrameForEmbeddedDoc(contentDom, lastPara, "Object in Object1");
          embDoc.save(ResourceUtilities.newTestOutputFile("111debug.odt"));

          There should be a embedded document being created "Object 1/Object in Object1" but it is created aside and I am puzzled.
          Any quick idea when debugging through these lines?

          Show
          Svante Schubert added a comment - Created an attachment (id=397) ZIP - INTERMEDIATE PATCH WITH ERRORS The status: I was able to synch this patch and work myself into it, but I would like to show where I am standing. org.odftoolkit.odfdom.doc.DocumentCreationTest: embDoc.insertDocument(OdfTextDocument.newTextDocument(), pathToDoc); Assert.assertNotNull(embDoc.getPackage().getFileEntry(embDoc.getDocumentPath() + pathToDoc)); OdfFileDom contentDom = embDoc.getContentDom(); XPath xpath = contentDom.getXPath(); TextPElement lastPara = (TextPElement) xpath.evaluate("//text [last()] ", contentDom, XPathConstants.NODE); addFrameForEmbeddedDoc(contentDom, lastPara, "Object in Object1"); embDoc.save(ResourceUtilities.newTestOutputFile("111debug.odt")); There should be a embedded document being created "Object 1/Object in Object1" but it is created aside and I am puzzled. Any quick idea when debugging through these lines?
          Hide
          Svante Schubert added a comment -

          Hi Daisy,

          do agree on the written below so far? Especially that OdfDocuments are opened with a path relative to the document. That was your team's first suggestion, I opposed when there was no way of absolute positioning as now possible on the OdfPackage.

          A further none implemented design decision is that any OdfPackageDocument opened in the package layer should be the correct object. For instance, and ODF Text (ODT) document opened as OdfPackageDocument should be opened as OdfTextDocument.
          By this we have no problems with casting and we only need to register on the OdfPackage layer.

          Similar with XML for DOMs. It should be possible to register MathML to the OdfPackage and provide a typed DOM classes as well, making the architecture more generic for the user.

          This does not have to be fixed within this issue, but I did a intermediate approach in the given patch to hardwire the ODF DOM XML by file name to the upper layer classes.

          PS: If you have any change to take a quick look on the issue, it would be quite appreciated.

          Thanks,
          Svante

          Show
          Svante Schubert added a comment - Hi Daisy, do agree on the written below so far? Especially that OdfDocuments are opened with a path relative to the document. That was your team's first suggestion, I opposed when there was no way of absolute positioning as now possible on the OdfPackage. A further none implemented design decision is that any OdfPackageDocument opened in the package layer should be the correct object. For instance, and ODF Text (ODT) document opened as OdfPackageDocument should be opened as OdfTextDocument. By this we have no problems with casting and we only need to register on the OdfPackage layer. Similar with XML for DOMs. It should be possible to register MathML to the OdfPackage and provide a typed DOM classes as well, making the architecture more generic for the user. This does not have to be fixed within this issue, but I did a intermediate approach in the given patch to hardwire the ODF DOM XML by file name to the upper layer classes. PS: If you have any change to take a quick look on the issue, it would be quite appreciated. Thanks, Svante
          Hide
          devin added a comment -

          (In reply to comment #3)
          > Created an attachment (id=397) [details]
          > ZIP - INTERMEDIATE PATCH WITH ERRORS
          >
          > The status:
          > I was able to synch this patch and work myself into it, but I would like to
          > show where I am standing.
          > org.odftoolkit.odfdom.doc.DocumentCreationTest:
          >
          > embDoc.insertDocument(OdfTextDocument.newTextDocument(), pathToDoc);
          >
          > Assert.assertNotNull(embDoc.getPackage().getFileEntry(embDoc.getDocumentPath()
          > + pathToDoc));
          > OdfFileDom contentDom = embDoc.getContentDom();
          >
          > XPath xpath = contentDom.getXPath();
          > TextPElement lastPara = (TextPElement) xpath.evaluate("//text[last()]",
          > contentDom, XPathConstants.NODE);
          > addFrameForEmbeddedDoc(contentDom, lastPara, "Object in Object1");
          > embDoc.save(ResourceUtilities.newTestOutputFile("111debug.odt"));
          >
          >
          > There should be a embedded document being created "Object 1/Object in Object1"
          > but it is created aside and I am puzzled.
          > Any quick idea when debugging through these lines?

          embDoc is the reference of embedded document, the path should be "Object in Object1" in this document.
          docWithEmbeddedObject is the outside document, so if you call docWithEmbeddedObject.save(ResourceUtilities.newTestOutputFile("111debug.odt"));
          you will see the directory structure "Object 1/Object in Object1".
          I am not sure whether this is the right answer you want to get

          Devin

          Show
          devin added a comment - (In reply to comment #3) > Created an attachment (id=397) [details] > ZIP - INTERMEDIATE PATCH WITH ERRORS > > The status: > I was able to synch this patch and work myself into it, but I would like to > show where I am standing. > org.odftoolkit.odfdom.doc.DocumentCreationTest: > > embDoc.insertDocument(OdfTextDocument.newTextDocument(), pathToDoc); > > Assert.assertNotNull(embDoc.getPackage().getFileEntry(embDoc.getDocumentPath() > + pathToDoc)); > OdfFileDom contentDom = embDoc.getContentDom(); > > XPath xpath = contentDom.getXPath(); > TextPElement lastPara = (TextPElement) xpath.evaluate("//text [last()] ", > contentDom, XPathConstants.NODE); > addFrameForEmbeddedDoc(contentDom, lastPara, "Object in Object1"); > embDoc.save(ResourceUtilities.newTestOutputFile("111debug.odt")); > > > There should be a embedded document being created "Object 1/Object in Object1" > but it is created aside and I am puzzled. > Any quick idea when debugging through these lines? embDoc is the reference of embedded document, the path should be "Object in Object1" in this document. docWithEmbeddedObject is the outside document, so if you call docWithEmbeddedObject.save(ResourceUtilities.newTestOutputFile("111debug.odt")); you will see the directory structure "Object 1/Object in Object1". I am not sure whether this is the right answer you want to get Devin
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=405)
          Intermediate Patch - Package Saving - moving XML functionality to package

          Just an internal update, in case Devin got space to continue during German night time.

          I adapted various things today, still in doc.DocumentCreationTest there is the following problem:

          OdfDocument emb_embDoc = rootDocument.loadSubDocument(embDoc.getDocumentPath() + pathOfSecondInnerDoc);

          Loads a document that was added to the package, but is not yet in the ZIP.
          OdfFileDom line 136 "fileStream = mPackage.getInputStream(mPackagePath);" returns null as the cached document is not correctly returned.

          Note: There is a problem with DOM we have to solve in the future, that I started to solve in this patch. We have to read the XML file first and decide upon the root element, which DOM file class (OdfContentDom, etc.) we want to use.

          In general we have three similar problems for the package class creating OdfPackageDocuments, DOM file objects and typed XML DOM objects.
          Higher level like DOC layer need to register their Documents, DOMs and XML to the package class.

          Thanks,
          Svante

          Show
          Svante Schubert added a comment - Created an attachment (id=405) Intermediate Patch - Package Saving - moving XML functionality to package Just an internal update, in case Devin got space to continue during German night time. I adapted various things today, still in doc.DocumentCreationTest there is the following problem: OdfDocument emb_embDoc = rootDocument.loadSubDocument(embDoc.getDocumentPath() + pathOfSecondInnerDoc); Loads a document that was added to the package, but is not yet in the ZIP. OdfFileDom line 136 "fileStream = mPackage.getInputStream(mPackagePath);" returns null as the cached document is not correctly returned. Note: There is a problem with DOM we have to solve in the future, that I started to solve in this patch. We have to read the XML file first and decide upon the root element, which DOM file class (OdfContentDom, etc.) we want to use. In general we have three similar problems for the package class creating OdfPackageDocuments, DOM file objects and typed XML DOM objects. Higher level like DOC layer need to register their Documents, DOMs and XML to the package class. Thanks, Svante
          Hide
          devin added a comment -

          Created an attachment (id=410)
          new version patch fixed test failure issues

          Hi, Svante
          Hope you have a good holiday
          It's a nightmare to review and update a so complex patch. Thank god, after a painful struggle nearly one week, all of the test cases have passed.
          I don't want to explain the detail of the fix process, in order to avoid transferring the nightmare to you. Most of the test failures are caused by incorrect directory or incorrect variable settings, which are trivial but boring.
          I suggest you push this patch ASAP, even if you aren't satisfied with it 100%. Some little issues, we can fix it later. Anyway, I so tired and don't want to see this large patch any more...

          Thanks&Regards,
          Devin

          Show
          devin added a comment - Created an attachment (id=410) new version patch fixed test failure issues Hi, Svante Hope you have a good holiday It's a nightmare to review and update a so complex patch. Thank god, after a painful struggle nearly one week, all of the test cases have passed. I don't want to explain the detail of the fix process, in order to avoid transferring the nightmare to you. Most of the test failures are caused by incorrect directory or incorrect variable settings, which are trivial but boring. I suggest you push this patch ASAP, even if you aren't satisfied with it 100%. Some little issues, we can fix it later. Anyway, I so tired and don't want to see this large patch any more... Thanks&Regards, Devin
          Hide
          devin added a comment -

          assign to svante

          Show
          devin added a comment - assign to svante
          Hide
          Svante Schubert added a comment -

          Hi Devin,

          thanks for the great work you have provided.

          I did some review today and agree that the patch become to large in the end.
          Although if I am not completly mistaken, you have added as well some parts of the recent reopned issue 69 to the patch (e.g. OdfDomDocument.java).
          But that is fine for me, a fair price for your hard work.

          The downside, I need more time to review, than I am able to offer during my vacation, therefore I will finish it starting next week.

          Two points I would already discuss with my colleagues:
          1) As W3C DOM API uses the term Document in the context of DOM for an XML document, but the OdfDomDocument represents a ZIP directory with a MIME type described by the ODF XML Schema specification - including all XML implementation details - we might perhaps want to call the class OdfSchemaDocument

          2) I am uncertain if it does not serve the user better to call all methods of the inheriting document classes getDocument() allow overwriting.

          PS: I will leave my quarters in Amsterdam tomorrow and will not have Internet until Monday morning back in the Office.

          Have a nice week-end,
          Svante

          Show
          Svante Schubert added a comment - Hi Devin, thanks for the great work you have provided. I did some review today and agree that the patch become to large in the end. Although if I am not completly mistaken, you have added as well some parts of the recent reopned issue 69 to the patch (e.g. OdfDomDocument.java). But that is fine for me, a fair price for your hard work. The downside, I need more time to review, than I am able to offer during my vacation, therefore I will finish it starting next week. Two points I would already discuss with my colleagues: 1) As W3C DOM API uses the term Document in the context of DOM for an XML document, but the OdfDomDocument represents a ZIP directory with a MIME type described by the ODF XML Schema specification - including all XML implementation details - we might perhaps want to call the class OdfSchemaDocument 2) I am uncertain if it does not serve the user better to call all methods of the inheriting document classes getDocument() allow overwriting. PS: I will leave my quarters in Amsterdam tomorrow and will not have Internet until Monday morning back in the Office. Have a nice week-end, Svante
          Hide
          devin added a comment -

          Hi Svante,
          Thanks for your response.
          1) Yes, I added some parts of the issue 69 to the patch. As I have stated before, I found some repeated work between the two patches. So, I added them to avoid potential conflicts. Then I can update the patch of ODFTOOLKIT-46 easier after this issue pushed
          2) I agree with you that OdfSchemaDocument is more appropriate than OdfDomDocument, you can rename it when you are reviewing.
          3) About getDocument() method, I think your opinion is reasonable. Overwriting getDocument() will let the api easy to use.

          Best regards,
          Devin

          Show
          devin added a comment - Hi Svante, Thanks for your response. 1) Yes, I added some parts of the issue 69 to the patch. As I have stated before, I found some repeated work between the two patches. So, I added them to avoid potential conflicts. Then I can update the patch of ODFTOOLKIT-46 easier after this issue pushed 2) I agree with you that OdfSchemaDocument is more appropriate than OdfDomDocument, you can rename it when you are reviewing. 3) About getDocument() method, I think your opinion is reasonable. Overwriting getDocument() will let the api easy to use. Best regards, Devin
          Hide
          Svante Schubert added a comment -

          Hi Devin,

          I have reviewed your extensive work, thank you very much again.

          There are two things remaining for us:

          1) The method of OdfDocument
          public List<OdfDocument> loadSubDocuments()
          is very instable as it uses a list in a none defined order. There are even tests that rely that a document comes at a certain place. This is highly instable and breaks as soon a new document gets into the document.
          Instead a Map with the path as key and the document as value should be returned.

          2) Hard, but true. The original reason of this issue is not yet fulfilled.
          I would take care of the second part and I would like you take care of the first as you added the List to the API, I would ask you to adapt the method. Unfortunately I realized the problem quite late when I was working on the tests and not at the first review. The rule that a API have to be used evolve is true.

          Thanks in advance,
          Svante

          Show
          Svante Schubert added a comment - Hi Devin, I have reviewed your extensive work, thank you very much again. There are two things remaining for us: 1) The method of OdfDocument public List<OdfDocument> loadSubDocuments() is very instable as it uses a list in a none defined order. There are even tests that rely that a document comes at a certain place. This is highly instable and breaks as soon a new document gets into the document. Instead a Map with the path as key and the document as value should be returned. 2) Hard, but true. The original reason of this issue is not yet fulfilled. I would take care of the second part and I would like you take care of the first as you added the List to the API, I would ask you to adapt the method. Unfortunately I realized the problem quite late when I was working on the tests and not at the first review. The rule that a API have to be used evolve is true. Thanks in advance, Svante
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=412)
          Reviewed Patch with 2 problems remaining..

          Show
          Svante Schubert added a comment - Created an attachment (id=412) Reviewed Patch with 2 problems remaining..
          Hide
          devin added a comment -

          Created an attachment (id=413)
          bug219 patch method exchange

          Hi, Svante

          I have exchanged the return type of loadSubDocuments() from List to Map as your need. You can continue the second part now.
          Hope you can finish your work ASAP, then we can release ASAP

          Thanks&Regards
          Devin

          Show
          devin added a comment - Created an attachment (id=413) bug219 patch method exchange Hi, Svante I have exchanged the return type of loadSubDocuments() from List to Map as your need. You can continue the second part now. Hope you can finish your work ASAP, then we can release ASAP Thanks&Regards Devin
          Hide
          Svante Schubert added a comment -

          Hi Devin,

          I reviewed your patch and thank you for the given changes.
          As you exchanged the method below a second time to public from package

          public OdfPackageDocument getCachedPackageDocument(String internalPath)

          I assume it did not happened by accident.
          Please use instead

          public OdfPackageDocument loadPackageDocument(String internalPath)

          It calls getCachedPackageDocument and if not available loads the document instead.
          The state of being cached is an implementation detail and should not be part of the API.

          Would you agree?
          I continue to work now on the patch.

          Regards,
          Svante

          Show
          Svante Schubert added a comment - Hi Devin, I reviewed your patch and thank you for the given changes. As you exchanged the method below a second time to public from package public OdfPackageDocument getCachedPackageDocument(String internalPath) I assume it did not happened by accident. Please use instead public OdfPackageDocument loadPackageDocument(String internalPath) It calls getCachedPackageDocument and if not available loads the document instead. The state of being cached is an implementation detail and should not be part of the API. Would you agree? I continue to work now on the patch. Regards, Svante
          Hide
          devin added a comment -

          Hi Svante,
          I guessed you might ask this question
          I don't think loadPackageDocument is a better way than getCachedPackageDocument.
          I saw some code like:
          try {
          String documentMediaType = getMediaTypeString(internalPath);
          odfMediaType = OdfMediaType.getOdfMediaType(documentMediaType);
          if (odfMediaType == null)

          { return null; }

          // ToDo: Remove dependency by facotory issue ??? (to be written)
          doc = OdfDocument.loadDocument(this, internalPath);
          }
          There are OdfDocument and OdfMediaType in this method realization. These two classes are in doc layer. Shouldn't be used in this location, isn't it? I don't think it's sensible to supply this intermediate as API. If a user only want to use package layer classes to realize a function. How could he do?

          Now let's talk about getCachedPackageDocument, which may be an implementation detail for doc or dom layer, but it's possible to be considered as an API for package layer. Whether a method should be published as an API, different layer has different request. OdfPackage represents the package view to an OpenDocument document. The state of being cached is not an implementation detail, but an usefull API for the programming of package layer. At least it's a stable and determinate realization than loadPackageDocument.

          So, I insist to exchange getCachedPackageDocument to public from package.

          But team cooperation needs compromise. As splitting the difference, in the future, if you remove the dependency of OdfDocument and OdfMediaType from loadPackageDocument, I will agree to change getCachedPackageDocument to package.

          Regards,
          Devin

          > Hi Devin,
          >
          > I reviewed your patch and thank you for the given changes.
          > As you exchanged the method below a second time to public from package
          >
          > public OdfPackageDocument getCachedPackageDocument(String internalPath)
          >
          > I assume it did not happened by accident.
          > Please use instead
          >
          > public OdfPackageDocument loadPackageDocument(String internalPath)
          >
          > It calls getCachedPackageDocument and if not available loads the document
          > instead.
          > The state of being cached is an implementation detail and should not be part of
          > the API.
          >
          > Would you agree?
          > I continue to work now on the patch.
          >
          > Regards,
          > Svante

          Show
          devin added a comment - Hi Svante, I guessed you might ask this question I don't think loadPackageDocument is a better way than getCachedPackageDocument. I saw some code like: try { String documentMediaType = getMediaTypeString(internalPath); odfMediaType = OdfMediaType.getOdfMediaType(documentMediaType); if (odfMediaType == null) { return null; } // ToDo: Remove dependency by facotory issue ??? (to be written) doc = OdfDocument.loadDocument(this, internalPath); } There are OdfDocument and OdfMediaType in this method realization. These two classes are in doc layer. Shouldn't be used in this location, isn't it? I don't think it's sensible to supply this intermediate as API. If a user only want to use package layer classes to realize a function. How could he do? Now let's talk about getCachedPackageDocument, which may be an implementation detail for doc or dom layer, but it's possible to be considered as an API for package layer. Whether a method should be published as an API, different layer has different request. OdfPackage represents the package view to an OpenDocument document. The state of being cached is not an implementation detail, but an usefull API for the programming of package layer. At least it's a stable and determinate realization than loadPackageDocument. So, I insist to exchange getCachedPackageDocument to public from package. But team cooperation needs compromise. As splitting the difference, in the future, if you remove the dependency of OdfDocument and OdfMediaType from loadPackageDocument, I will agree to change getCachedPackageDocument to package. Regards, Devin > Hi Devin, > > I reviewed your patch and thank you for the given changes. > As you exchanged the method below a second time to public from package > > public OdfPackageDocument getCachedPackageDocument(String internalPath) > > I assume it did not happened by accident. > Please use instead > > public OdfPackageDocument loadPackageDocument(String internalPath) > > It calls getCachedPackageDocument and if not available loads the document > instead. > The state of being cached is an implementation detail and should not be part of > the API. > > Would you agree? > I continue to work now on the patch. > > Regards, > Svante
          Hide
          Svante Schubert added a comment -

          Hi Devin,

          let me try to rephrase your two statements:

          1) As the method loadPackageDocument uses DOC classes, you do not want to use that method.

          2) You state of caching of a document within the package is not an implementation detail, but needs to become part of the API.

          My reply might be:
          1) Of course DOC should not be used, and will be forbidden when the API is stable.
          But it is yet necessary to return the correct Document type for our ODF documents. Otherwise the loadDocument of OdfPackage would always return an OdfPackageDocument, but never for instance an OdfTextDocument from DOC layer and it can not be changed afterwards.
          Therefore as long we have no more sophisticated mechanism to register document, dom and element classes to the package, we are in need of breaking the architecture behind the scenes, and fixing it for 0.9.
          The methods signatures nevertheless are already forbidden to break those rules.

          In any case, by avoiding the usage of the method, and instead using a different method of the same class, will not change this dependency problem and therefore makes no difference from the user perspective.

          2) You state that it is not implementation detail, but as caching is not part of the ODF spec and the existence of caching should not consider the user at all to me it is a good example for an implementation detail.

          May I ask for the user scenario that requires the knowledge of caching?

          PS: If you anticipated the dispute, why did you not bring it to issue in the first place, but waited till I corrected it a second time?

          PPS: There is no insisting on things, if you and me disagree we have to bring it to the list and discuss our stand points there.

          Regards,
          Svante

          Show
          Svante Schubert added a comment - Hi Devin, let me try to rephrase your two statements: 1) As the method loadPackageDocument uses DOC classes, you do not want to use that method. 2) You state of caching of a document within the package is not an implementation detail, but needs to become part of the API. My reply might be: 1) Of course DOC should not be used, and will be forbidden when the API is stable. But it is yet necessary to return the correct Document type for our ODF documents. Otherwise the loadDocument of OdfPackage would always return an OdfPackageDocument, but never for instance an OdfTextDocument from DOC layer and it can not be changed afterwards. Therefore as long we have no more sophisticated mechanism to register document, dom and element classes to the package, we are in need of breaking the architecture behind the scenes, and fixing it for 0.9. The methods signatures nevertheless are already forbidden to break those rules. In any case, by avoiding the usage of the method, and instead using a different method of the same class, will not change this dependency problem and therefore makes no difference from the user perspective. 2) You state that it is not implementation detail, but as caching is not part of the ODF spec and the existence of caching should not consider the user at all to me it is a good example for an implementation detail. May I ask for the user scenario that requires the knowledge of caching? PS: If you anticipated the dispute, why did you not bring it to issue in the first place, but waited till I corrected it a second time? PPS: There is no insisting on things, if you and me disagree we have to bring it to the list and discuss our stand points there. Regards, Svante
          Hide
          Svante Schubert added a comment -

          Hi Devin,

          I did a review with Frank Meies on this issue in regard of ODFTOOLKIT-46, which you merged.

          We found two further implementation details in the DOC layer that should be moved to the OdfSchemaDocument:

          1)
          From all Document classes the getContentRoot() have to be moved, e.g. for OdfDocument
          public OdfElement getContentRoot() throws Exception

          2) Strictly talking, the OdfMediaType is a implementation detail as well, using the file suffix and even worse the mimetype.
          I have to sleep over it how to solve this at best and if the alternative is better, as so much is certain the change of the mode (e.g. to template) is necesary in DOC layer document.

          Would it be possible for you to adapt the getContentRoot() change?

          Regards,
          Svante

          Show
          Svante Schubert added a comment - Hi Devin, I did a review with Frank Meies on this issue in regard of ODFTOOLKIT-46 , which you merged. We found two further implementation details in the DOC layer that should be moved to the OdfSchemaDocument: 1) From all Document classes the getContentRoot() have to be moved, e.g. for OdfDocument public OdfElement getContentRoot() throws Exception 2) Strictly talking, the OdfMediaType is a implementation detail as well, using the file suffix and even worse the mimetype. I have to sleep over it how to solve this at best and if the alternative is better, as so much is certain the change of the mode (e.g. to template) is necesary in DOC layer document. Would it be possible for you to adapt the getContentRoot() change? Regards, Svante
          Hide
          Svante Schubert added a comment -

          Hi Devin,

          when we consider the mediatype, we might as well consider the ScriptType to be moved to DOM package/OdfSchemaDocument.

          After my holiday the methods in OdfDocument were much harder to understand.
          I suggest to keep those methods in OdfDocument as the Locale setting is quite convenient, but I suggest further to ease the methods a little bit:

          1) I find the naming ScriptType, which is an OOO term quite confusing and changed it to UnicodeGroup, as this is what it is about. CJK, CTL and the rest are groups of UnicodeCharacters.

          2) If
          public void setLocale(Locale locale, UnicodeGroup unicodeGroup) does have no effect if the UnicodeGroup does not relate to the Locale, why allow it at all aside
          public void setLocale(Locale locale)
          therefore turned it to private

          3) If setLocale is private, the initial check of the private methods setDefaultAsianLanguage(locale);
          setDefaultComplexLanguage(locale);
          setWesternLanguage(locale);
          can be dropped, saving us the time.

          4) All methods above are private so it as no effect on the API of the previous patch, but I renamed setWesternLanguage consistently to setDefaultWesternLanguage.

          5) The user has no idea about our mapping, therefore I changed the private none static method getScriptType(Locale l) to
          public static UnicodeGroup getUnicodeGroup(Locale locale)

          In addition I have updated the JavaDoc.
          Although it was your issue once, I had to fix it to see if it works fine and the changes evolved during refactoring.

          Please review the changes in OdfDocument.
          Patch will follow later..
          Svante

          Show
          Svante Schubert added a comment - Hi Devin, when we consider the mediatype, we might as well consider the ScriptType to be moved to DOM package/OdfSchemaDocument. After my holiday the methods in OdfDocument were much harder to understand. I suggest to keep those methods in OdfDocument as the Locale setting is quite convenient, but I suggest further to ease the methods a little bit: 1) I find the naming ScriptType, which is an OOO term quite confusing and changed it to UnicodeGroup, as this is what it is about. CJK, CTL and the rest are groups of UnicodeCharacters. 2) If public void setLocale(Locale locale, UnicodeGroup unicodeGroup) does have no effect if the UnicodeGroup does not relate to the Locale, why allow it at all aside public void setLocale(Locale locale) therefore turned it to private 3) If setLocale is private, the initial check of the private methods setDefaultAsianLanguage(locale); setDefaultComplexLanguage(locale); setWesternLanguage(locale); can be dropped, saving us the time. 4) All methods above are private so it as no effect on the API of the previous patch, but I renamed setWesternLanguage consistently to setDefaultWesternLanguage. 5) The user has no idea about our mapping, therefore I changed the private none static method getScriptType(Locale l) to public static UnicodeGroup getUnicodeGroup(Locale locale) In addition I have updated the JavaDoc. Although it was your issue once, I had to fix it to see if it works fine and the changes evolved during refactoring. Please review the changes in OdfDocument. Patch will follow later.. Svante
          Hide
          Svante Schubert added a comment -

          From OdfDocument the method
          public void insertDocument(OdfPackageDocument sourceDocument, String documentPath)

          had been moved to OdfPackageDocument as this functionality should be available for all OdfPackageDocuments.
          Even for those, that have a different mimetype as of declared in the ODF Spec.

          Looking at the method, it seems there are two issues here
          public void insertDocument(OdfPackageDocument newDocument, String documentPath)

          { mPackage.insertDocument(newDocument, documentPath); }

          1) While the paths on the document are all relative to the document root, the path of the package are all relative to the package root.

          2) The DOM of the document to be inserted have to be flushed first (seems a test is missing)

          Therefore it should be:
          public void insertDocument(OdfPackageDocument newDocument, String documentPath)

          { newDocument.flushDoms(); mPackage.insertDocument(newDocument, mDocumentPathInPackage + documentPath); }

          As the day is at the end, I would be thankful if you could verify the above (and the other document methods with paths) and adjust source and tests.

          Thanks in advance..

          I have started to overwork the save functionality but could not fix it completly today. Will continue tomorrow..

          Show
          Svante Schubert added a comment - From OdfDocument the method public void insertDocument(OdfPackageDocument sourceDocument, String documentPath) had been moved to OdfPackageDocument as this functionality should be available for all OdfPackageDocuments. Even for those, that have a different mimetype as of declared in the ODF Spec. Looking at the method, it seems there are two issues here public void insertDocument(OdfPackageDocument newDocument, String documentPath) { mPackage.insertDocument(newDocument, documentPath); } 1) While the paths on the document are all relative to the document root, the path of the package are all relative to the package root. 2) The DOM of the document to be inserted have to be flushed first (seems a test is missing) Therefore it should be: public void insertDocument(OdfPackageDocument newDocument, String documentPath) { newDocument.flushDoms(); mPackage.insertDocument(newDocument, mDocumentPathInPackage + documentPath); } As the day is at the end, I would be thankful if you could verify the above (and the other document methods with paths) and adjust source and tests. Thanks in advance.. I have started to overwork the save functionality but could not fix it completly today. Will continue tomorrow..
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=414)
          Intermediate patch - review of 69 merge

          Show
          Svante Schubert added a comment - Created an attachment (id=414) Intermediate patch - review of 69 merge
          Hide
          Svante Schubert added a comment -

          One area I forgot to mention:

          All the loadSubDocuments methods of OdfDocument have to be in OdfPackageDocument as well, but using OdfPackageDocument instead of OdfDocument and creating a document for every directory with mimetype.

          In addition we are always instantiating all documents. What if the user does not need all the documents, what about offering a method

          Set<String> getSubDocuments() for all paths of potential OdfDocument

          overriding the OdfDocument method

          Set<String> getSubDocuments() for every document not only ODF

          Keep in mind that even a document (or DOM) created on the Package level should be of the instance of the higher level DOC API. Currently it will be a hack behind the scene and later the necessary registration will be added.

          Could you take care of this?
          Than all that is left for me should be the save method (hopefully) as I need to do some ODF TC work..

          Show
          Svante Schubert added a comment - One area I forgot to mention: All the loadSubDocuments methods of OdfDocument have to be in OdfPackageDocument as well, but using OdfPackageDocument instead of OdfDocument and creating a document for every directory with mimetype. In addition we are always instantiating all documents. What if the user does not need all the documents, what about offering a method Set<String> getSubDocuments() for all paths of potential OdfDocument overriding the OdfDocument method Set<String> getSubDocuments() for every document not only ODF Keep in mind that even a document (or DOM) created on the Package level should be of the instance of the higher level DOC API. Currently it will be a hack behind the scene and later the necessary registration will be added. Could you take care of this? Than all that is left for me should be the save method (hopefully) as I need to do some ODF TC work..
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=415)
          Additional minor api changes based on patch 414 that still causes problems

          Hi Devin,

          first the good news, you might use yesterdays patch with the deprecation+public added as fall-back.

          I reviewed the methods of OdfPackage, OdfPackageDocument and OdfDocument for consistency naming. I did some renamings and found three issues.
          Even when I turned back the fixes and kept only the renaming the test fail and I have really no idea could have caused this. My only guess was that now method is being overloaded, but if this is the case I have overseen this case.

          I run into problems of the duplicate use of empty string and slash as document root, but the attempt to use consistently slash and check when doc path is being added to neglect the slash if it is the root document lead to no success.

          As I said if you are not able to find the problem, neither we might want to keep the API for Fridays snapshot.

          Regards,
          Svante

          Show
          Svante Schubert added a comment - Created an attachment (id=415) Additional minor api changes based on patch 414 that still causes problems Hi Devin, first the good news, you might use yesterdays patch with the deprecation+public added as fall-back. I reviewed the methods of OdfPackage, OdfPackageDocument and OdfDocument for consistency naming. I did some renamings and found three issues. Even when I turned back the fixes and kept only the renaming the test fail and I have really no idea could have caused this. My only guess was that now method is being overloaded, but if this is the case I have overseen this case. I run into problems of the duplicate use of empty string and slash as document root, but the attempt to use consistently slash and check when doc path is being added to neglect the slash if it is the root document lead to no success. As I said if you are not able to find the problem, neither we might want to keep the API for Fridays snapshot. Regards, Svante
          Hide
          devin added a comment -

          Hi Svante,
          Thanks for your response.
          I have updated patch 414 as you request and pushed it.
          If I have time after finishing the snapshot work, I will help you review patch 415.

          Thanks&Regards
          Devin

          > Created an attachment (id=415) [details]
          > Additional minor api changes based on patch 414 that still causes problems
          >
          > Hi Devin,
          >
          > first the good news, you might use yesterdays patch with the deprecation+public
          > added as fall-back.
          >
          > I reviewed the methods of OdfPackage, OdfPackageDocument and OdfDocument for
          > consistency naming. I did some renamings and found three issues.
          > Even when I turned back the fixes and kept only the renaming the test fail and
          > I have really no idea could have caused this. My only guess was that now method
          > is being overloaded, but if this is the case I have overseen this case.
          >
          > I run into problems of the duplicate use of empty string and slash as document
          > root, but the attempt to use consistently slash and check when doc path is
          > being added to neglect the slash if it is the root document lead to no success.
          >
          > As I said if you are not able to find the problem, neither we might want to
          > keep the API for Fridays snapshot.
          >
          > Regards,
          > Svante

          Show
          devin added a comment - Hi Svante, Thanks for your response. I have updated patch 414 as you request and pushed it. If I have time after finishing the snapshot work, I will help you review patch 415. Thanks&Regards Devin > Created an attachment (id=415) [details] > Additional minor api changes based on patch 414 that still causes problems > > Hi Devin, > > first the good news, you might use yesterdays patch with the deprecation+public > added as fall-back. > > I reviewed the methods of OdfPackage, OdfPackageDocument and OdfDocument for > consistency naming. I did some renamings and found three issues. > Even when I turned back the fixes and kept only the renaming the test fail and > I have really no idea could have caused this. My only guess was that now method > is being overloaded, but if this is the case I have overseen this case. > > I run into problems of the duplicate use of empty string and slash as document > root, but the attempt to use consistently slash and check when doc path is > being added to neglect the slash if it is the root document lead to no success. > > As I said if you are not able to find the problem, neither we might want to > keep the API for Fridays snapshot. > > Regards, > Svante
          Hide
          Svante Schubert added a comment -

          Hi Devin,

          Just for your interest, I have found the issue, but I have not merged it into your version, yet.

          In OdfPackageDocument.init()

          pkg.insertPackageDocument(this, internalPath);
          was changed to
          pkg.insertDocument(this, internalPath);

          But it should have named to:
          pkg.cacheDocument(this, internalPath);

          With this the build runs smooth:

          Regards,
          Svante

          Show
          Svante Schubert added a comment - Hi Devin, Just for your interest, I have found the issue, but I have not merged it into your version, yet. In OdfPackageDocument.init() pkg.insertPackageDocument(this, internalPath); was changed to pkg.insertDocument(this, internalPath); But it should have named to: pkg.cacheDocument(this, internalPath); With this the build runs smooth: Regards, Svante
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=416)
          Minor API change & minor bug fix

          Hi Devin,

          as discussed on last weeks call it would be great if you could assist me on the next full release, after my assistance on the snapshot.
          To ease your life, I have merged and uploaded my work of last Thursday. Within the patch you will be able to find two FIXME, which would be great if you could dedicate yourself on it.
          I already fixed the third: In OdfPackage the subset of documents for a certain media type will never be narrowed, added a test.

          The other two are within OdfPackageDocument, insert/removeDocument gets a parameter as path, which is relative to the document. When the same named package funktion is being called, the path have to be made absolute.
          I suggest to expand the tests first, this would fail a document is being inserted to another document, the fix was easy and is already given as comment.

          Meanwhile I will focus on the save of the package. Not only saving all dom and docs by the package, but as well test the idea of saving the document aside and moving it (as other ODF applications are handling it).
          Including the idea to differentiate the saving in the same file as "save" from into a different location via "saveas" to spare us the overhead of copying the ZIP content on every time as JDK ZIPs are not extensible.

          PS: I would suggest to make it possible that with insert/removeDocument even the root document is being changed.

          Best regards,
          Svante

          Show
          Svante Schubert added a comment - Created an attachment (id=416) Minor API change & minor bug fix Hi Devin, as discussed on last weeks call it would be great if you could assist me on the next full release, after my assistance on the snapshot. To ease your life, I have merged and uploaded my work of last Thursday. Within the patch you will be able to find two FIXME, which would be great if you could dedicate yourself on it. I already fixed the third: In OdfPackage the subset of documents for a certain media type will never be narrowed, added a test. The other two are within OdfPackageDocument, insert/removeDocument gets a parameter as path, which is relative to the document. When the same named package funktion is being called, the path have to be made absolute. I suggest to expand the tests first, this would fail a document is being inserted to another document, the fix was easy and is already given as comment. Meanwhile I will focus on the save of the package. Not only saving all dom and docs by the package, but as well test the idea of saving the document aside and moving it (as other ODF applications are handling it). Including the idea to differentiate the saving in the same file as "save" from into a different location via "saveas" to spare us the overhead of copying the ZIP content on every time as JDK ZIPs are not extensible. PS: I would suggest to make it possible that with insert/removeDocument even the root document is being changed. Best regards, Svante
          Hide
          Svante Schubert added a comment -

          Give the token back to you, as the next week starts earlier in China..
          Thanks
          Svante

          Show
          Svante Schubert added a comment - Give the token back to you, as the next week starts earlier in China.. Thanks Svante
          Hide
          devin added a comment -

          Hi Svante,

          Pls update your patch based on the newest code on master repository.
          Thank you!

          Devin
          > Give the token back to you, as the next week starts earlier in China..
          > Thanks
          > Svante

          Show
          devin added a comment - Hi Svante, Pls update your patch based on the newest code on master repository. Thank you! Devin > Give the token back to you, as the next week starts earlier in China.. > Thanks > Svante
          Hide
          Svante Schubert added a comment -

          I am puzzled, Devin. What master repository and patch are you referring to?

          Everything was already synched, see "hg log -l1"

          changeset: 100:94c50603bbeb
          tag: tip
          user: Developers:Svante,Devin;Reviewers:Devin,Svante
          date: Fri Oct 29 10:20:26 2010 +0800
          summary: #ODFTOOLKIT-149# INTERMEDIATE PATCH – OdfPackag.save(..) should save as w
          ell all its open documents

          And the header of the patch, http://odftoolkit.org/bugzilla/attachment.cgi?id=416

          1. HG changeset patch
          2. User Svante Schubert <svanteschubert@odftoolkit.org>
          3. Date 1288372882 -7200
          4. Node ID 0db8fd757c0925e1bbdd39684e9a3a38c268ea56
          5. Parent 94c50603bbebbb8df87e088f2b2981cd4329ae62
            #ODFTOOLKIT-149# Intermediate patch after snapshot

          Could you please idle this week in the IRC, so we might speed up the fix of this issue.

          Thanks,
          Svante

          Show
          Svante Schubert added a comment - I am puzzled, Devin. What master repository and patch are you referring to? Everything was already synched, see "hg log -l1" changeset: 100:94c50603bbeb tag: tip user: Developers:Svante,Devin;Reviewers:Devin,Svante date: Fri Oct 29 10:20:26 2010 +0800 summary: # ODFTOOLKIT-149 # INTERMEDIATE PATCH – OdfPackag.save(..) should save as w ell all its open documents And the header of the patch, http://odftoolkit.org/bugzilla/attachment.cgi?id=416 HG changeset patch User Svante Schubert <svanteschubert@odftoolkit.org> Date 1288372882 -7200 Node ID 0db8fd757c0925e1bbdd39684e9a3a38c268ea56 Parent 94c50603bbebbb8df87e088f2b2981cd4329ae62 # ODFTOOLKIT-149 # Intermediate patch after snapshot Could you please idle this week in the IRC, so we might speed up the fix of this issue. Thanks, Svante
          Hide
          devin added a comment -

          Hi Svante,
          As we discussed on IRC, I think we must clear two issues:
          1)the path relative to the root document or current document. As far as I know, we only hope use the path relative to the root document in OdfPackage, isn't it? But I see nearly all of the path argument are named "internalPath". It's lead too much difficulty for our work. So , I suggest we clear up "internalPath" and give those path relative to the root document a new name. It's also an opportunity to clear our design.
          2)The root patch should be "" or "/". manifest.xml needs patch star with "", while zip package file needs patch start with "/". They influence the realization of method normalizeDocumentPath. It's easy to lead disorder. We should unify the root path setting. And we should realize different normalizeDocumentPath for manifest.xml and zip package file.
          I believe if we clear these two issues, it will be not only easy to work on this path, but also make our code looks more clear.

          Regards,
          Devin

          Show
          devin added a comment - Hi Svante, As we discussed on IRC, I think we must clear two issues: 1)the path relative to the root document or current document. As far as I know, we only hope use the path relative to the root document in OdfPackage, isn't it? But I see nearly all of the path argument are named "internalPath". It's lead too much difficulty for our work. So , I suggest we clear up "internalPath" and give those path relative to the root document a new name. It's also an opportunity to clear our design. 2)The root patch should be "" or "/". manifest.xml needs patch star with "", while zip package file needs patch start with "/". They influence the realization of method normalizeDocumentPath. It's easy to lead disorder. We should unify the root path setting. And we should realize different normalizeDocumentPath for manifest.xml and zip package file. I believe if we clear these two issues, it will be not only easy to work on this path, but also make our code looks more clear. Regards, Devin
          Hide
          Svante Schubert added a comment -

          > As we discussed on IRC, I think we must clear two issues:
          > 1)the path relative to the root document or current document. As far as I know,
          > we only hope use the path relative to the root document in OdfPackage, isn't
          > it? But I see nearly all of the path argument are named "internalPath". It's
          > lead too much difficulty for our work. So , I suggest we clear up
          > "internalPath" and give those path relative to the root document a new name.
          > It's also an opportunity to clear our design.
          I stumbled over this inconsistency myself and I am happy to have one consistent naming for all classes.
          As all paths have in common that they are relative to its class (e.g. OdfPackage, OdfDocument, OdfFile, OdfContent, etc.),
          I would suggest to call the paramater. relativePath and describe in the JavaDoc of the method explizit its relation.

          > 2)The root patch should be "" or "/". manifest.xml needs patch star with "",
          > while zip package file needs patch start with "/". They influence the
          > realization of method normalizeDocumentPath. It's easy to lead disorder. We
          > should unify the root path setting. And we should realize different
          > normalizeDocumentPath for manifest.xml and zip package file.
          Again I had the same observation.
          We might stick with our conclusion from the IRC and stick with the "/" used in ODF Specification and in case of patch concatenation (document root to a path of a file within) check for the root document "/" to prevent the duplication of "/" at the start of the path.

          PS: I will have to continue on the OdfValidator first as all ODF users need an update of the validator on lastest ODF Schema.

          Best regards,
          Svante

          Show
          Svante Schubert added a comment - > As we discussed on IRC, I think we must clear two issues: > 1)the path relative to the root document or current document. As far as I know, > we only hope use the path relative to the root document in OdfPackage, isn't > it? But I see nearly all of the path argument are named "internalPath". It's > lead too much difficulty for our work. So , I suggest we clear up > "internalPath" and give those path relative to the root document a new name. > It's also an opportunity to clear our design. I stumbled over this inconsistency myself and I am happy to have one consistent naming for all classes. As all paths have in common that they are relative to its class (e.g. OdfPackage, OdfDocument, OdfFile, OdfContent, etc.), I would suggest to call the paramater. relativePath and describe in the JavaDoc of the method explizit its relation. > 2)The root patch should be "" or "/". manifest.xml needs patch star with "", > while zip package file needs patch start with "/". They influence the > realization of method normalizeDocumentPath. It's easy to lead disorder. We > should unify the root path setting. And we should realize different > normalizeDocumentPath for manifest.xml and zip package file. Again I had the same observation. We might stick with our conclusion from the IRC and stick with the "/" used in ODF Specification and in case of patch concatenation (document root to a path of a file within) check for the root document "/" to prevent the duplication of "/" at the start of the path. PS: I will have to continue on the OdfValidator first as all ODF users need an update of the validator on lastest ODF Schema. Best regards, Svante
          Hide
          Svante Schubert added a comment -

          Hi Devin,

          after reviewing the last week's ODF 1.2 part 3 (ODF Package) draft, I realized that I might have misinterpreted the spec regarding the root document path "/".

          I had the assumption that any access of the root document (the root directory of the package) would have to be by "/" SLASH.

          In fact, it only have to be addressed as "/" when being used within the manifest.xml
          manifest:manifest/manifest:file-entry/@manifest:full-path attribute.

          In all other cases, I suggest to use an empty string to refer to the root document as an empty string would be returned by our path remove-dot normaliziation, according to http://www.apps.ietf.org/rfc/rfc3986.html#sec-5.2.4

          If there is anything left to discuss, I will be at the IRC the next two days..

          Thanks,
          Svante

          Show
          Svante Schubert added a comment - Hi Devin, after reviewing the last week's ODF 1.2 part 3 (ODF Package) draft, I realized that I might have misinterpreted the spec regarding the root document path "/". I had the assumption that any access of the root document (the root directory of the package) would have to be by "/" SLASH. In fact, it only have to be addressed as "/" when being used within the manifest.xml manifest:manifest/manifest:file-entry/@manifest:full-path attribute. In all other cases, I suggest to use an empty string to refer to the root document as an empty string would be returned by our path remove-dot normaliziation, according to http://www.apps.ietf.org/rfc/rfc3986.html#sec-5.2.4 If there is anything left to discuss, I will be at the IRC the next two days.. Thanks, Svante
          Hide
          Svante Schubert added a comment -

          To avoid merge conflicts I suggest to review and integrate the patches of issue 66 prior to finish one.

          Show
          Svante Schubert added a comment - To avoid merge conflicts I suggest to review and integrate the patches of issue 66 prior to finish one.
          Hide
          devin added a comment -

          Ok, I will review and integrate this issue after you push bug66.

          > To avoid merge conflicts I suggest to review and integrate the patches of issue
          > 66 prior to finish one.

          Show
          devin added a comment - Ok, I will review and integrate this issue after you push bug66. > To avoid merge conflicts I suggest to review and integrate the patches of issue > 66 prior to finish one.
          Hide
          devin added a comment -

          Hi Svante,
          I have pushed your newest patch.
          As there is an intermediate patch, I keep this issue status as open.
          BTW: I saw some messages, such as
          INFO: EXPECTED VALIDATION MESSAGE:"The directory 'ObjectReplacements/' is not a
          sub-document and should not be listed in the 'META-INF/manifest.xml' file of ODF
          package 'file:/C:/ODFWorkspace/odfdom~developer/target/test-classes/performance
          /Presentation1.odp'!"
          I think we should update all of the test files to fit the validation rules in future.

          Thanks,
          Devin

          Show
          devin added a comment - Hi Svante, I have pushed your newest patch. As there is an intermediate patch, I keep this issue status as open. BTW: I saw some messages, such as INFO: EXPECTED VALIDATION MESSAGE:"The directory 'ObjectReplacements/' is not a sub-document and should not be listed in the 'META-INF/manifest.xml' file of ODF package 'file:/C:/ODFWorkspace/odfdom~developer/target/test-classes/performance /Presentation1.odp'!" I think we should update all of the test files to fit the validation rules in future. Thanks, Devin
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=454)
          Methods now act as explained in JavaDoc

          The OdfPackagedocument insertDocument(String path) and removeDocument(String path) will now use a document path as parameter, which is relative to the document.

          This was already explained in the JavaDoc, but not yet fixed, only marked as a ToDo of this issue.

          The fix is to add the documentpath to the given path before dispatching the call to the OdfPackage, which takes paths relative to the package root.

          Show
          Svante Schubert added a comment - Created an attachment (id=454) Methods now act as explained in JavaDoc The OdfPackagedocument insertDocument(String path) and removeDocument(String path) will now use a document path as parameter, which is relative to the document. This was already explained in the JavaDoc, but not yet fixed, only marked as a ToDo of this issue. The fix is to add the documentpath to the given path before dispatching the call to the OdfPackage, which takes paths relative to the package root.
          Hide
          Svante Schubert added a comment -

          Can you please review this next iteration of this issue and push it to the repository if you agree on it.

          Please do not close this issue, but reassign it to me in any case.

          Thanks in advance, Devin!
          Svante

          Show
          Svante Schubert added a comment - Can you please review this next iteration of this issue and push it to the repository if you agree on it. Please do not close this issue, but reassign it to me in any case. Thanks in advance, Devin! Svante
          Hide
          devin added a comment -

          Push and assign back to Svante. Thanks!
          Devin
          > Can you please review this next iteration of this issue and push it to the
          > repository if you agree on it.
          >
          > Please do not close this issue, but reassign it to me in any case.
          >
          > Thanks in advance, Devin!
          > Svante

          Show
          devin added a comment - Push and assign back to Svante. Thanks! Devin > Can you please review this next iteration of this issue and push it to the > repository if you agree on it. > > Please do not close this issue, but reassign it to me in any case. > > Thanks in advance, Devin! > Svante
          Mark Thomas made changes -
          Field Original Value New Value
          issue.field.bugzillaimportkey 219 12524078
          made changes -
          Link This issue blocks ODFTOOLKIT-43 [ ODFTOOLKIT-43 ]
          made changes -
          Link This issue depends on ODFTOOLKIT-99 [ ODFTOOLKIT-99 ]
          made changes -
          Link This issue depends on ODFTOOLKIT-110 [ ODFTOOLKIT-110 ]
          Mark Thomas made changes -
          Workflow jira [ 12634093 ] Default workflow, editable Closed status [ 12634299 ]
          Mark Thomas made changes -
          Workflow Default workflow, editable Closed status [ 12634299 ] jira [ 12634590 ]
          Gavin made changes -
          Link This issue blocks ODFTOOLKIT-43 [ ODFTOOLKIT-43 ]
          Gavin made changes -
          Link This issue is depended upon by ODFTOOLKIT-43 [ ODFTOOLKIT-43 ]
          Gavin made changes -
          Link This issue depends on ODFTOOLKIT-99 [ ODFTOOLKIT-99 ]
          Gavin made changes -
          Link This issue depends upon ODFTOOLKIT-99 [ ODFTOOLKIT-99 ]
          Gavin made changes -
          Link This issue depends on ODFTOOLKIT-110 [ ODFTOOLKIT-110 ]
          Gavin made changes -
          Link This issue depends upon ODFTOOLKIT-110 [ ODFTOOLKIT-110 ]

            People

            • Assignee:
              Svante Schubert
              Reporter:
              Svante Schubert
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development