From 96e0bafddd2e5d4c891668de5a19b64a03aa1ee4 Mon Sep 17 00:00:00 2001 From: Baruch Even Date: Thu, 22 Feb 2001 16:53:59 +0000 Subject: [PATCH] Cleaned up the GraphicsCache mechanism, made it use shared_ptr for cleanliness Updated InsetGraphics to use the changed the GraphicsCache, and fixed a lurking bug in LyXImage. git-svn-id: svn://svn.lyx.org/lyx/lyx-devel/trunk@1606 a592a061-630c-0410-9148-cb99ea01b6c8 --- src/graphics/ChangeLog | 13 +++ src/graphics/GraphicsCache.C | 21 ++-- src/graphics/GraphicsCache.h | 9 +- src/graphics/GraphicsCacheItem.C | 135 ++++++++++++---------- src/graphics/GraphicsCacheItem.h | 48 +++----- src/graphics/GraphicsCacheItem_pimpl.C | 152 ------------------------- src/graphics/GraphicsCacheItem_pimpl.h | 85 -------------- src/graphics/ImageLoader.C | 14 ++- src/graphics/ImageLoader.h | 16 ++- src/graphics/Makefile.am | 2 - src/insets/ChangeLog | 5 + src/insets/insetgraphics.C | 18 ++- src/insets/insetgraphics.h | 3 +- 13 files changed, 159 insertions(+), 362 deletions(-) delete mode 100644 src/graphics/GraphicsCacheItem_pimpl.C delete mode 100644 src/graphics/GraphicsCacheItem_pimpl.h diff --git a/src/graphics/ChangeLog b/src/graphics/ChangeLog index 699d200b08..9cedf2bc33 100644 --- a/src/graphics/ChangeLog +++ b/src/graphics/ChangeLog @@ -1,3 +1,16 @@ +2001-02-20 Baruch Even + + * GraphicsCache.C: Changed to use shared_ptr + instead of a pure pointer. + + * GraphicsCacheItem.[Ch]: + * GraphicsCacheItem_pimpl.[Ch]: Collapsed them into GraphicsCacheItem, + removed the reference counting that was inside. Also fixed a bug where + a temporary file wouldn't get erased. + + * ImageLoader.[Ch]: Changed the semantics of the image_ pointers usage. + Ownership is now dropped when the caller requests the image_ pointer. + 2001-02-20 Baruch Even * GraphicsCache.C: Cleared up the confusion on when and how it is diff --git a/src/graphics/GraphicsCache.C b/src/graphics/GraphicsCache.C index a2956ddf6e..a2bed25973 100644 --- a/src/graphics/GraphicsCache.C +++ b/src/graphics/GraphicsCache.C @@ -16,6 +16,7 @@ #endif #include "GraphicsCache.h" +#include "GraphicsCacheItem.h" #include "support/LAssert.h" @@ -43,26 +44,24 @@ GraphicsCache::~GraphicsCache() } -GraphicsCacheItem * +GraphicsCache::shared_ptr_item GraphicsCache::addFile(string const & filename) { CacheType::iterator it = cache.find(filename); if (it != cache.end()) { - return new GraphicsCacheItem( *((*it).second) ); + return (*it).second; } - GraphicsCacheItem * cacheItem = new GraphicsCacheItem(); - if (cacheItem == 0) - return 0; - - cacheItem->setFilename(filename); + shared_ptr_item cacheItem(new GraphicsCacheItem(filename)); + if (cacheItem.get() == 0) + return cacheItem; cache[filename] = cacheItem; - - // We do not want to return the main cache object, otherwise when the - // will destroy their copy they will destroy the main copy. - return new GraphicsCacheItem( *cacheItem ); + + // GraphicsCacheItem_ptr is a shared_ptr and thus reference counted, + // it is safe to return it directly. + return cacheItem; } diff --git a/src/graphics/GraphicsCache.h b/src/graphics/GraphicsCache.h index b0c7e4f6e6..3f28d385cc 100644 --- a/src/graphics/GraphicsCache.h +++ b/src/graphics/GraphicsCache.h @@ -21,6 +21,9 @@ #include "LString.h" #include "GraphicsCacheItem.h" #include +#include + +class GraphicsCacheItem; /** GraphicsCache is the manager of the image cache. It is responsible of create the GraphicsCacheItem's and maintain them. @@ -33,8 +36,10 @@ public: /// Get the instance of the class. static GraphicsCache * getInstance(); + typedef boost::shared_ptr shared_ptr_item; + /// Add a file to the cache. - GraphicsCacheItem * addFile(string const & filename); + shared_ptr_item addFile(string const & filename); private: /// Remove a cache item if it's count has gone to zero. @@ -49,7 +54,7 @@ private: /// Holder of the single instance of the class. static GraphicsCache * singleton; /// - typedef std::map CacheType; + typedef std::map CacheType; /// CacheType cache; diff --git a/src/graphics/GraphicsCacheItem.C b/src/graphics/GraphicsCacheItem.C index be2f9bc1f2..7f1804aafb 100644 --- a/src/graphics/GraphicsCacheItem.C +++ b/src/graphics/GraphicsCacheItem.C @@ -17,87 +17,106 @@ #include "graphics/GraphicsCache.h" #include "graphics/GraphicsCacheItem.h" -#include "graphics/GraphicsCacheItem_pimpl.h" #include "frontends/support/LyXImage.h" +#include "graphics/ImageLoaderXPM.h" +#include "support/filetools.h" +#include "support/lyxlib.h" +#include "support/syscall.h" +#include "debug.h" -GraphicsCacheItem::GraphicsCacheItem() - : pimpl(new GraphicsCacheItem_pimpl) +using std::endl; + +GraphicsCacheItem::GraphicsCacheItem(string const & filename) + : imageStatus_(GraphicsCacheItem::Loading) { - pimpl->refCount = 1; + filename_ = filename; + + renderXPM(filename); + // For now we do it synchronously + imageConverted(0); } GraphicsCacheItem::~GraphicsCacheItem() -{ - destroy(); -} +{} -bool -GraphicsCacheItem::setFilename(string const & filename) -{ - filename_ = filename; - return pimpl->setFilename(filename); -} - +GraphicsCacheItem::ImageStatus +GraphicsCacheItem::getImageStatus() const { return imageStatus_; } -GraphicsCacheItem::GraphicsCacheItem(GraphicsCacheItem const & gci) - : pimpl(0) -{ - // copy will set the actual value of the pimpl. - copy(gci); -} -GraphicsCacheItem & -GraphicsCacheItem::operator=(GraphicsCacheItem const & gci) -{ - // Are we trying to copy the object onto itself. - if (this == &gci) - return *this; +LyXImage * +GraphicsCacheItem::getImage() const { return image_.get(); } - // Destroy old copy - destroy(); - // And then copy new object. - copy(gci); +void +GraphicsCacheItem::imageConverted(int retval) +{ + lyxerr << "imageConverted, retval=" << retval << endl; - return *this; -} + if (retval) { + lyxerr << "(GraphicsCacheItem::imageConverter) " + "Error converting image." << endl; + imageStatus_ = GraphicsCacheItem::ErrorConverting; + return; + } -GraphicsCacheItem * -GraphicsCacheItem::Clone() const -{ - return new GraphicsCacheItem(*this); + // Do the actual image loading from XPM to memory. + loadXPMImage(); } -void -GraphicsCacheItem::copy(GraphicsCacheItem const & gci) + +bool +GraphicsCacheItem::renderXPM(string const & filename) { - pimpl = gci.pimpl; - ++(pimpl->refCount); + // Create the command to do the conversion, this depends on ImageMagicks + // convert program. + string command = "convert "; + command += filename; + command += " XPM:"; + + // Take only the filename part of the file, without path or extension. + string temp = OnlyFilename(filename); + temp = ChangeExtension(filename, string()); + + // Add some stuff to have it a unique temp file. + // This tempfile is deleted in loadXPMImage after it is loaded to memory. + tempfile = lyx::tempName(string(), temp); + // Remove the temp file, we only want the name... + lyx::unlink(tempfile); + tempfile = ChangeExtension(tempfile, ".xpm"); + + command += tempfile; + + // Run the convertor. + lyxerr << "Launching convert to xpm, command=" << command << endl; + Systemcalls syscall; + syscall.startscript(Systemcalls::Wait, command); + + return true; } +// This function gets called from the callback after the image has been +// converted successfully. void -GraphicsCacheItem::destroy() +GraphicsCacheItem::loadXPMImage() { - if (!pimpl) - return; - - --(pimpl->refCount); - if (pimpl->refCount == 0) { - delete pimpl; - pimpl = 0; - - GraphicsCache * gc = GraphicsCache::getInstance(); - gc->removeFile(filename_); + lyxerr << "Loading XPM Image... "; + + ImageLoaderXPM imageLoader; + if (imageLoader.loadImage(tempfile) == ImageLoader::OK) { + lyxerr << "Success." << endl; + image_.reset(imageLoader.getImage()); + imageStatus_ = GraphicsCacheItem::Loaded; + } else { + lyxerr << "Fail." << endl; + imageStatus_ = GraphicsCacheItem::ErrorReading; } -} - -GraphicsCacheItem::ImageStatus -GraphicsCacheItem::getImageStatus() const { return pimpl->imageStatus_; } - -LyXImage * -GraphicsCacheItem::getImage() const { return pimpl->getImage(); } + // remove the xpm file now. + lyx::unlink(tempfile); + // and remove the reference to the filename. + tempfile = string(); +} diff --git a/src/graphics/GraphicsCacheItem.h b/src/graphics/GraphicsCacheItem.h index 52d710cc08..266b70d723 100644 --- a/src/graphics/GraphicsCacheItem.h +++ b/src/graphics/GraphicsCacheItem.h @@ -21,33 +21,24 @@ #include XPM_H_LOCATION #include "LString.h" +#include +#include + #include "sigc++/signal_system.h" #ifdef SIGC_CXX_NAMESPACES using SigC::Signal0; #endif -/* (Baruch Even 2000-08-05) - * This has a major drawback: it is only designed for X servers, no easy - * porting to non X-server based platform is offered right now, this is done - * in order to get a first version out of the door. - * - * Later versions should consider how to do this with more platform - * independence, this will probably involve changing the Painter class too. - */ - -class GraphicsCacheItem_pimpl; class LyXImage; /// A GraphicsCache item holder. -class GraphicsCacheItem { +class GraphicsCacheItem : public noncopyable { public: + /// c-tor + GraphicsCacheItem(string const & filename); /// d-tor, frees the image structures. ~GraphicsCacheItem(); - /// copy c-tor. - GraphicsCacheItem(GraphicsCacheItem const &); - /// Assignment operator. - GraphicsCacheItem & operator=(GraphicsCacheItem const &); /// Return a pixmap that can be displayed on X server. LyXImage * getImage() const; @@ -73,31 +64,20 @@ public: */ void imageConverted(int retval); - /// Create another copy of the object. - GraphicsCacheItem * Clone() const; - private: - /// Private c-tor so that only GraphicsCache can create an instance. - GraphicsCacheItem(); - - /// internal copy mechanism. - void copy(GraphicsCacheItem const &); - /// internal destroy mechanism. - void destroy(); - - /// Set the filename this item will be pointing too. - bool setFilename(string const & filename); - - /// - friend class GraphicsCache; - - /// - GraphicsCacheItem_pimpl * pimpl; + bool renderXPM(string const & filename); + void loadXPMImage(); /** The filename we refer too. This is used when removing ourselves from the cache. */ string filename_; + /// The temporary file that we use + string tempfile; + /// The image status + ImageStatus imageStatus_; + /// The image (if it got loaded) + boost::scoped_ptr image_; }; #endif diff --git a/src/graphics/GraphicsCacheItem_pimpl.C b/src/graphics/GraphicsCacheItem_pimpl.C deleted file mode 100644 index ccc7b39caf..0000000000 --- a/src/graphics/GraphicsCacheItem_pimpl.C +++ /dev/null @@ -1,152 +0,0 @@ -// -*- C++ -*- -/* This file is part of - * ================================================= - * - * LyX, The Document Processor - * Copyright 1995 Matthias Ettrich. - * Copyright 1995-2000 The LyX Team. - * - * This file Copyright 2000 Baruch Even - * ================================================= */ - -#include - -#include - -#include FORMS_H_LOCATION - -#ifdef __GNUG__ -#pragma implementation -#endif - -#include "GraphicsCacheItem.h" -#include "GraphicsCacheItem_pimpl.h" - -#include "frontends/support/LyXImage.h" -#include "ImageLoaderXPM.h" -#include "support/filetools.h" -#include "debug.h" -#include "support/LAssert.h" - -using std::endl; -using std::map; - - -GraphicsCacheItem_pimpl::GraphicsCacheItem_pimpl() - : imageStatus_(GraphicsCacheItem::Loading), - image_(0), imageLoader(0), refCount(0) -{} - - -GraphicsCacheItem_pimpl::~GraphicsCacheItem_pimpl() -{ - delete image_; image_ = 0; - delete imageLoader; imageLoader = 0; -} - - -bool -GraphicsCacheItem_pimpl::setFilename(string const & filename) -{ - imageLoader = new ImageLoaderXPM(); - imageStatus_ = GraphicsCacheItem::Loading; - - if (renderXPM(filename)) - return true; - - return false; -} - - -/*** Callback method ***/ - -typedef map CallbackMap; -static CallbackMap callbackMap; - - -static -void callback(string cmd, int retval) -{ - lyxerr << "callback, cmd=" << cmd << ", retval=" << retval << endl; - - GraphicsCacheItem_pimpl * item = callbackMap[cmd]; - callbackMap.erase(cmd); - - item->imageConverted(retval); -} - - -void -GraphicsCacheItem_pimpl::imageConverted(int retval) -{ - lyxerr << "imageConverted, retval=" << retval << endl; - - if (retval) { - lyxerr << "(GraphicsCacheItem_pimpl::imageConverter) " - "Error converting image." << endl; - imageStatus_ = GraphicsCacheItem::ErrorConverting; - return; - } - - // Do the actual image loading from XPM to memory. - loadXPMImage(); -} - -/**********************/ - -bool -GraphicsCacheItem_pimpl::renderXPM(string const & filename) -{ - // Create the command to do the conversion, this depends on ImageMagicks - // convert program. - string command = "convert "; - command += filename; - command += " XPM:"; - - // Take only the filename part of the file, without path or extension. - string temp = OnlyFilename(filename); - temp = ChangeExtension(filename, string()); - - // Add some stuff to have it a unique temp file. - // This tempfile is deleted in loadXPMImage after it is loaded to memory. - xpmfile = lyx::tempName(string(), temp); - xpmfile = ChangeExtension(xpmfile, ".xpm"); - - command += xpmfile; - - // Set the callback mapping to point to us. - callbackMap[command] = this; - - // Run the convertor. - // There is a problem with running it asyncronously, it doesn't return - // to call the callback, so until the Systemcalls mechanism is fixed - // I use the syncronous method. - lyxerr << "Launching convert to xpm, command=" << command << endl; -// syscall.startscript(Systemcalls::DontWait, command, &callback); - syscall.startscript(Systemcalls::Wait, command, &callback); - - return true; -} - - -// This function gets called from the callback after the image has been -// converted successfully. -void -GraphicsCacheItem_pimpl::loadXPMImage() -{ - lyxerr << "Loading XPM Image... "; - - if (imageLoader->loadImage(xpmfile) == ImageLoader::OK) { - lyxerr << "Success." << endl; - image_ = imageLoader->getImage(); - imageStatus_ = GraphicsCacheItem::Loaded; - } else { - lyxerr << "Fail." << endl; - imageStatus_ = GraphicsCacheItem::ErrorReading; - } - - // remove the xpm file now. - lyx::unlink(xpmfile); - // and remove the reference to the filename. - xpmfile = string(); -} diff --git a/src/graphics/GraphicsCacheItem_pimpl.h b/src/graphics/GraphicsCacheItem_pimpl.h deleted file mode 100644 index 84098bccf5..0000000000 --- a/src/graphics/GraphicsCacheItem_pimpl.h +++ /dev/null @@ -1,85 +0,0 @@ -// -*- C++ -*- -/* This file is part of - * ================================================= - * - * LyX, The Document Processor - * Copyright 1995 Matthias Ettrich. - * Copyright 1995-2000 The LyX Team. - * - * This file Copyright 2000 Baruch Even - * ================================================= */ - -#ifndef GRAPHICSCACHEITEM_PIMPL_H -#define GRAPHICSCACHEITEM_PIMPL_H - -#include - -#ifdef __GNUG__ -#pragma interface -#endif - -#include "graphics/GraphicsCacheItem.h" - -#include XPM_H_LOCATION -#include "LString.h" -#include "graphics/ImageLoader.h" -#include "support/syscall.h" - -#include "sigc++/signal_system.h" -#ifdef SIGC_CXX_NAMESPACES -using SigC::Signal0; -#endif - -class LyXImage; - -/// A GraphicsCache item holder. -class GraphicsCacheItem_pimpl { -public: - /// d-tor, frees the image structures. - ~GraphicsCacheItem_pimpl(); - - /// Return a pixmap that can be displayed on X server. - LyXImage * getImage() const { return image_; }; - - typedef GraphicsCacheItem::ImageStatus ImageStatus; - - /// Is the pixmap ready for display? - ImageStatus getImageStatus() const; - - /** Get a notification when the image conversion is done. - used by an internal callback mechanism. */ - void imageConverted(int retval); - -private: - /// Private c-tor so that only GraphicsCache can create an instance. - GraphicsCacheItem_pimpl(); - - /// Set the filename this item will be pointing too. - bool setFilename(string const & filename); - - /// Create an XPM file version of the image. - bool renderXPM(string const & filename); - - /// Load the image from XPM to memory Pixmap - void loadXPMImage(); - - /// - friend class GraphicsCacheItem; - - /// The file name of the XPM file. - string xpmfile; - /// Is the pixmap loaded? - ImageStatus imageStatus_; - /// The image pixmap - LyXImage * image_; - /// The rendering object. - ImageLoader * imageLoader; - - /// The system caller, runs the convertor. - Systemcalls syscall; - - /// The reference count - int refCount; -}; - -#endif diff --git a/src/graphics/ImageLoader.C b/src/graphics/ImageLoader.C index 74710b452b..e164fd8e7b 100644 --- a/src/graphics/ImageLoader.C +++ b/src/graphics/ImageLoader.C @@ -30,6 +30,13 @@ ImageLoader::~ImageLoader() freeImage(); } +void +ImageLoader::freeImage() +{ + delete image_; + image_ = 0; +} + bool ImageLoader::isImageFormatOK(string const & /*filename*/) const { return false; @@ -37,15 +44,14 @@ bool ImageLoader::isImageFormatOK(string const & /*filename*/) const void ImageLoader::setImage(LyXImage * image) { - freeImage(); - image_ = image; } -void ImageLoader::freeImage() +LyXImage * ImageLoader::getImage() { - delete image_; + LyXImage * tmp = image_; image_ = 0; + return tmp; } ImageLoader::FormatList const diff --git a/src/graphics/ImageLoader.h b/src/graphics/ImageLoader.h index 4a1a4dc379..3a45f1269f 100644 --- a/src/graphics/ImageLoader.h +++ b/src/graphics/ImageLoader.h @@ -50,8 +50,17 @@ public: /// Start loading the image file. ImageLoader::Result loadImage(string const & filename); - /// Get the last rendered pixmap. Returns 0 if no image is ready. - LyXImage * getImage() const { return image_; }; + /** Get the last rendered pixmap. Returns 0 if no image is ready. + * + * It is a one time operation, that is, after you get the image + * you are completely responsible to destroy it and the ImageLoader + * will not know about the image. + * + * This way we avoid deleting the image if you still use it and the + * ImageLoader is destructed, and if you don't use it we get to + * destruct the image to avoid memory leaks. + */ + LyXImage * getImage(); /// Return the list of loadable formats. virtual FormatList const loadableFormats() const; @@ -70,7 +79,8 @@ private: /// Free the loaded image. void freeImage(); - /// The loaded image. + /// The loaded image. An auto_ptr would be great here, but it's not + /// available everywhere (gcc 2.95.2 doesnt have it). LyXImage * image_; }; diff --git a/src/graphics/Makefile.am b/src/graphics/Makefile.am index 6ac35a3759..ce0b4c0b64 100644 --- a/src/graphics/Makefile.am +++ b/src/graphics/Makefile.am @@ -12,8 +12,6 @@ libgraphics_la_SOURCES = \ GraphicsCache.C \ GraphicsCacheItem.h \ GraphicsCacheItem.C \ - GraphicsCacheItem_pimpl.h \ - GraphicsCacheItem_pimpl.C \ ImageLoaderXPM.h \ ImageLoaderXPM.C \ ImageLoader.h \ diff --git a/src/insets/ChangeLog b/src/insets/ChangeLog index 54ad06e408..99a65a6112 100644 --- a/src/insets/ChangeLog +++ b/src/insets/ChangeLog @@ -1,3 +1,8 @@ +2001-02-21 Baruch Even + + * insetgraphics.[Ch]: Changed to use boost::shared_ptr + instead of GraphicsCacheItem *. + 2001-02-22 Jean-Marc Lasgouttes * insetcollapsable.C (getLyXText): add const qualifier to second diff --git a/src/insets/insetgraphics.C b/src/insets/insetgraphics.C index 4f89710b3e..5ea6f7c085 100644 --- a/src/insets/insetgraphics.C +++ b/src/insets/insetgraphics.C @@ -181,7 +181,7 @@ InsetGraphics::statusMessage() const { char const * msg = 0; - if (cacheHandle) { + if (cacheHandle.get()) { switch (cacheHandle->getImageStatus()) { case GraphicsCacheItem::UnknownError: msg = _("Unknown Error"); @@ -211,7 +211,7 @@ InsetGraphics::statusMessage() const int InsetGraphics::ascent(BufferView *, LyXFont const &) const { LyXImage * pixmap = 0; - if (cacheHandle && (pixmap = cacheHandle->getImage())) + if (cacheHandle.get() && (pixmap = cacheHandle->getImage())) return pixmap->getHeight(); else return 50; @@ -229,7 +229,7 @@ int InsetGraphics::width(BufferView *, LyXFont const & font) const { LyXImage * pixmap = 0; - if (cacheHandle && (pixmap = cacheHandle->getImage())) + if (cacheHandle.get() && (pixmap = cacheHandle->getImage())) return pixmap->getWidth(); else { char const * msg = statusMessage(); @@ -266,7 +266,7 @@ void InsetGraphics::draw(BufferView * bv, LyXFont const & font, // Get the image status, default to unknown error. GraphicsCacheItem::ImageStatus status = GraphicsCacheItem::UnknownError; - if (cacheHandle) + if (cacheHandle.get()) status = cacheHandle->getImageStatus(); // Check if the image is now ready. @@ -576,8 +576,10 @@ void InsetGraphics::Validate(LaTeXFeatures & features) const void InsetGraphics::updateInset() const { GraphicsCache * gc = GraphicsCache::getInstance(); - GraphicsCacheItem * temp = 0; + boost::shared_ptr temp(0); + // We do it this way so that in the face of some error, we will still + // be in a valid state. if (!params.filename.empty()) { temp = gc->addFile(params.filename); } @@ -585,7 +587,6 @@ void InsetGraphics::updateInset() const // Mark the image as unloaded so that it gets updated. imageLoaded = false; - delete cacheHandle; cacheHandle = temp; } @@ -614,10 +615,7 @@ Inset * InsetGraphics::Clone(Buffer const &) const { InsetGraphics * newInset = new InsetGraphics; - if (cacheHandle) - newInset->cacheHandle = cacheHandle->Clone(); - else - newInset->cacheHandle = 0; + newInset->cacheHandle = cacheHandle; newInset->imageLoaded = imageLoaded; newInset->setParams(getParams()); diff --git a/src/insets/insetgraphics.h b/src/insets/insetgraphics.h index a481b83f79..5cb82a91de 100644 --- a/src/insets/insetgraphics.h +++ b/src/insets/insetgraphics.h @@ -20,6 +20,7 @@ #include "insets/lyxinset.h" #include "insets/insetgraphicsParams.h" #include "graphics/GraphicsCacheItem.h" +#include #include "LaTeXFeatures.h" @@ -113,7 +114,7 @@ private: string const prepareFile(Buffer const * buf) const; /// The graphics cache handle. - mutable GraphicsCacheItem * cacheHandle; + mutable boost::shared_ptr cacheHandle; /// is the pixmap initialized? mutable bool imageLoaded; -- 2.39.5