commit fe246160603786a9d299755d88eabd3d7ce5439e
Author: Richard Heck <rgh...@lyx.org>
Date:   Sat Nov 4 21:23:25 2017 -0400

    Attempt to fix bug 9158 using updateBuffer.
    
    Along the lines suggested by JMarc, we now collect the list of bibfiles
    in use in the updateBuffer routines. This actually does simplify the code
    quite a bit. See the discussion there for reasons to go this way.
    
    (cherry picked from commit 8b9d1b860187338e06e10261b391886d50423239)
---
 src/Buffer.cpp              |  124 +++++++++++++++++++------------------------
 src/Buffer.h                |   23 ++------
 src/BufferView.cpp          |    8 +--
 src/insets/InsetBibtex.cpp  |   37 ++++---------
 src/insets/InsetBibtex.h    |    4 +-
 src/insets/InsetInclude.cpp |    7 ---
 src/insets/InsetInclude.h   |    8 ---
 7 files changed, 75 insertions(+), 136 deletions(-)

diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index 6e523d9..1597e89 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -289,8 +289,6 @@ public:
        mutable BiblioInfo bibinfo_;
        /// whether the bibinfo cache is valid
        mutable bool bibinfo_cache_valid_;
-       /// whether the bibfile cache is valid
-       mutable bool bibfile_cache_valid_;
        /// Cache of timestamps of .bib files
        map<FileName, time_t> bibfile_status_;
        /// Indicates whether the bibinfo has changed since the last time
@@ -343,10 +341,8 @@ public:
                if (!cloned_buffer_ && parent_buffer && pb)
                        LYXERR0("Warning: a buffer should not have two 
parents!");
                parent_buffer = pb;
-               if (!cloned_buffer_ && parent_buffer) {
-                       parent_buffer->invalidateBibfileCache();
+               if (!cloned_buffer_ && parent_buffer)
                        parent_buffer->invalidateBibinfoCache();
-               }
        }
 
        /// If non zero, this buffer is a clone of existing buffer \p 
cloned_buffer_
@@ -432,7 +428,7 @@ Buffer::Impl::Impl(Buffer * owner, FileName const & file, 
bool readonly_,
          file_fully_loaded(false), file_format(LYX_FORMAT), 
need_format_backup(false),
          ignore_parent(false),  toc_backend(owner), macro_lock(false),
          checksum_(0), wa_(0),  gui_(0), undo_(*owner), 
bibinfo_cache_valid_(false),
-         bibfile_cache_valid_(false), cite_labels_valid_(false), 
preview_error_(false),
+         cite_labels_valid_(false), preview_error_(false),
          inset(0), preview_loader_(0), cloned_buffer_(cloned_buffer),
          clone_list_(0), doing_export(false),
          tracked_changes_present_(0), externally_modified_(false), 
parent_buffer(0),
@@ -452,7 +448,6 @@ Buffer::Impl::Impl(Buffer * owner, FileName const & file, 
bool readonly_,
        bibfiles_cache_ = cloned_buffer_->d->bibfiles_cache_;
        bibinfo_ = cloned_buffer_->d->bibinfo_;
        bibinfo_cache_valid_ = cloned_buffer_->d->bibinfo_cache_valid_;
-       bibfile_cache_valid_ = cloned_buffer_->d->bibfile_cache_valid_;
        bibfile_status_ = cloned_buffer_->d->bibfile_status_;
        cite_labels_valid_ = cloned_buffer_->d->cite_labels_valid_;
        unnamed = cloned_buffer_->d->unnamed;
@@ -1904,7 +1899,7 @@ void Buffer::writeLaTeXSource(otexstream & os,
                // Biblatex bibliographies are loaded here
                if (params().useBiblatex()) {
                        vector<docstring> const bibfiles =
-                               prepareBibFilePaths(runparams, 
getBibfilesCache(), true);
+                               prepareBibFilePaths(runparams, getBibfiles(), 
true);
                        for (docstring const & file: bibfiles)
                                os << "\\addbibresource{" << file << "}\n";
                }
@@ -2322,47 +2317,11 @@ void Buffer::getLabelList(vector<docstring> & list) 
const
 }
 
 
-void Buffer::updateBibfilesCache(UpdateScope scope) const
-{
-       // FIXME This is probably unnecssary, given where we call this.
-       // If this is a child document, use the parent's cache instead.
-       if (parent() && scope != UpdateChildOnly) {
-               masterBuffer()->updateBibfilesCache();
-               return;
-       }
-
-       d->bibfiles_cache_.clear();
-       for (InsetIterator it = inset_iterator_begin(inset()); it; ++it) {
-               if (it->lyxCode() == BIBTEX_CODE) {
-                       InsetBibtex const & inset = static_cast<InsetBibtex 
const &>(*it);
-                       support::FileNamePairList const bibfiles = 
inset.getBibFiles();
-                       d->bibfiles_cache_.insert(d->bibfiles_cache_.end(),
-                               bibfiles.begin(),
-                               bibfiles.end());
-               } else if (it->lyxCode() == INCLUDE_CODE) {
-                       InsetInclude & inset = static_cast<InsetInclude &>(*it);
-                       Buffer const * const incbuf = inset.getChildBuffer();
-                       if (!incbuf)
-                               continue;
-                       support::FileNamePairList const & bibfiles =
-                                       
incbuf->getBibfilesCache(UpdateChildOnly);
-                       if (!bibfiles.empty()) {
-                               
d->bibfiles_cache_.insert(d->bibfiles_cache_.end(),
-                                       bibfiles.begin(),
-                                       bibfiles.end());
-                       }
-               }
-       }
-       d->bibfile_cache_valid_ = true;
-       d->bibinfo_cache_valid_ = false;
-       d->cite_labels_valid_ = false;
-}
-
-
 void Buffer::invalidateBibinfoCache() const
 {
        d->bibinfo_cache_valid_ = false;
        d->cite_labels_valid_ = false;
+       removeBiblioTempFiles();
        // also invalidate the cache for the parent buffer
        Buffer const * const pbuf = d->parent();
        if (pbuf)
@@ -2370,29 +2329,13 @@ void Buffer::invalidateBibinfoCache() const
 }
 
 
-void Buffer::invalidateBibfileCache() const
-{
-       d->bibfile_cache_valid_ = false;
-       d->bibinfo_cache_valid_ = false;
-       d->cite_labels_valid_ = false;
-       // also invalidate the cache for the parent buffer
-       Buffer const * const pbuf = d->parent();
-       if (pbuf)
-               pbuf->invalidateBibfileCache();
-}
-
-
-support::FileNamePairList const & Buffer::getBibfilesCache(UpdateScope scope) 
const
+FileNamePairList const & Buffer::getBibfiles(UpdateScope scope) const
 {
        // FIXME This is probably unnecessary, given where we call this.
-       // If this is a child document, use the master's cache instead.
+       // If this is a child document, use the master instead.
        Buffer const * const pbuf = masterBuffer();
        if (pbuf != this && scope != UpdateChildOnly)
-               return pbuf->getBibfilesCache();
-
-       if (!d->bibfile_cache_valid_)
-               this->updateBibfilesCache(scope);
-
+               return pbuf->getBibfiles();
        return d->bibfiles_cache_;
 }
 
@@ -2412,6 +2355,20 @@ BiblioInfo const & Buffer::bibInfo() const
 }
 
 
+void Buffer::registerBibfiles(FileNamePairList const & bf) const {
+       Buffer const * const tmp = masterBuffer();
+       if (tmp != this)
+               return tmp->registerBibfiles(bf);
+
+       for (auto const & p : bf) {
+               FileNamePairList::const_iterator tmp =
+                       find(d->bibfiles_cache_.begin(), 
d->bibfiles_cache_.end(), p);
+               if (tmp == d->bibfiles_cache_.end())
+                       d->bibfiles_cache_.push_back(p);
+       }
+}
+
+
 void Buffer::checkIfBibInfoCacheIsValid() const
 {
        // use the master's cache
@@ -2421,8 +2378,13 @@ void Buffer::checkIfBibInfoCacheIsValid() const
                return;
        }
 
+       // if we already know the cache is invalid, no need to check
+       // the timestamps
+       if (!d->bibinfo_cache_valid_)
+               return;
+
        // compare the cached timestamps with the actual ones.
-       FileNamePairList const & bibfiles_cache = getBibfilesCache();
+       FileNamePairList const & bibfiles_cache = getBibfiles();
        FileNamePairList::const_iterator ei = bibfiles_cache.begin();
        FileNamePairList::const_iterator en = bibfiles_cache.end();
        for (; ei != en; ++ ei) {
@@ -4753,10 +4715,16 @@ void Buffer::updateBuffer(UpdateScope scope, UpdateType 
utype) const
        Buffer const * const master = masterBuffer();
        DocumentClass const & textclass = master->params().documentClass();
 
+       FileNamePairList old_bibfiles;
        // do this only if we are the top-level Buffer
        if (master == this) {
                textclass.counters().reset(from_ascii("bibitem"));
                reloadBibInfoCache();
+               // we will re-read this cache as we go through, but we need
+               // to know whether it's changed to know whether we need to
+               // update the bibinfo cache.
+               old_bibfiles = d->bibfiles_cache_;
+               d->bibfiles_cache_.clear();
        }
 
        // keep the buffers to be children in this set. If the call from the
@@ -4802,14 +4770,30 @@ void Buffer::updateBuffer(UpdateScope scope, UpdateType 
utype) const
        ParIterator parit = cbuf.par_iterator_begin();
        updateBuffer(parit, utype);
 
+       // If this document has siblings, then update the TocBackend later. The
+       // reason is to ensure that later siblings are up to date when e.g. the
+       // broken or not status of references is computed. The update is called
+       // in InsetInclude::addToToc.
        if (master != this)
-               // If this document has siblings, then update the TocBackend 
later. The
-               // reason is to ensure that later siblings are up to date when 
e.g. the
-               // broken or not status of references is computed. The update 
is called
-               // in InsetInclude::addToToc.
                return;
 
-       d->bibinfo_cache_valid_ = true;
+       // if the bibfiles changed, the cache of bibinfo is invalid
+       sort(d->bibfiles_cache_.begin(), d->bibfiles_cache_.end());
+       // the old one should already be sorted
+       if (old_bibfiles != d->bibfiles_cache_) {
+               invalidateBibinfoCache();
+               reloadBibInfoCache();
+               // We relied upon the bibinfo cache when recalculating labels. 
But that
+               // cache was invalid, although we didn't find that out until 
now. So we
+               // have to do it all again.
+               // That said, the only thing we really need to do is update the 
citation
+               // labels. Nothing else will have changed. So we could create a 
new 
+               // UpdateType that would signal that fact, if we needed to do 
so.
+               parit = cbuf.par_iterator_begin();
+               updateBuffer(parit, utype);
+       }
+       else
+               d->bibinfo_cache_valid_ = true;
        d->cite_labels_valid_ = true;
        /// FIXME: Perf
        cbuf.tocBackend().update(true, utype);
diff --git a/src/Buffer.h b/src/Buffer.h
index ae1d670..0e01dbc 100644
--- a/src/Buffer.h
+++ b/src/Buffer.h
@@ -493,25 +493,13 @@ public:
        */
        void validate(LaTeXFeatures &) const;
 
-       /// Reference information is cached in the Buffer, so we do not
+       /// Bibliography information is cached in the Buffer, so we do not
        /// have to check or read things over and over.
-       ///
-       /// There are two caches.
-       ///
-       /// One is a cache of the BibTeX files from which reference info is
-       /// being gathered. This cache is PER BUFFER, and the cache for the
-       /// master essentially includes the cache for its children. This gets
-       /// invalidated when an InsetBibtex is created, deleted, or modified.
-       ///
-       /// The other is a cache of the reference information itself. This
-       /// exists only in the master buffer, and when it needs to be updated,
+       /// The cache exists only in the master buffer. When it is updated,
        /// the children add their information to the master's cache.
-
        /// Calling this method invalidates the cache and so requires a
        /// re-read.
        void invalidateBibinfoCache() const;
-       /// This invalidates the cache of files we need to check.
-       void invalidateBibfileCache() const;
        /// Updates the cached bibliography information, checking first to see
        /// whether the cache is valid. If so, we do nothing. If not, then we
        /// reload all the BibTeX info.
@@ -777,6 +765,8 @@ public:
        void setChangesPresent(bool) const;
        bool areChangesPresent() const;
        void updateChangesPresent() const;
+       ///
+       void registerBibfiles(support::FileNamePairList const & bf) const;
 
 private:
        friend class MarkAsExporting;
@@ -793,13 +783,10 @@ private:
        /// last time we loaded the cache. Note that this does NOT update the
        /// cached information.
        void checkIfBibInfoCacheIsValid() const;
-       /// Update the list of all bibfiles in use (including bibfiles
-       /// of loaded child documents).
-       void updateBibfilesCache(UpdateScope scope = UpdateMaster) const;
        /// Return the list with all bibfiles in use (including bibfiles
        /// of loaded child documents).
        support::FileNamePairList const &
-               getBibfilesCache(UpdateScope scope = UpdateMaster) const;
+               getBibfiles(UpdateScope scope = UpdateMaster) const;
        ///
        void collectChildren(ListOfBuffers & children, bool grand_children) 
const;
 
diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 7788f76..f1b5c37 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -1637,10 +1637,8 @@ void BufferView::dispatch(FuncRequest const & cmd, 
DispatchResult & dr)
                InsetBibtex * inset = getInsetByCode<InsetBibtex>(tmpcur,
                                                BIBTEX_CODE);
                if (inset) {
-                       if (inset->addDatabase(cmd.argument())) {
-                               buffer_.invalidateBibfileCache();
+                       if (inset->addDatabase(cmd.argument()))
                                dr.forceBufferUpdate();
-                       }
                }
                break;
        }
@@ -1651,10 +1649,8 @@ void BufferView::dispatch(FuncRequest const & cmd, 
DispatchResult & dr)
                InsetBibtex * inset = getInsetByCode<InsetBibtex>(tmpcur,
                                                BIBTEX_CODE);
                if (inset) {
-                       if (inset->delDatabase(cmd.argument())) {
-                               buffer_.invalidateBibfileCache();
+                       if (inset->delDatabase(cmd.argument()))
                                dr.forceBufferUpdate();
-                       }
                }
                break;
        }
diff --git a/src/insets/InsetBibtex.cpp b/src/insets/InsetBibtex.cpp
index 30b169a..4ecb522 100644
--- a/src/insets/InsetBibtex.cpp
+++ b/src/insets/InsetBibtex.cpp
@@ -59,22 +59,7 @@ namespace os = support::os;
 
 InsetBibtex::InsetBibtex(Buffer * buf, InsetCommandParams const & p)
        : InsetCommand(buf, p)
-{
-       buffer().invalidateBibfileCache();
-       buffer().removeBiblioTempFiles();
-}
-
-
-InsetBibtex::~InsetBibtex()
-{
-       if (isBufferLoaded()) {
-               /* We do not use buffer() because Coverity believes that this
-                * may throw an exception. Actually this code path is not
-                * taken when buffer_ == 0 */
-               buffer_-> invalidateBibfileCache();
-               buffer_->removeBiblioTempFiles();
-       }
-}
+{}
 
 
 ParamInfo const & InsetBibtex::findInfo(string const & /* cmdName */)
@@ -116,8 +101,6 @@ void InsetBibtex::doDispatch(Cursor & cur, FuncRequest & 
cmd)
 
                cur.recordUndo();
                setParams(p);
-               buffer().invalidateBibfileCache();
-               buffer().removeBiblioTempFiles();
                cur.forceBufferUpdate();
                break;
        }
@@ -378,15 +361,15 @@ void InsetBibtex::latex(otexstream & os, OutputParams 
const & runparams) const
 }
 
 
-support::FileNamePairList InsetBibtex::getBibFiles() const
+FileNamePairList InsetBibtex::getBibFiles() const
 {
        FileName path(buffer().filePath());
-       support::PathChanger p(path);
+       PathChanger p(path);
 
        // We need to store both the real FileName and the way it is entered
        // (with full path, rel path or as a single file name).
        // The latter is needed for biblatex's central bibfile macro.
-       support::FileNamePairList vec;
+       FileNamePairList vec;
 
        vector<docstring> bibfilelist = 
getVectorFromString(getParam("bibfiles"));
        vector<docstring>::const_iterator it = bibfilelist.begin();
@@ -401,7 +384,6 @@ support::FileNamePairList InsetBibtex::getBibFiles() const
        }
 
        return vec;
-
 }
 
 namespace {
@@ -674,9 +656,9 @@ void InsetBibtex::parseBibTeXFiles(FileNameList & 
checkedFiles) const
 
        BiblioInfo keylist;
 
-       support::FileNamePairList const files = getBibFiles();
-       support::FileNamePairList::const_iterator it = files.begin();
-       support::FileNamePairList::const_iterator en = files.end();
+       FileNamePairList const files = getBibFiles();
+       FileNamePairList::const_iterator it = files.begin();
+       FileNamePairList::const_iterator en = files.end();
        for (; it != en; ++ it) {
                FileName const bibfile = it->second;
                if (find(checkedFiles.begin(), checkedFiles.end(), bibfile) != 
checkedFiles.end())
@@ -912,6 +894,11 @@ void InsetBibtex::validate(LaTeXFeatures & features) const
 }
 
 
+void InsetBibtex::updateBuffer(ParIterator const &, UpdateType) {
+       buffer().registerBibfiles(getBibFiles());
+}
+
+
 int InsetBibtex::plaintext(odocstringstream & os,
        OutputParams const & op, size_t max_length) const
 {
diff --git a/src/insets/InsetBibtex.h b/src/insets/InsetBibtex.h
index 2d03346..882f0b8 100644
--- a/src/insets/InsetBibtex.h
+++ b/src/insets/InsetBibtex.h
@@ -29,8 +29,6 @@ class InsetBibtex : public InsetCommand {
 public:
        ///
        InsetBibtex(Buffer *, InsetCommandParams const &);
-       ///
-       ~InsetBibtex();
 
        ///
        support::FileNamePairList getBibFiles() const;
@@ -57,6 +55,8 @@ public:
        int plaintext(odocstringstream & ods, OutputParams const & op,
                      size_t max_length = INT_MAX) const;
        ///
+       void updateBuffer(ParIterator const &, UpdateType);
+       ///
        void collectBibKeys(InsetIterator const &, support::FileNameList &) 
const;
        ///
        void validate(LaTeXFeatures &) const;
diff --git a/src/insets/InsetInclude.cpp b/src/insets/InsetInclude.cpp
index 7b6f389..89f2533 100644
--- a/src/insets/InsetInclude.cpp
+++ b/src/insets/InsetInclude.cpp
@@ -197,11 +197,6 @@ InsetInclude::InsetInclude(InsetInclude const & other)
 
 InsetInclude::~InsetInclude()
 {
-       if (isBufferLoaded())
-               /* We do not use buffer() because Coverity believes that this
-                * may throw an exception. Actually this code path is not
-                * taken when buffer_ == 0 */
-               buffer_->invalidateBibfileCache();
        delete label_;
 }
 
@@ -354,8 +349,6 @@ void InsetInclude::setParams(InsetCommandParams const & p)
 
        if (type(params()) == INPUT)
                add_preview(*preview_, *this, buffer());
-
-       buffer().invalidateBibfileCache();
 }
 
 
diff --git a/src/insets/InsetInclude.h b/src/insets/InsetInclude.h
index 009b49e..18329fe 100644
--- a/src/insets/InsetInclude.h
+++ b/src/insets/InsetInclude.h
@@ -59,14 +59,6 @@ public:
         */
        void updateBibfilesCache();
 
-       /** Return the cache with all bibfiles in use of the child buffer
-        *  (including bibfiles of grandchild documents).
-        *  Return an empty vector if the child doc is not loaded.
-        *  \param buffer the Buffer containing this inset.
-        */
-       support::FileNameList const &
-               getBibfilesCache() const;
-
        ///
        void updateCommand();
        ///

Reply via email to