Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: odfdom-0.7.5
    • Fix Version/s: None
    • Component/s: java
    • Labels:
      None
    • Environment:
      Operating System: Windows
      Platform: PC

      Description

      Created an attachment (id=112)
      Image convenient APIs

      This patch is for image convenient APIs. We add some functions in OdfDocument and ImageHandler to implement the image convenient APIs :

      org.odftoolkit.odfdom.doc.OdfDocument
      //return the image handler instance of this document
      public ImageHandler getImageContainer()

      //The ImageContainer interface is used to access a image object or a collection of image objects in some image container
      org.odftoolkit.odfdom.doc.draw.ImageHandler

      //insert an Image from the specified uri to the OdfDocument,
      public String createImage(URI imageUri)

      //The method returns the specific one or more images by image path since the image may be inserted to the document several times.
      public List<OdfDrawImage> getImageByPath(String imagePath)

      //The method deletes one or more images from image container by image path
      public void deleteImageByPath(String imagePath)

      //The method deletes the specified image from image container
      public void deleteImage(OdfDrawImage image)

      //get the count of image objects in the image container
      public int getImageCount()

      //the method return the image list in the image container
      public List<OdfDrawImage> getImages()

      //the method return the set of all the image paths
      public Set<String> getImagePathSet()

        Activity

        Hide
        DaLi Liu added a comment -

        Verified status of this issue
        Base on devin's comment this issue maybe can be closed;

        Show
        DaLi Liu added a comment - Verified status of this issue Base on devin's comment this issue maybe can be closed;
        Hide
        devin added a comment -

        Simple ODF has supply this feature in version 0.5.5.
        More information please reference:
        (1) Simple ODF has supply this feature in version 0.5.5.
        More information please reference:
        (1) Bug 333 Provide convenient methods for image and text span.
        (2) Simple 0.5.5 release notes: http://odftoolkit.org/projects/simple/pages/ReleaseNotes Provide convenient methods for image and text span.
        (2) Simple 0.5.5 release notes: http://odftoolkit.org/projects/simple/pages/ReleaseNotes

        Show
        devin added a comment - Simple ODF has supply this feature in version 0.5.5. More information please reference: (1) Simple ODF has supply this feature in version 0.5.5. More information please reference: (1) Bug 333 Provide convenient methods for image and text span. (2) Simple 0.5.5 release notes: http://odftoolkit.org/projects/simple/pages/ReleaseNotes Provide convenient methods for image and text span. (2) Simple 0.5.5 release notes: http://odftoolkit.org/projects/simple/pages/ReleaseNotes
        Hide
        Svante Schubert added a comment -

        Again, you might consider this to be fixed in the Simple API project first, as ODFDOM will stabalize lower layers first.

        Show
        Svante Schubert added a comment - Again, you might consider this to be fixed in the Simple API project first, as ODFDOM will stabalize lower layers first.
        Hide
        Ying Chun Guo added a comment -

        The patch has been pushed to the main repository from 0.7.5, but the structure is not the new one: no inheritance relationship between DOM and DOC layer. So this issue is still kept open and reset assignee to default.

        Volunteers to refactor these API to the new structure (refer to http://odftoolkit.org/bugzilla/show_bug.cgi?id=69) are welcomed.

        Show
        Ying Chun Guo added a comment - The patch has been pushed to the main repository from 0.7.5, but the structure is not the new one: no inheritance relationship between DOM and DOC layer. So this issue is still kept open and reset assignee to default. Volunteers to refactor these API to the new structure (refer to http://odftoolkit.org/bugzilla/show_bug.cgi?id=69 ) are welcomed.
        Hide
        Svante Schubert added a comment -

        Created an attachment (id=140)
        Move convenient API from OdfDocument to OdfDrawImage

        Tony, of course it looks fine now (in this patch) to put half the dozen of image function to the document, but we going to have similar functions for every other visible ODF feature as well and that it becomes a little crowded on the document.

        We might put later a generic method to delete a feature, that has a path in the document (e.g. Image, Video, OLE object, etc.) but still the path is seen to be an implementation detail from some people.

        For now I moved the helper functions to the OdfDrawImage as static functions having a document as first parameter.

        I renamed the usability function insertImage, to newImage as there is no existing Image object being inserted. You might realize that we have two similar newImage functions on OdfDrawImage and OdfDocument. The latter seems the right place, but we might fix this later, when we move XML details to the DOM layer and handle have only one OdfImage in the convenient layer, taking care of <draw:frame> and <draw:image> from the DOM layer.

        Please review and submit the patch!
        Thanks in advance,
        Svante

        Show
        Svante Schubert added a comment - Created an attachment (id=140) Move convenient API from OdfDocument to OdfDrawImage Tony, of course it looks fine now (in this patch) to put half the dozen of image function to the document, but we going to have similar functions for every other visible ODF feature as well and that it becomes a little crowded on the document. We might put later a generic method to delete a feature, that has a path in the document (e.g. Image, Video, OLE object, etc.) but still the path is seen to be an implementation detail from some people. For now I moved the helper functions to the OdfDrawImage as static functions having a document as first parameter. I renamed the usability function insertImage, to newImage as there is no existing Image object being inserted. You might realize that we have two similar newImage functions on OdfDrawImage and OdfDocument. The latter seems the right place, but we might fix this later, when we move XML details to the DOM layer and handle have only one OdfImage in the convenient layer, taking care of <draw:frame> and <draw:image> from the DOM layer. Please review and submit the patch! Thanks in advance, Svante
        Hide
        Svante Schubert added a comment -

        This one I will return...but I will provide a patch, you might add if it suits you.

        I did the changes as suggested in #6.

        One further comment a newImage in OdfDocument makes no sense, if the next thing you do in the method is a switch based on the type of the document.
        It seems more elegant to move the newImage directly into the adequate document types, e.g. OdfSpreadsheet.

        Show
        Svante Schubert added a comment - This one I will return...but I will provide a patch, you might add if it suits you. I did the changes as suggested in #6. One further comment a newImage in OdfDocument makes no sense, if the next thing you do in the method is a switch based on the type of the document. It seems more elegant to move the newImage directly into the adequate document types, e.g. OdfSpreadsheet.
        Hide
        tony wei added a comment -

        Created an attachment (id=138)
        Fixed JavaDoc

        There are no argument on this issue. I very agree your opinion on that. A strong building need each correct position bricks. The tip opening warning switch is a useful suggestion to fix these issue. Thanks

        Show
        tony wei added a comment - Created an attachment (id=138) Fixed JavaDoc There are no argument on this issue. I very agree your opinion on that. A strong building need each correct position bricks. The tip opening warning switch is a useful suggestion to fix these issue. Thanks
        Hide
        Svante Schubert added a comment -

        Just minor problems, we are close to a release of the patch...

        Still some JavaDoc problems here, please enable your IDE warning.

        PS: You might think it is a little too restrict, but when shall we fix those JavaDoc problems in the future?
        The good part, when you think I am too strict is, you may turn the same argument now on me

        Show
        Svante Schubert added a comment - Just minor problems, we are close to a release of the patch... Still some JavaDoc problems here, please enable your IDE warning. PS: You might think it is a little too restrict, but when shall we fix those JavaDoc problems in the future? The good part, when you think I am too strict is, you may turn the same argument now on me
        Hide
        tony wei added a comment -

        Created an attachment (id=135)
        Refine the JavaDoc

        Refine the javadoc and change the retrun value is not null.

        Show
        tony wei added a comment - Created an attachment (id=135) Refine the JavaDoc Refine the javadoc and change the retrun value is not null.
        Hide
        Svante Schubert added a comment -

        Thanks for the single patch, it makes the review with 'hg import --no-commit <PATCH>' much easier!

        Noticed in the patch, that you have moved the insertion of images to the root document.
        If you do so, don't simply comment the method out, but removed it. Keep the code tidy.
        But I am uncertain if this movement is correct.
        It would be, when an image exists on root level for all documents, but as it does not work for formula.
        Furthermore we might want to place only feature manipulation functions to Odf*Document.
        All the image function could as well being place at the OdfImage class, as otherwise Odf*Document get to crowded with common usability functions.
        An object oriented approach keeps the functions to their objects, that might be the reasons why I would instinctivly look for such a functionality at the Image class.
        Please add to the JavaDoc that the function public List<OdfDrawImage> getImageByPath(String imagePath)

        { may return NULL instead of an empty list, to prevent null pointer exception of the users. Similar missing JavaDoc explaination of -1 for getImageCount(). Strange that public List<OdfDrawImage> getImageByPath(String imagePath) returns null when an exception occurs public List<OdfDrawImage> getImages() returns the empty List. I would suggest to return always a list, instead of null, but enhance the logging. Perhaps we might use logging to give out the message and the stacktrace? }

        catch (Exception ex)

        { ex.printStackTrace(); }
        Show
        Svante Schubert added a comment - Thanks for the single patch, it makes the review with 'hg import --no-commit <PATCH>' much easier! Noticed in the patch, that you have moved the insertion of images to the root document. If you do so, don't simply comment the method out, but removed it. Keep the code tidy. But I am uncertain if this movement is correct. It would be, when an image exists on root level for all documents, but as it does not work for formula. Furthermore we might want to place only feature manipulation functions to Odf*Document. All the image function could as well being place at the OdfImage class, as otherwise Odf*Document get to crowded with common usability functions. An object oriented approach keeps the functions to their objects, that might be the reasons why I would instinctivly look for such a functionality at the Image class. Please add to the JavaDoc that the function public List<OdfDrawImage> getImageByPath(String imagePath) { may return NULL instead of an empty list, to prevent null pointer exception of the users. Similar missing JavaDoc explaination of -1 for getImageCount(). Strange that public List<OdfDrawImage> getImageByPath(String imagePath) returns null when an exception occurs public List<OdfDrawImage> getImages() returns the empty List. I would suggest to return always a list, instead of null, but enhance the logging. Perhaps we might use logging to give out the message and the stacktrace? } catch (Exception ex) { ex.printStackTrace(); }
        Hide
        Svante Schubert added a comment -

        (From update of attachment 133)
        Change MIME type to patch to trigger download menu in my browser instead the text editor..

        Show
        Svante Schubert added a comment - (From update of attachment 133) Change MIME type to patch to trigger download menu in my browser instead the text editor..
        Hide
        tony wei added a comment -

        Created an attachment (id=133)
        Merge all Image changes to one

        I export a new patch and import it with last version in repository. Seems ok now. Please try again.

        Show
        tony wei added a comment - Created an attachment (id=133) Merge all Image changes to one I export a new patch and import it with last version in repository. Seems ok now. Please try again.
        Hide
        Svante Schubert added a comment -

        Tony, I have problems to apply both patches with --no-commit.
        Adding both into a repository and creating a new one, and import this did not work for me.
        Could you take a look into it and create a single patch based on both above?

        Thanks in advance,
        Svante

        Show
        Svante Schubert added a comment - Tony, I have problems to apply both patches with --no-commit. Adding both into a repository and creating a new one, and import this did not work for me. Could you take a look into it and create a single patch based on both above? Thanks in advance, Svante
        Hide
        tony wei added a comment -

        Created an attachment (id=123)
        Remove the ImageHandler

        We get rid of ImageHandler and move its methods to OdfDocument.

        Show
        tony wei added a comment - Created an attachment (id=123) Remove the ImageHandler We get rid of ImageHandler and move its methods to OdfDocument.
        Hide
        tony wei added a comment -

        For conveniently review this pack, I extract the comments given by Has-Peter and Svante about this subject. We will follow these comments to revise it. Also hope give more suggestion before push to repository.

        hason peter#

        • Image API:
          We already have OdfDrawImage->insertImage which is used by
          OdfTextDocument->newImage. So I think we should merge these with your new
          functionality:
          I'm all for replacing OdfTextDocument->newImage by a document type independent
          approach like your proposed ImageHandler. How about using
          OdfDrawImage->insertImage in ImageHandler?

        3) Singleton: ChartHandler and ImageHandler have to be singletons because of
        their internal HashMap of Chart-Names->Charts / List of Images. You cannot use
        two ChartHandlers for the same document without unwanted side effects. So you
        introduce OdfDocument->getChartHandler and OdfDocument->getImageContainer which
        make sure each Handler is a Singleton. However the handler's constructors are
        public so a user still can do mySecondCharthandler = new ChartHandler(myDoc).

        6) create... methods: IMO odfdom convention is new...

        Svante Schubert#

        • ImageHandler is being received by using getImageContainer (aside of using the
          same class name for the method, i.e. getImageHandler)
          I would ask myself if the methods should not be added to an OdfImage class,
          which represents an image ODF (using draw:frame and draw:image).
          How many Handler will be there in the end? Are Chart and Image part of every
          ODF MIME type? What about an formular document? Was this tested?

        Svante Schubert
        Furthermore, my current idea for placing feature dependent methods is still on
        the feature. For example, all methods referring to OdfImage (the image
        feature), instead moving a part to a handler, some to an image.
        The reason is that I (as one example user), would search at the image for image
        related methods at the first place. But I will take a closer look at this, too.

        Show
        tony wei added a comment - For conveniently review this pack, I extract the comments given by Has-Peter and Svante about this subject. We will follow these comments to revise it. Also hope give more suggestion before push to repository. hason peter# Image API: We already have OdfDrawImage->insertImage which is used by OdfTextDocument->newImage. So I think we should merge these with your new functionality: I'm all for replacing OdfTextDocument->newImage by a document type independent approach like your proposed ImageHandler. How about using OdfDrawImage->insertImage in ImageHandler? 3) Singleton: ChartHandler and ImageHandler have to be singletons because of their internal HashMap of Chart-Names->Charts / List of Images. You cannot use two ChartHandlers for the same document without unwanted side effects. So you introduce OdfDocument->getChartHandler and OdfDocument->getImageContainer which make sure each Handler is a Singleton. However the handler's constructors are public so a user still can do mySecondCharthandler = new ChartHandler(myDoc). 6) create... methods: IMO odfdom convention is new... Svante Schubert# ImageHandler is being received by using getImageContainer (aside of using the same class name for the method, i.e. getImageHandler) I would ask myself if the methods should not be added to an OdfImage class, which represents an image ODF (using draw:frame and draw:image). How many Handler will be there in the end? Are Chart and Image part of every ODF MIME type? What about an formular document? Was this tested? Svante Schubert Furthermore, my current idea for placing feature dependent methods is still on the feature. For example, all methods referring to OdfImage (the image feature), instead moving a part to a handler, some to an image. The reason is that I (as one example user), would search at the image for image related methods at the first place. But I will take a closer look at this, too.

          People

          • Assignee:
            Ying Chun Guo
            Reporter:
            tony wei
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:

              Development