Version: 1.2.1 (using KDE KDE 3.5.0) Installed from: Compiled From Sources OS: Linux PROBLEM: For various data objects (e.g. histogram, power spectrum, equation, etc.) it is possible to create such an object and then edit it to select its own slace vector (i.e. its output) as its input. This is not desirable as it will cause an update flood. STEPS TO REPRODUCE: Start Kst Create a vector Create a histogram from the vector Edit the histogram and selects its own -sv vector as its input RESULTS: This is permissible and results in a flood of updates EXPECTED RESULTS: The objects own output should not be found in the list of allowable input vectors
The solution would be to explicitly remove from the input vector combobox all the existing output vectors. At the same time we should also remove any vector that has a dependency on one or more of the output vectors.
This is a very ugly path to go down. We'll be stuffing hacks in every corner of Kst well into the future. This is a long-known problem and we need a better solution if we want to try to protect users from themselves. For now, it's I/O error.
More to the point, it only prevents child recursion - there could still easily be recursion which uses a grandchild. I think the proper solution will have to be a recursion catcher in the update loop - nothing else is general.
Do we even need it in the update loop? Maybe whenever docModified happens, we can run a cycle detection algorithm to predict problems. Will have to check to see how KstScript behaves with this. Also the algorithm should probably try to use a visitor instead of adding more junk into the objects themselves.
SVN commit 663644 by arwalker: CCBUG:129068 Add code for recursion detection M +51 -3 kstdataobject.cpp M +2 -1 kstdataobject.h --- branches/work/kst/1.5/kst/src/libkstmath/kstdataobject.cpp #663643:663644 @@ -723,11 +723,11 @@ for (; scalarDictIter.current(); ++scalarDictIter) { if (scalarDictIter.current() == k.data()) { return true; - } + } } } } - + for (KstScalarMap::Iterator j = obj->outputScalars().begin(); j != obj->outputScalars().end(); ++j) { for (KstScalarMap::ConstIterator k = _inputScalars.begin(); k != _inputScalars.end(); ++k) { if (j.data() == k.data()) { @@ -735,7 +735,7 @@ } } } - + for (KstStringMap::Iterator j = obj->outputStrings().begin(); j != obj->outputStrings().end(); ++j) { for (KstStringMap::ConstIterator k = _inputStrings.begin(); k != _inputStrings.end(); ++k) { if (j.data() == k.data()) { @@ -748,5 +748,53 @@ } +bool KstDataObject::recursion(KstDataObjectDataObjectMap& objectsToCheck) { + KstDataObjectDataObjectMap objectsToFollow; + bool recurses = false; + + for (KstDataObjectList::ConstIterator it = KST::dataObjectList.begin(); it != KST::dataObjectList.end(); ++it) { + if ((*it)->uses(this)) { + if (objectsToCheck.find(*it) == objectsToCheck.end()) { + objectsToFollow.insert(*it, *it); + } else { + recurses = true; + + break; + } + } + } + + if (!recurses) { + for (KstDataObjectDataObjectMap::Iterator j = objectsToFollow.begin(); j != objectsToFollow.end(); ++j) { + if ((*j)->recursion(objectsToCheck)) { + recurses = true; + + break; + } + objectsToCheck.insert(*j, *j); + } + } + + return recurses; +} + + +bool KstDataObject::recursion() { + bool recurses = false; + + if (uses(this)) { + recurses = true; + } else { + KstDataObjectDataObjectMap objectsToCheck; + + objectsToCheck.insert(this, this); + + recurses = recursion(objectsToCheck); + } + + return recurses; +} + + #include "kstdataobject.moc" // vim: ts=2 sw=2 et --- branches/work/kst/1.5/kst/src/libkstmath/kstdataobject.h #663643:663644 @@ -55,7 +55,6 @@ virtual QString propertyString() const = 0; virtual const QString& type() const { return _type; } virtual Kind kind() const { return Generic; } - virtual int sampleCount() const { return 0; } // If you use these, you must lock() and unlock() the object as long as you @@ -106,6 +105,8 @@ virtual void replaceDependency(KstMatrixPtr oldMatrix, KstMatrixPtr newMatrix); virtual bool uses(KstObjectPtr p) const; + virtual bool recursion(KstDataObjectDataObjectMap& objectsToCheck); + virtual bool recursion(); //These are generally only valid for plugins... const QString& name() const { return _name; }
Another option would be to check for recursions in the editSingleObject() methods of the KstDataDialog dialogs. This will allow us to at least warn the user and potentially prevent the recursion from being created in the first place.
SVN commit 664798 by arwalker: CCBUG:129068 Display message to user to inform they have created a recursion in their dependencies. Probably still some other ways for the user to achieve this M +4 -1 kstbasicdialog_i.cpp M +19 -13 kstcsddialog_i.cpp M +13 -7 ksteqdialog_i.cpp M +6 -0 ksthsdialog_i.cpp M +6 -0 kstplugindialog_i.cpp M +10 -4 kstpsddialog_i.cpp --- branches/work/kst/1.5/kst/src/libkstapp/kstbasicdialog_i.cpp #664797:664798 @@ -311,6 +311,10 @@ KMessageBox::sorry(this, i18n("There is an error in the values you entered.")); return false; } + if (ptr->recursion()) { + KMessageBox::sorry(this, i18n("There is a recursion resulting from the plugin you entered.")); + return false; + } ptr->setDirty(); @@ -394,7 +398,6 @@ } } } - } return true; --- branches/work/kst/1.5/kst/src/libkstapp/kstcsddialog_i.cpp #664797:664798 @@ -245,19 +245,19 @@ // get the values that need to be checked for consistency double pSampRate; int pFFTLen; - + if (_sampRateDirty) { pSampRate = _w->_kstFFTOptions->SampRate->text().toDouble(); } else { pSampRate = csPtr->freq(); } - + if (_fFTLenDirty) { pFFTLen = _w->_kstFFTOptions->FFTLen->text().toInt(); } else { pFFTLen = csPtr->length(); } - + if (!_w->_kstFFTOptions->checkGivenValues(pSampRate, pFFTLen)) { csPtr->unlock(); return false; @@ -266,7 +266,7 @@ if (_sampRateDirty) { csPtr->setFreq(_w->_kstFFTOptions->SampRate->text().toDouble()); } - + if (_fFTLenDirty) { csPtr->setLength(_w->_kstFFTOptions->FFTLen->text().toInt()); } @@ -274,40 +274,46 @@ if (_apodizeDirty) { csPtr->setApodize(_w->_kstFFTOptions->Apodize->isChecked()); } - + if (_apodizeFxnDirty) { csPtr->setApodizeFxn(ApodizeFunction(_w->_kstFFTOptions->ApodizeFxn->currentItem())); } - + if (_gaussianSigmaDirty) { csPtr->setGaussianSigma(_editMultipleMode ? _w->_kstFFTOptions->Sigma->value() - 1 : _w->_kstFFTOptions->Sigma->value()); } - + if (_removeMeanDirty) { csPtr->setRemoveMean(_w->_kstFFTOptions->RemoveMean->isChecked()); } - + if (_interleavedDirty) { csPtr->setAverage(_w->_kstFFTOptions->Interleaved->isChecked()); } - + if (_windowSizeDirty) { csPtr->setWindowSize(_w->_windowSize->value()); } - + if (_rateUnitsDirty) { csPtr->setRateUnits(_w->_kstFFTOptions->RateUnits->text()); } - + if (_vectorUnitsDirty) { csPtr->setVectorUnits(_w->_kstFFTOptions->VectorUnits->text()); } - + if (_outputDirty) { csPtr->setOutput(PSDType(_w->_kstFFTOptions->Output->currentItem())); } - + + if (csPtr->recursion()) { + KMessageBox::error(this, i18n("There is a recursion resulting from the spectrogram you entered.")); + csPtr->unlock(); + return false; + } + csPtr->unlock(); return true; } --- branches/work/kst/1.5/kst/src/libkstapp/ksteqdialog_i.cpp #664797:664798 @@ -293,14 +293,14 @@ vp = eqPtr->vX(); } KST::vectorList.lock().unlock(); - + // update the DoInterpolation only if it is dirty if (_doInterpolationDirty) { eqPtr->setExistingXVector(vp, _w->_doInterpolation->isChecked()); } else { eqPtr->setExistingXVector(vp, eqPtr->doInterp()); } - + if (_equationDirty) { eqPtr->setEquation(_w->_equation->text()); if (!eqPtr->isValid()) { @@ -311,23 +311,29 @@ } KMessageBox::detailedSorry(this, i18n("There is an error in the equation you entered."), parseErrors); eqPtr->unlock(); + return true; + } + if (eqPtr->recursion()) { + KMessageBox::error(this, i18n("There is a recursion resulting from the equation you entered.")); + eqPtr->unlock(); return false; } } eqPtr->unlock(); + return true; } bool KstEqDialogI::editObject() { KstEquationList eqList = kstObjectSubList<KstDataObject,KstEquation>(KST::dataObjectList); - + // if editing multiple objects, edit each one if (_editMultipleMode) { // if the user selected no vector, treat it as non-dirty _xVectorsDirty = _w->_xVectors->_vector->currentItem() != 0; _equationDirty = !_w->_equation->text().isEmpty(); - + bool didEdit = false; for (uint i = 0; i < _editMultipleWidget->_objectList->count(); i++) { if (_editMultipleWidget->_objectList->isSelected(i)) { @@ -336,7 +342,7 @@ if (eqIter == eqList.end()) { return false; } - + KstEquationPtr eqPtr = *eqIter; if (!editSingleObject(eqPtr)) { return false; @@ -356,11 +362,11 @@ _tagName->setFocus(); return false; } - + ep->writeLock(); ep->setTagName(tag_name); ep->unlock(); - + // then edit the object _equationDirty = true; _xVectorsDirty = true; --- branches/work/kst/1.5/kst/src/libkstapp/ksthsdialog_i.cpp #664797:664798 @@ -331,6 +331,12 @@ hsPtr->writeLock(); + if (hsPtr->recursion()) { + KMessageBox::error(this, i18n("There is a recursion resulting from the histogram you entered.")); + hsPtr->unlock(); + return false; + } + if (_nDirty) { hsPtr->setNBins(new_n_bins); } --- branches/work/kst/1.5/kst/src/libkstapp/kstplugindialog_i.cpp #664797:664798 @@ -639,6 +639,12 @@ KMessageBox::sorry(this, i18n("There is an error in the plugin you entered.")); return false; } + + if (pp->recursion()) { + KMessageBox::sorry(this, i18n("There is a recursion resulting from the plugin you entered.")); + return false; + } + pp->setDirty(); emit modified(); --- branches/work/kst/1.5/kst/src/libkstapp/kstpsddialog_i.cpp #664797:664798 @@ -261,22 +261,28 @@ psPtr->setVector(v); } + if (psPtr->recursion()) { + KMessageBox::error(this, i18n("There is a recursion resulting from the spectrum you entered.")); + psPtr->unlock(); + return false; + } + // get the values that need to be checked for consistency double pSampRate; int pFFTLen; - + if (_sampRateDirty) { pSampRate = _w->_kstFFTOptions->SampRate->text().toDouble(); } else { pSampRate = psPtr->freq(); } - + if (_fFTLenDirty) { pFFTLen = _w->_kstFFTOptions->FFTLen->text().toInt(); } else { pFFTLen = psPtr->len(); } - + if (!_w->_kstFFTOptions->checkGivenValues(pSampRate, pFFTLen)) { psPtr->unlock(); return false; @@ -285,7 +291,7 @@ if (_sampRateDirty) { psPtr->setFreq(_w->_kstFFTOptions->SampRate->text().toDouble()); } - + if (_fFTLenDirty) { psPtr->setLen(_w->_kstFFTOptions->FFTLen->text().toInt()); }
SVN commit 665376 by arwalker: CCBUG:129068 Warn user if event monitor entry has a recursion. Also prevent crash from using an invalid expression when the event slave vector is used as input to the expression. M +50 -44 ksteventmonitorentry.cpp --- branches/work/kst/1.5/kst/src/libkstapp/ksteventmonitorentry.cpp #665375:665376 @@ -130,7 +130,11 @@ Equation::FoldVisitor vis(&ctx, &_pExpression); KstStringMap stm; if (_pExpression->collectObjects(_vectorsUsed, _inputScalars, stm)) { - _isValid = true; + if (recursion()) { + KstDebug::self()->log( i18n("There is a recursion resulting from the event monitor entry you entered, '%s'").arg(_event), KstDebug::Warning); + } else { + _isValid = true; + } } else { //we have bad objects... delete (Equation::Node*)ParsedEquation; @@ -190,63 +194,65 @@ if (!_pExpression) { reparse(); } + if (_isValid) { + KstVectorPtr xv = *_xVector; + KstVectorPtr yv = *_yVector; + int ns = 1; - KstVectorPtr xv = *_xVector; - KstVectorPtr yv = *_yVector; - int ns = 1; + for (KstVectorMap::ConstIterator i = _vectorsUsed.begin(); i != _vectorsUsed.end(); ++i) { + ns = kMax(ns, i.data()->length()); + } - for (KstVectorMap::ConstIterator i = _vectorsUsed.begin(); i != _vectorsUsed.end(); ++i) { - ns = kMax(ns, i.data()->length()); - } + double *rawValuesX = 0L; + double *rawValuesY = 0L; + if (xv && yv) { + if (xv->resize(ns)) { + rawValuesX = xv->value(); + } - double *rawValuesX = 0L; - double *rawValuesY = 0L; - if (xv && yv) { - if (xv->resize(ns)) { - rawValuesX = xv->value(); + if (yv->resize(ns)) { + rawValuesY = yv->value(); + } } - if (yv->resize(ns)) { - rawValuesY = yv->value(); - } - } + Equation::Context ctx; + ctx.sampleCount = ns; + ctx.x = 0.0; - Equation::Context ctx; - ctx.sampleCount = ns; - ctx.x = 0.0; + if (needToEvaluate()) { + if (_pExpression) { + for (ctx.i = _numDone; ctx.i < ns; ++ctx.i) { + const double value = _pExpression->value(&ctx); - if (needToEvaluate()) { - if (_pExpression) { - for (ctx.i = _numDone; ctx.i < ns; ++ctx.i) { - const double value = _pExpression->value(&ctx); - if (value != 0.0) { // The expression evaluates to true - log(ctx.i); - if (rawValuesX && rawValuesY) { - rawValuesX[ctx.i] = ctx.i; - rawValuesY[ctx.i] = 1.0; + if (value != 0.0) { // The expression evaluates to true + log(ctx.i); + if (rawValuesX && rawValuesY) { + rawValuesX[ctx.i] = ctx.i; + rawValuesY[ctx.i] = 1.0; + } + } else { + if (rawValuesX && rawValuesY) { + rawValuesX[ctx.i] = ctx.i; + rawValuesY[ctx.i] = 0.0; + } } - } else { - if (rawValuesX && rawValuesY) { - rawValuesX[ctx.i] = ctx.i; - rawValuesY[ctx.i] = 0.0; - } } + _numDone = ns; + logImmediately(); } + } else { _numDone = ns; - logImmediately(); } - } else { - _numDone = ns; - } - if (xv) { - xv->setDirty(); - xv->update(updateCounter); - } + if (xv) { + xv->setDirty(); + xv->update(updateCounter); + } - if (yv) { - yv->setDirty(); - yv->update(updateCounter); + if (yv) { + yv->setDirty(); + yv->update(updateCounter); + } } unlockInputsAndOutputs();
SVN commit 665403 by arwalker: CCBUG:129068 Prevent updates when an object is recursed M +3 -0 libkstapp/kstbasicdialog_i.cpp M +2 -0 libkstapp/kstcsddialog_i.cpp M +4 -0 libkstapp/ksteqdialog_i.cpp M +8 -6 libkstapp/ksthsdialog_i.cpp M +3 -0 libkstapp/kstplugindialog_i.cpp M +8 -6 libkstapp/kstpsddialog_i.cpp M +4 -0 libkstmath/kstbasicplugin.cpp M +4 -0 libkstmath/kstcplugin.cpp M +7 -0 libkstmath/kstcsd.cpp M +12 -0 libkstmath/kstdataobject.cpp M +4 -0 libkstmath/kstdataobject.h M +5 -0 libkstmath/kstequation.cpp M +5 -0 libkstmath/ksthistogram.cpp M +6 -0 libkstmath/kstpsd.cpp --- branches/work/kst/1.5/kst/src/libkstapp/kstbasicdialog_i.cpp #665402:665403 @@ -311,7 +311,10 @@ KMessageBox::sorry(this, i18n("There is an error in the values you entered.")); return false; } + + ptr->setRecursed(false); if (ptr->recursion()) { + ptr->setRecursed(true); KMessageBox::sorry(this, i18n("There is a recursion resulting from the plugin you entered.")); return false; } --- branches/work/kst/1.5/kst/src/libkstapp/kstcsddialog_i.cpp #665402:665403 @@ -308,8 +308,10 @@ csPtr->setOutput(PSDType(_w->_kstFFTOptions->Output->currentItem())); } + csPtr->setRecursed(false); if (csPtr->recursion()) { KMessageBox::error(this, i18n("There is a recursion resulting from the spectrogram you entered.")); + csPtr->setRecursed(true); csPtr->unlock(); return false; } --- branches/work/kst/1.5/kst/src/libkstapp/ksteqdialog_i.cpp #665402:665403 @@ -313,12 +313,16 @@ eqPtr->unlock(); return true; } + + eqPtr->setRecursed(false); if (eqPtr->recursion()) { KMessageBox::error(this, i18n("There is a recursion resulting from the equation you entered.")); + eqPtr->setRecursed(true); eqPtr->unlock(); return false; } } + eqPtr->unlock(); return true; --- branches/work/kst/1.5/kst/src/libkstapp/ksthsdialog_i.cpp #665402:665403 @@ -331,12 +331,6 @@ hsPtr->writeLock(); - if (hsPtr->recursion()) { - KMessageBox::error(this, i18n("There is a recursion resulting from the histogram you entered.")); - hsPtr->unlock(); - return false; - } - if (_nDirty) { hsPtr->setNBins(new_n_bins); } @@ -361,6 +355,14 @@ } } + hsPtr->setRecursed(false); + if (hsPtr->recursion()) { + KMessageBox::error(this, i18n("There is a recursion resulting from the histogram you entered.")); + hsPtr->setRecursed(true); + hsPtr->unlock(); + return false; + } + hsPtr->setDirty(); hsPtr->unlock(); return true; --- branches/work/kst/1.5/kst/src/libkstapp/kstplugindialog_i.cpp #665402:665403 @@ -616,6 +616,7 @@ int pitem = _w->PluginCombo->currentItem(); KstSharedPtr<Plugin> pPtr = PluginCollection::self()->plugin(_pluginList[pitem]); + pp->setRecursed(false); pp->inputVectors().clear(); pp->inputScalars().clear(); pp->inputStrings().clear(); @@ -640,8 +641,10 @@ return false; } + pp->setRecursed(false); if (pp->recursion()) { KMessageBox::sorry(this, i18n("There is a recursion resulting from the plugin you entered.")); + pp->setRecursed(true); return false; } --- branches/work/kst/1.5/kst/src/libkstapp/kstpsddialog_i.cpp #665402:665403 @@ -261,12 +261,6 @@ psPtr->setVector(v); } - if (psPtr->recursion()) { - KMessageBox::error(this, i18n("There is a recursion resulting from the spectrum you entered.")); - psPtr->unlock(); - return false; - } - // get the values that need to be checked for consistency double pSampRate; int pFFTLen; @@ -333,6 +327,14 @@ psPtr->setInterpolateHoles(_w->_kstFFTOptions->InterpolateHoles->isChecked()); } + psPtr->setRecursed(false); + if (psPtr->recursion()) { + KMessageBox::error(this, i18n("There is a recursion resulting from the spectrum you entered.")); + psPtr->setRecursed(true); + psPtr->unlock(); + return false; + } + psPtr->unlock(); return true; } --- branches/work/kst/1.5/kst/src/libkstmath/kstbasicplugin.cpp #665402:665403 @@ -205,6 +205,10 @@ KstObject::UpdateType KstBasicPlugin::update(int updateCounter) { Q_ASSERT(myLockStatus() == KstRWLock::WRITELOCKED); + if (recursed()) { + return setLastUpdateResult(NO_CHANGE); + } + bool force = dirty(); setDirty(false); --- branches/work/kst/1.5/kst/src/libkstmath/kstcplugin.cpp #665402:665403 @@ -256,6 +256,10 @@ return setLastUpdateResult(NO_CHANGE); } + if (recursed()) { + return setLastUpdateResult(NO_CHANGE); + } + bool force = dirty(); setDirty(false); --- branches/work/kst/1.5/kst/src/libkstmath/kstcsd.cpp #665402:665403 @@ -171,6 +171,10 @@ return lastUpdateResult(); } + if (recursed()) { + return setLastUpdateResult(NO_CHANGE); + } + writeLockInputsAndOutputs(); if (update_counter <= 0) { @@ -255,6 +259,9 @@ void KstCSD::setVector(KstVectorPtr new_v) { KstVectorPtr v = _inputVectors[INVECTOR]; + + setRecursed(false); + if (v) { if (v == new_v) { return; --- branches/work/kst/1.5/kst/src/libkstmath/kstdataobject.cpp #665402:665403 @@ -40,6 +40,7 @@ //kstdDebug() << "+++ CREATING DATA OBJECT: " << (void*)this << endl; _curveHints = new KstCurveHintList; _isInputLoaded = false; + _isRecursed = false; } KstDataObject::KstDataObject(const QDomElement& e) : KstObject() { @@ -47,6 +48,7 @@ //kstdDebug() << "+++ CREATING DATA OBJECT: " << (void*)this << endl; _curveHints = new KstCurveHintList; _isInputLoaded = false; + _isRecursed = false; } @@ -796,5 +798,15 @@ } +void KstDataObject::setRecursed( bool isRecursed ) { + _isRecursed = isRecursed; +} + + +bool KstDataObject::recursed() const { + return _isRecursed; +} + + #include "kstdataobject.moc" // vim: ts=2 sw=2 et --- branches/work/kst/1.5/kst/src/libkstmath/kstdataobject.h #665402:665403 @@ -57,6 +57,9 @@ virtual Kind kind() const { return Generic; } virtual int sampleCount() const { return 0; } + virtual bool recursed() const; + virtual void setRecursed(bool recursed); + // If you use these, you must lock() and unlock() the object as long as you // hold the reference const KstVectorMap& inputVectors() const { return _inputVectors; } @@ -148,6 +151,7 @@ QString _typeString, _type; bool _isInputLoaded; + bool _isRecursed : 1; QValueList<QPair<QString,QString> > _inputVectorLoadQueue; QValueList<QPair<QString,QString> > _inputScalarLoadQueue; QValueList<QPair<QString,QString> > _inputStringLoadQueue; --- branches/work/kst/1.5/kst/src/libkstmath/kstequation.cpp #665402:665403 @@ -193,6 +193,10 @@ return setLastUpdateResult(NO_CHANGE); } + if (recursed()) { + return setLastUpdateResult(NO_CHANGE); + } + assert(update_counter >= 0); if (_xInVector == _inputVectors.end()) { @@ -295,6 +299,7 @@ // document loading with vector lazy-loading setDirty(); _equation = in_fn; + setRecursed(false); VectorsUsed.clear(); ScalarsUsed.clear(); --- branches/work/kst/1.5/kst/src/libkstmath/ksthistogram.cpp #665402:665403 @@ -158,6 +158,10 @@ bool force = dirty(); setDirty(false); + if (recursed()) { + return setLastUpdateResult(KstObject::NO_CHANGE); + } + if (KstObject::checkUpdateCounter(update_counter) && !force) { return lastUpdateResult(); } @@ -320,6 +324,7 @@ void KstHistogram::setVector(KstVectorPtr new_v) { _inputVectors[RAWVECTOR] = new_v; + setRecursed(false); } --- branches/work/kst/1.5/kst/src/libkstmath/kstpsd.cpp #665402:665403 @@ -194,6 +194,10 @@ return lastUpdateResult(); } + if (recursed()) { + return setLastUpdateResult(NO_CHANGE); + } + writeLockInputsAndOutputs(); KstVectorPtr iv = _inputVectors[INVECTOR]; @@ -370,6 +374,8 @@ void KstPSD::setVector(KstVectorPtr new_v) { Q_ASSERT(myLockStatus() == KstRWLock::WRITELOCKED); + setRecursed(false); + KstVectorPtr v = _inputVectors[INVECTOR]; if (v) { if (v == new_v) {
Recursion tests now prevent the user from selecting the slave vector of an object for the object's input. Further, any recursion has been prevented. The user will be prevented from ok'ing a dialog with a selection that would result in a recursion.