From 8b9d1b860187338e06e10261b391886d50423239 Mon Sep 17 00:00:00 2001 From: Richard Heck Date: Sat, 4 Nov 2017 21:23:25 -0400 Subject: [PATCH] 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. --- 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 c6546508d2..a85555c169 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 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; @@ -1892,7 +1887,7 @@ void Buffer::writeLaTeXSource(otexstream & os, // Biblatex bibliographies are loaded here if (params().useBiblatex()) { vector const bibfiles = - prepareBibFilePaths(runparams, getBibfilesCache(), true); + prepareBibFilePaths(runparams, getBibfiles(), true); for (docstring const & file: bibfiles) os << "\\addbibresource{" << file << "}\n"; } @@ -2306,47 +2301,11 @@ void Buffer::getLabelList(vector & 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(*it); - 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(*it); - Buffer const * const incbuf = inset.getChildBuffer(); - if (!incbuf) - continue; - 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) @@ -2354,29 +2313,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(); -} - - -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_; } @@ -2390,6 +2333,20 @@ BiblioInfo const & Buffer::masterBibInfo() 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 @@ -2399,8 +2356,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) { @@ -4662,10 +4624,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 @@ -4711,14 +4679,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 0bfe18f40f..96fd8e25f8 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. @@ -771,6 +759,8 @@ public: void setChangesPresent(bool) const; bool areChangesPresent() const; void updateChangesPresent() const; + /// + void registerBibfiles(support::FileNamePairList const & bf) const; private: friend class MarkAsExporting; @@ -787,13 +777,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 4e2428d7d7..7828be1d3d 100644 --- a/src/BufferView.cpp +++ b/src/BufferView.cpp @@ -1633,10 +1633,8 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr) InsetBibtex * inset = getInsetByCode(tmpcur, BIBTEX_CODE); if (inset) { - if (inset->addDatabase(cmd.argument())) { - buffer_.invalidateBibfileCache(); + if (inset->addDatabase(cmd.argument())) dr.forceBufferUpdate(); - } } break; } @@ -1647,10 +1645,8 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr) InsetBibtex * inset = getInsetByCode(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 30b169acb0..4ecb522737 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 bibfilelist = getVectorFromString(getParam("bibfiles")); vector::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 2d033464f0..882f0b8460 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 d6e3ae2c63..87488f0e6a 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 009b49e3ac..18329fe6d9 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(); /// -- 2.39.5