From 9be33afba06c468e37a0148342d039c1aa500051 Mon Sep 17 00:00:00 2001 From: Bill Dolinar Date: Tue, 6 Oct 2026 10:56:14 -0600 Subject: [PATCH 1/3] Fix the critical items from the 2026-09-25 repo review Addresses C1-C10 in docs/reviews/2026-09-25-repo-review.md: - C1: Observer's default handlers call super() instead of calling themselves forever. - C2: ProgressListener.on_begin_operation_string calls super() instead of calling itself forever. - C3: DaStreamReader::ReadBinaryBytes checks the block lengths it reads from the file before using them, sizes its decode buffer for the longest decode, and fails on truncated data. iBase64Decode rejects a length of 1 mod 4, where boost's decoder steps past the end of the source, and returns a failure for a non-base64 character instead of letting boost's dataflow_exception escape. - C4: ReadBinaryBytes rejects a block bigger than the destination. - C5: DaStreamWriter::SetBinaryBlockSize rejects a non-positive size, which made WriteBinaryBytes loop forever. - C6: stSplit without compression gives n + 1 fields for n delimiters, and an empty source gives one empty field instead of reading past the end. - C7: XM_ENSURE_FALSE_T_5 expands to XM_ENSURE_TRUE_T_5, so it compiles. - C8: Pt3d and Pt4d operator< compare z in the right direction. - C9: IntPairFromPyIter returns the pair's values instead of (1, 1). - C10: PublicProgressListener uninstalls its global listener when it is destroyed, so the listener cannot call back into a deleted object. Each fix has a regression test. The Doxygen for SetBinaryBlockSize, iBase64Decode and stSplit is updated to match. --- _package/tests/unit_tests/observer_pyt.py | 45 +++++ .../tests/unit_tests/progress_listener_pyt.py | 23 +++ _package/xms/core/misc/observer.py | 12 +- _package/xms/core/misc/progress_listener.py | 2 +- xmscore/dataio/daStreamIo.cpp | 161 ++++++++++++++++-- xmscore/dataio/daStreamIo.h | 4 +- xmscore/dataio/daStreamIo.t.h | 2 + xmscore/misc/StringUtil.cpp | 45 +++-- xmscore/misc/StringUtil.h | 4 +- xmscore/misc/XmError.cpp | 38 +++++ xmscore/misc/XmError.h | 2 +- xmscore/points/pt.cpp | 8 + xmscore/points/pt.h | 4 +- xmscore/python/misc/PyUtils.cpp | 2 +- .../misc/detail/PublicProgressListener.cpp | 9 +- 15 files changed, 320 insertions(+), 41 deletions(-) create mode 100644 _package/tests/unit_tests/progress_listener_pyt.py diff --git a/_package/tests/unit_tests/observer_pyt.py b/_package/tests/unit_tests/observer_pyt.py index 1e0399769..b0fb064e3 100644 --- a/_package/tests/unit_tests/observer_pyt.py +++ b/_package/tests/unit_tests/observer_pyt.py @@ -190,5 +190,50 @@ def test_update_message(self): self.assertEqual(message, self.observer.status['message']) +class PartialObserver(Observer): + """Observer that overrides only on_progress_status and inherits the other handlers.""" + + def __init__(self): + """ + The __init__ function for the PartialObserver class. + """ + super().__init__() + self.progress = [] + + def on_progress_status(self, percent_complete): + """ + Record percent complete. + + Args: + percent_complete: The percent complete. + """ + self.progress.append(percent_complete) + + +class TestObserverDefaultHandlers(unittest.TestCase): + """Test the handlers Observer provides when a subclass does not override them.""" + + def test_observer_without_overrides(self): + """ + Test that an Observer with no overrides accepts every event without recursing. + """ + observer = Observer() + observer.begin_operation_string('count-jelly-beans') + observer.progress_status(0.5) + observer.update_message('21 jelly beans counted so far.') + observer.end_operation() + + def test_observer_with_partial_overrides(self): + """ + Test that a subclass overriding one handler still gets events while the rest use the defaults. + """ + observer = PartialObserver() + observer.begin_operation_string('count-jelly-beans') + observer.progress_status(0.5) + self.assertEqual(0.5, observer.progress[-1]) + observer.update_message('21 jelly beans counted so far.') + observer.end_operation() + + if __name__ == '__main__': unittest.main() diff --git a/_package/tests/unit_tests/progress_listener_pyt.py b/_package/tests/unit_tests/progress_listener_pyt.py new file mode 100644 index 000000000..3634ce10a --- /dev/null +++ b/_package/tests/unit_tests/progress_listener_pyt.py @@ -0,0 +1,23 @@ +""" +Test progress_listener.py. +""" +import unittest + +from xms.core.misc.progress_listener import set_listener_callback + + +class TestProgressListener(unittest.TestCase): + """Test ProgressListener Class.""" + + def test_on_begin_operation_string(self): + """ + Test that beginning an operation reports the stack index it returns to the call back. + """ + messages = [] + listener = set_listener_callback(messages.append) + stack_index = listener.on_begin_operation_string('count-jelly-beans') + self.assertEqual([('begin_operation', stack_index, 'count-jelly-beans')], messages) + + +if __name__ == '__main__': + unittest.main() diff --git a/_package/xms/core/misc/observer.py b/_package/xms/core/misc/observer.py index 66f23880a..2f2d6c305 100644 --- a/_package/xms/core/misc/observer.py +++ b/_package/xms/core/misc/observer.py @@ -21,7 +21,7 @@ def on_progress_status(self, percent_complete): Args: percent_complete (float): The percent complete """ - self.on_progress_status(percent_complete) + super().on_progress_status(percent_complete) def on_begin_operation_string(self, operation): """ @@ -30,13 +30,13 @@ def on_begin_operation_string(self, operation): Args: operation: Name of the operation being monitored. """ - self.on_begin_operation_string(operation) + super().on_begin_operation_string(operation) def on_end_operation(self): """ The end operation event. """ - self.on_end_operation() + super().on_end_operation() def on_update_message(self, message): """ @@ -45,7 +45,7 @@ def on_update_message(self, message): Args: message: The new message """ - self.on_update_message(message) + super().on_update_message(message) def time_remaining_in_seconds(self, remaining_seconds): """ @@ -54,7 +54,7 @@ def time_remaining_in_seconds(self, remaining_seconds): Args: remaining_seconds (Float): The time remaining for the current operation that the class is observing. """ - self.time_remaining_in_seconds(remaining_seconds) + super().time_remaining_in_seconds(remaining_seconds) def time_elapsed_in_seconds(self, elapsed_seconds): """ @@ -63,4 +63,4 @@ def time_elapsed_in_seconds(self, elapsed_seconds): Args: elapsed_seconds (Float): The elapsed time since the operation began. """ - self.time_elapsed_in_seconds(elapsed_seconds) + super().time_elapsed_in_seconds(elapsed_seconds) diff --git a/_package/xms/core/misc/progress_listener.py b/_package/xms/core/misc/progress_listener.py index f62e2175f..ca7ad814d 100644 --- a/_package/xms/core/misc/progress_listener.py +++ b/_package/xms/core/misc/progress_listener.py @@ -58,7 +58,7 @@ def on_begin_operation_string(self, operation): Returns: (int): stack index for the operation """ - stack_index = self.on_begin_operation_string(operation) + stack_index = super().on_begin_operation_string(operation) self.call_back(('begin_operation', stack_index, operation)) return stack_index diff --git a/xmscore/dataio/daStreamIo.cpp b/xmscore/dataio/daStreamIo.cpp index 2728218b1..22d884776 100644 --- a/xmscore/dataio/daStreamIo.cpp +++ b/xmscore/dataio/daStreamIo.cpp @@ -19,6 +19,7 @@ // 4. External Library Headers #include #include +#include #include #include #include @@ -156,7 +157,14 @@ int32_t iBase64Encode(const char* a_source, int32_t a_sourceLength, char* a_dest return encodedLength; } // iBase64Encode //------------------------------------------------------------------------------ -/// \brief +/// \brief Decodes base64 text. +/// \param a_source The base64 text to decode. +/// \param a_sourceLength The number of characters to decode. +/// \param a_dest The destination for the decoded bytes. Must hold at least +/// a_sourceLength * 3 / 4 bytes. +/// \return The length of the decoded bytes, or -1 if the text holds a +/// character outside the base64 alphabet or its length without +/// padding leaves a single character over. //------------------------------------------------------------------------------ int32_t iBase64Decode(const char* a_source, int32_t a_sourceLength, char* a_dest) { @@ -171,14 +179,30 @@ int32_t iBase64Decode(const char* a_source, int32_t a_sourceLength, char* a_dest --size; } + // A single trailing character holds less than a byte, and the decoder steps + // past the end of the source looking for the rest of it. + if (size % 4 == 1) + { + XM_LOG(xmlog::error, "Unable to read file. Invalid base64 data length.") + return -1; + } + if (size != 0) { char* destBegin = a_dest; char* destIterator = a_dest; auto base64Begin = base64_dec(a_source); auto base64End = base64_dec(a_source + size); - for (auto it = base64Begin; it != base64End; ++it) - *destIterator++ = *it; + try + { + for (auto it = base64Begin; it != base64End; ++it) + *destIterator++ = *it; + } + catch (const dataflow_exception&) + { + XM_LOG(xmlog::error, "Unable to read file. Invalid base64 character.") + return -1; + } auto decodedLength = int32_t(destIterator - destBegin); return decodedLength; @@ -666,25 +690,40 @@ bool DaStreamReader::ReadBinaryBytes(char* a_dest, long long a_destLength) { auto maxCompressedLength = static_cast(compressBound(MAX_BLOCK_SIZE)); int32_t maxEncodeLength = iBase64EncodeSize(maxCompressedLength); - std::unique_ptr compressed(new char[maxCompressedLength]); + // Every 4 base64 characters decode to 3 bytes, so a block of maxEncodeLength + // characters can decode to a few bytes more than maxCompressedLength. + int32_t maxDecodeLength = maxEncodeLength * 3 / 4; + std::unique_ptr compressed(new char[maxDecodeLength]); std::unique_ptr encoded(new char[maxEncodeLength + 1]); while (a_destLength > 0) { // read block info std::string blockString; - int32_t encodedLength; - int32_t blockLength; - ReadString(blockString); - ReadInt(encodedLength); - ReadInt(blockLength); - NextLine(); + int32_t encodedLength = 0; + int32_t blockLength = 0; + if (!ReadString(blockString) || !ReadInt(encodedLength) || !ReadInt(blockLength) || + !NextLine()) + { + return false; + } + + // the lengths come from the file so they must fit the buffers + if (encodedLength < 0 || encodedLength > maxEncodeLength || blockLength <= 0 || + blockLength > a_destLength) + { + XM_LOG(xmlog::error, "Unable to read file. Invalid binary block length."); + return false; + } // read and decode m_impl->m_inStream.read(encoded.get(), encodedLength); - NextLine(); + if (m_impl->m_inStream.gcount() != encodedLength || !NextLine()) + return false; auto compressedLength = iBase64Decode(encoded.get(), encodedLength, compressed.get()); + if (compressedLength < 0) + return false; // decompress data if (!iUncompress(compressed.get(), compressedLength, a_dest, blockLength)) @@ -950,10 +989,13 @@ bool DaStreamWriter::WriteBinaryBytes(const char* a_source, long long a_sourceLe } // DaStreamWriter::WriteBinaryBytes //------------------------------------------------------------------------------ /// \brief Set the block size to use when writing binary arrays. -/// \param a_blockSize The block size. +/// \param a_blockSize The block size. Must be positive; larger values are +/// clamped to the maximum block size. A non-positive value is +/// rejected and the current block size is kept. //------------------------------------------------------------------------------ void DaStreamWriter::SetBinaryBlockSize(int a_blockSize) { + XM_ENSURE_TRUE_VOID(a_blockSize > 0); m_impl->m_blockSize = a_blockSize > MAX_BLOCK_SIZE ? MAX_BLOCK_SIZE : a_blockSize; } // DaStreamWriter::SetBinaryBlockSize @@ -2279,6 +2321,101 @@ void DaReaderWriterIoUnitTests::testReadWriteBinaryArrays() TS_ASSERT_EQUALS(outValues, inValues); } // DaReaderWriterIoUnitTests::testReadWriteBinaryArrays //------------------------------------------------------------------------------ +/// \brief Test that DaStreamReader::ReadBinaryBytes returns false instead of +/// overrunning its buffers when a binary block is malformed. +//------------------------------------------------------------------------------ +void DaReaderWriterIoUnitTests::testReadBinaryBytesRejectsBadBlocks() +{ + const std::string goodData = + "eAENw4UNwDAAAKDO3d3+f3OQEEIIkbGJqZm5haWVtY2tnb2Do5Ozi6ubu4enl7ePr58/RrQBtA\n"; + int32_t maxEncodeLength = + iBase64EncodeSize(static_cast(compressBound(MAX_BLOCK_SIZE))); + const std::string badLength = "Invalid binary block length."; + const std::string badBase64Length = "Invalid base64 data length."; + const std::string badBase64Char = "Invalid base64 character."; + const std::string badCompressed = "Unable to uncompress data."; + // Each bad input and the error logged by the check that should reject it. + // An empty error means the input is rejected without logging. + const std::vector> badInputs = { + {"", ""}, // no header + {"BINARY_BLOCK\n", ""}, // truncated header + {"BINARY_BLOCK 74\n", ""}, // no block length + {"BINARY_BLOCK 999999 120\n" + goodData, badLength}, // encoded length too big + {"BINARY_BLOCK -1 120\n" + goodData, badLength}, // negative encoded length + {"BINARY_BLOCK 74 0\n" + goodData, badLength}, // zero block length + {"BINARY_BLOCK 74 -5\n" + goodData, badLength}, // negative block length + {"BINARY_BLOCK 74 121\n" + goodData, badLength}, // bigger than destination + {"BINARY_BLOCK 74 120\neAENw4UNwDAAAKDO\n", ""}, // truncated data + {"BINARY_BLOCK 75 120\n" + goodData + "\n", badBase64Char}, // newline in the data + {"BINARY_BLOCK 5 120\nAAAAA\n", badBase64Length}, // not a base64 length + {"BINARY_BLOCK 3 120\nA==\n", badBase64Length}, // not a base64 length + // longest allowed block without base64 padding decodes past the + // compressed length; must fail to uncompress without overflowing + {"BINARY_BLOCK " + std::to_string(maxEncodeLength) + " 120\n" + + std::string(maxEncodeLength, 'A') + "\n", + badCompressed}, + }; + + XmLog::Instance().GetAndClearStackStr(); + for (const auto& [input, error] : badInputs) + { + std::istringstream inputStream(input); + DaStreamReader reader(inputStream); + std::vector dest(120); + std::string label = input.substr(0, 30); + TSM_ASSERT(label, !reader.ReadBinaryBytes(&dest[0], (long long)dest.size())); + std::string errors = XmLog::Instance().GetAndClearStackStr(); + TSM_ASSERT(label + " logged: " + errors, errors.find(error) != std::string::npos); + } + + // a good block that is bigger than the destination + { + std::istringstream inputStream("BINARY_BLOCK 74 120\n" + goodData); + DaStreamReader reader(inputStream); + std::vector dest(120); + TS_ASSERT(!reader.ReadBinaryBytes(&dest[0], 60)); + } + + // the good block still reads + std::istringstream inputStream("BINARY_BLOCK 74 120\n" + goodData); + DaStreamReader reader(inputStream); + VecInt values(30); + TS_ASSERT(reader.ReadBinaryBytes((char*)&values[0], 120)); + VecInt expected(30); + std::iota(expected.begin(), expected.end(), 0); + TS_ASSERT_EQUALS(expected, values); + + XmLog::Instance().GetAndClearStackStr(); +} // DaReaderWriterIoUnitTests::testReadBinaryBytesRejectsBadBlocks +//------------------------------------------------------------------------------ +/// \brief Test that DaStreamWriter::SetBinaryBlockSize ignores a non-positive +/// size instead of making WriteBinaryBytes loop forever. +//------------------------------------------------------------------------------ +void DaReaderWriterIoUnitTests::testSetBinaryBlockSizeRejectsNonPositive() +{ + VecInt values(100); + std::iota(values.begin(), values.end(), 0); + long long lengthInBytes = (long long)values.size() * sizeof(VecInt::value_type); + + std::ostringstream expected; + DaStreamWriter expectedWriter(expected); + expectedWriter.SetBinaryBlockSize(120); + expectedWriter.WriteBinaryBytes((char*)&values[0], lengthInBytes); + + bool asserting = xmAsserting(); + xmAsserting() = false; + for (int blockSize : {0, -1}) + { + std::ostringstream output; + DaStreamWriter writer(output); + writer.SetBinaryBlockSize(120); + writer.SetBinaryBlockSize(blockSize); // rejected, keeps 120 + TS_ASSERT(writer.WriteBinaryBytes((char*)&values[0], lengthInBytes)); + TS_ASSERT_EQUALS(expected.str(), output.str()); + } + xmAsserting() = asserting; +} // DaReaderWriterIoUnitTests::testSetBinaryBlockSizeRejectsNonPositive +//------------------------------------------------------------------------------ /// \brief Test DaStreamReader::LineBeginsWith //------------------------------------------------------------------------------ void DaReaderWriterIoUnitTests::testLineBeginsWith() diff --git a/xmscore/dataio/daStreamIo.h b/xmscore/dataio/daStreamIo.h index 060ec44c8..f76bfd9bb 100644 --- a/xmscore/dataio/daStreamIo.h +++ b/xmscore/dataio/daStreamIo.h @@ -170,7 +170,9 @@ class DaStreamWriter /// \brief Write a_sourceLength raw bytes into the binary section of the stream. bool WriteBinaryBytes(const char* a_source, long long a_sourceLength); - /// \brief Set the maximum number of bytes per binary block. + /// \brief Set the maximum number of bytes per binary block. Must be + /// positive; a non-positive value is rejected and the current size + /// is kept. void SetBinaryBlockSize(int a_blockSize); private: diff --git a/xmscore/dataio/daStreamIo.t.h b/xmscore/dataio/daStreamIo.t.h index a6f33b611..73db7585f 100644 --- a/xmscore/dataio/daStreamIo.t.h +++ b/xmscore/dataio/daStreamIo.t.h @@ -64,6 +64,8 @@ class DaReaderWriterIoUnitTests : public CxxTest::TestSuite void testReadWriteBinaryVecPt3d(); void testReadWriteLineParts(); void testReadWriteBinaryArrays(); + void testReadBinaryBytesRejectsBadBlocks(); + void testSetBinaryBlockSizeRejectsNonPositive(); void testLineBeginsWith(); }; diff --git a/xmscore/misc/StringUtil.cpp b/xmscore/misc/StringUtil.cpp index c9f28c64b..18eee962d 100644 --- a/xmscore/misc/StringUtil.cpp +++ b/xmscore/misc/StringUtil.cpp @@ -175,9 +175,12 @@ VecStr stExplode(const std::string &source, const std::string &a_delimiter) { /// \brief Breaks string into vector of strings based on one or more delimiters. /// \param[in] a_source: The string to be split. /// \param[in] a_delimiterList: String where each character is a delimeter. -/// \param[in] a_delimiterCompressOn: If true, consecutive delimeters are -/// treated as one. -/// \return A vector of strings. +/// \param[in] a_delimiterCompressOn: If true, leading and trailing delimeters +/// are dropped and consecutive delimeters +/// are treated as one. If false, n +/// delimeters give n + 1 strings, empty +/// ones included. +/// \return A vector of strings. An empty source gives one empty string. /// \see http://stackoverflow.com/questions/236129/split-a-string-in-c //------------------------------------------------------------------------------ VecStr stSplit(const std::string &a_source, @@ -190,21 +193,18 @@ VecStr stSplit(const std::string &a_source, boost::token_compress_on); } else { // Unfortunately, boost::token_compress_off doesn't do the trick because - // it still compresses the leading and trailing delimiters. The following - // code works. I don't know how fast it is. - std::vector temp(a_source.size()); - int end = 0; - for (size_t i = 0; i < a_source.size(); ++i) { - if (a_delimiterList.find(a_source[i]) != std::string::npos) { - elems.push_back(std::string(&temp[0], end)); - end = 0; + // it still compresses the leading and trailing delimiters. Split by hand + // so that n delimiters always give n + 1 tokens. + std::string token; + for (char c : a_source) { + if (a_delimiterList.find(c) != std::string::npos) { + elems.push_back(token); + token.clear(); } else { - temp[end++] = a_source[i]; + token += c; } } - if (a_delimiterList.find(a_source.back()) != std::string::npos) { - elems.push_back(""); - } + elems.push_back(token); } return elems; } // stSplit @@ -1391,6 +1391,21 @@ void StringUtilUnitTests::testSplit() { r = xms::stSplit(",,A,B,,C,,,", ",", false); expected = {"", "", "A", "B", "", "C", "", "", ""}; TS_ASSERT_EQUALS_VEC(expected, r); + + r = xms::stSplit("A,B", ",", false); + expected = {"A", "B"}; + TS_ASSERT_EQUALS_VEC(expected, r); + + r = xms::stSplit("A", ",", false); + expected = {"A"}; + TS_ASSERT_EQUALS_VEC(expected, r); + + // empty input gives one empty token with or without compression + expected = {""}; + r = xms::stSplit("", ",", false); + TS_ASSERT_EQUALS_VEC(expected, r); + r = xms::stSplit("", ","); + TS_ASSERT_EQUALS_VEC(expected, r); } // StringUtilUnitTests::testSplit //------------------------------------------------------------------------------ /// \brief Test stMakeUnique diff --git a/xmscore/misc/StringUtil.h b/xmscore/misc/StringUtil.h index a02fca05d..e10ec3ff3 100644 --- a/xmscore/misc/StringUtil.h +++ b/xmscore/misc/StringUtil.h @@ -65,7 +65,9 @@ bool stEqualNoCase(const std::string&, const std::string&); /// \brief Returns true if the second string occurs anywhere in the first, ignoring case. bool stFindNoCase(const std::string&, const std::string&); -/// \brief Split source on any character in a_delimiterList; runs of delimiters are coalesced when a_delimiterCompressOn is true. +/// \brief Split source on any character in a_delimiterList. When a_delimiterCompressOn is true, +/// leading and trailing delimiters are dropped and runs of delimiters are coalesced; when false, +/// n delimiters give n + 1 fields, empty ones included. An empty source gives one empty field. VecStr stSplit(const std::string& source, const std::string& a_delimiterList = ST_WHITESPACE, bool a_delimiterCompressOn = true); diff --git a/xmscore/misc/XmError.cpp b/xmscore/misc/XmError.cpp index 5a8f26052..5c4595727 100644 --- a/xmscore/misc/XmError.cpp +++ b/xmscore/misc/XmError.cpp @@ -250,6 +250,37 @@ xmresult iTest_XM_ENSURE_SUCCESS_T(bool a_assert) //------------------------------------------------------------------------------ /// \brief //------------------------------------------------------------------------------ +xmresult iTest_XM_ENSURE_FALSE_T(bool a_assert) +{ + try + { + bool foo = false; + XM_ENSURE_FALSE_T(foo, XR_FAILURE); // should pass + XM_ENSURE_FALSE_T_NO_ASSERT(foo, XR_FAILURE); // should pass + + foo = true; + if (a_assert) + { + XmAssertScopedDisable disable; + XM_ENSURE_FALSE_T(foo, XR_FAILURE); // should throw + } + else + { + XM_ENSURE_FALSE_T_NO_ASSERT(foo, XR_FAILURE); // should throw + } + + TS_FAIL("iTest_XM_ENSURE_FALSE_T"); // shouldn't get here + } + catch (xmresult& e) + { + TS_ASSERT_EQUALS(e, XR_FAILURE); + return XR_SUCCESS; + } + return XR_FAILURE; +} // iTest_XM_ENSURE_FALSE_T +//------------------------------------------------------------------------------ +/// \brief +//------------------------------------------------------------------------------ xmresult iTest_XM_RETURN_XMRESULT_bool(bool a) { XM_RETURN_XMRESULT(a); @@ -314,6 +345,13 @@ void XmErrorUnitTests::test1() xms::iTest_XM_ENSURE_SUCCESS_T(false); xms::iTest_XM_ENSURE_SUCCESS_T(true); + // XM_ENSURE_FALSE_T + + rv = xms::iTest_XM_ENSURE_FALSE_T(false); + TS_ASSERT_EQUALS(rv, xms::XR_SUCCESS); + rv = xms::iTest_XM_ENSURE_FALSE_T(true); + TS_ASSERT_EQUALS(rv, xms::XR_SUCCESS); + // XM_RETURN_RESULT rv = xms::iTest_XM_RETURN_XMRESULT_bool(true); diff --git a/xmscore/misc/XmError.h b/xmscore/misc/XmError.h index 853a2bc79..6644545c6 100644 --- a/xmscore/misc/XmError.h +++ b/xmscore/misc/XmError.h @@ -427,7 +427,7 @@ inline unsigned int XM_FAILED_impl(xmresult _xmresult) /// \param[in] msg: Log message (\c std::string). /// \param[in] ast: Asserts if true (\c bool). //------------------------------------------------------------------------------ -#define XM_ENSURE_FALSE_T_5(x, ret, lvl, msg, ast) XM_ENSURE_TRUE_T(!(x), ret, lvl, msg, ast) +#define XM_ENSURE_FALSE_T_5(x, ret, lvl, msg, ast) XM_ENSURE_TRUE_T_5(!(x), ret, lvl, msg, ast) //------------------------------------------------------------------------------ /// \brief Throws \a ret if \a x evaluates to \c true, logs, and optionally /// asserts. diff --git a/xmscore/points/pt.cpp b/xmscore/points/pt.cpp index bc90283d0..b4dec8613 100644 --- a/xmscore/points/pt.cpp +++ b/xmscore/points/pt.cpp @@ -160,6 +160,14 @@ void PtUnitTests::testIt() xms::Pt3d pt(1, 1, 3); TS_ASSERT_EQUALS(pt < xms::Pt4d(1, 2, 3, 4), true); } + { + // a larger z decides the order before w is compared + xms::Pt4d pt(1, 1, 5, 0); + TS_ASSERT_EQUALS(pt < xms::Pt4d(1, 1, 3, 9), false); + TS_ASSERT_EQUALS(xms::Pt4d(1, 1, 3, 9) < pt, true); + TS_ASSERT_EQUALS(pt < 1.0, false); + TS_ASSERT_EQUALS(xms::Pt4d(1, 1, 1, 0) < 1.0, true); + } // Test operator+ { diff --git a/xmscore/points/pt.h b/xmscore/points/pt.h index 7c244f395..d43869e4a 100644 --- a/xmscore/points/pt.h +++ b/xmscore/points/pt.h @@ -1795,7 +1795,7 @@ class Pt4 { if (z < a) return 1; - else if (z < a) + else if (z > a) return 0; else { @@ -1870,7 +1870,7 @@ class Pt4 { if (z < a.z) return 1; - else if (z < a.z) + else if (z > a.z) return 0; else { diff --git a/xmscore/python/misc/PyUtils.cpp b/xmscore/python/misc/PyUtils.cpp index 6b4892cef..7cddd8c38 100644 --- a/xmscore/python/misc/PyUtils.cpp +++ b/xmscore/python/misc/PyUtils.cpp @@ -538,7 +538,7 @@ std::pair IntPairFromPyIter(const py::iterable& intpair) if (py::len(pr) != 2) { throw py::type_error("arg must be an 2-tuple"); } else { - std::pair ret(1, 1);//ret(pr[0].cast, pr[1].cast); + std::pair ret(pr[0].cast(), pr[1].cast()); return ret; } } // IntPairFromPyIter diff --git a/xmscore/python/misc/detail/PublicProgressListener.cpp b/xmscore/python/misc/detail/PublicProgressListener.cpp index ab833ea0b..c610a0db7 100644 --- a/xmscore/python/misc/detail/PublicProgressListener.cpp +++ b/xmscore/python/misc/detail/PublicProgressListener.cpp @@ -51,8 +51,15 @@ class PublicProgressListener::impl { ProgressListener::SetListener(m_child); } + ~impl() + { + // m_child points back at the parent being destroyed, so it must not stay + // installed. Leave a listener installed by someone else alone. + if (ProgressListener::GetListener() == m_child) + ProgressListener::SetListener(BSHP()); + } - BSHP m_child; + BSHP m_child; ///< listener installed globally, forwards to parent }; //////////////////////////////////////////////////////////////////////////////// From b3c4db43f57a784f396825ec1dc62ee43cdb97bd Mon Sep 17 00:00:00 2001 From: Bill Dolinar Date: Tue, 6 Oct 2026 13:41:33 -0600 Subject: [PATCH 2/3] Report the real begin stack index and fail vector reads on bad blocks ProgressListener.on_begin_operation_string reported stack index 0, the return value of the C++ base stub, while Progress tags the progress, end and update events that follow with a 1-based depth. The Python class now tracks that depth itself, reports and returns it on begin, and steps it back on end. The Listener.cpp docs and getting_started.rst now say the outermost operation is 1. DaStreamReader::ReadVecInt, ReadVecDbl and ReadVecPt3d ignored the result of ReadBinaryBytes, so a corrupt binary block loaded as a zeroed or partly filled vector and the read reported success. They now return false. ReadBinaryBytes also requires the BINARY_BLOCK keyword and logs an error for a bad header and for truncated data, so every rejection leaves a message. Observer's default handlers are now empty instead of calling super(), which matches the empty C++ base handlers without relying on pybind11's frame check to avoid recursion. PublicProgressListener::impl deletes its copy operations, since its destructor uninstalls the global listener. Tests: every bad-block row checks its logged error, a good block reads with and without a trailing newline, the three vector readers reject a bad block, and the progress listener reports 1 and 2 for nested operations. --- _package/tests/unit_tests/observer_pyt.py | 5 +- .../tests/unit_tests/progress_listener_pyt.py | 24 +++- _package/xms/core/misc/observer.py | 12 +- _package/xms/core/misc/progress_listener.py | 15 ++- pydocs/source/getting_started.rst | 3 +- xmscore/dataio/daStreamIo.cpp | 126 +++++++++++++----- xmscore/dataio/daStreamIo.t.h | 2 + xmscore/python/misc/detail/Listener.cpp | 8 +- .../misc/detail/PublicProgressListener.cpp | 2 + 9 files changed, 145 insertions(+), 52 deletions(-) diff --git a/_package/tests/unit_tests/observer_pyt.py b/_package/tests/unit_tests/observer_pyt.py index b0fb064e3..da4628564 100644 --- a/_package/tests/unit_tests/observer_pyt.py +++ b/_package/tests/unit_tests/observer_pyt.py @@ -219,7 +219,8 @@ def test_observer_without_overrides(self): """ observer = Observer() observer.begin_operation_string('count-jelly-beans') - observer.progress_status(0.5) + # progress_status reaches the progress and timing handlers only when it reports + self.assertTrue(observer.progress_status(0.5)) observer.update_message('21 jelly beans counted so far.') observer.end_operation() @@ -230,7 +231,7 @@ def test_observer_with_partial_overrides(self): observer = PartialObserver() observer.begin_operation_string('count-jelly-beans') observer.progress_status(0.5) - self.assertEqual(0.5, observer.progress[-1]) + self.assertEqual([0.5], observer.progress) observer.update_message('21 jelly beans counted so far.') observer.end_operation() diff --git a/_package/tests/unit_tests/progress_listener_pyt.py b/_package/tests/unit_tests/progress_listener_pyt.py index 3634ce10a..dc894ae21 100644 --- a/_package/tests/unit_tests/progress_listener_pyt.py +++ b/_package/tests/unit_tests/progress_listener_pyt.py @@ -11,12 +11,30 @@ class TestProgressListener(unittest.TestCase): def test_on_begin_operation_string(self): """ - Test that beginning an operation reports the stack index it returns to the call back. + Test that beginning an operation returns and reports stack index 1 for the outermost operation. """ messages = [] listener = set_listener_callback(messages.append) - stack_index = listener.on_begin_operation_string('count-jelly-beans') - self.assertEqual([('begin_operation', stack_index, 'count-jelly-beans')], messages) + self.assertEqual(1, listener.on_begin_operation_string('count-jelly-beans')) + self.assertEqual([('begin_operation', 1, 'count-jelly-beans')], messages) + + def test_nested_operations(self): + """ + Test that the begin stack index tracks how many operations are in flight. + """ + messages = [] + listener = set_listener_callback(messages.append) + listener.on_begin_operation_string('count-jelly-beans') + listener.on_begin_operation_string('count-red-jelly-beans') + listener.on_end_operation(2) + listener.on_begin_operation_string('count-green-jelly-beans') + expected = [ + ('begin_operation', 1, 'count-jelly-beans'), + ('begin_operation', 2, 'count-red-jelly-beans'), + ('end_operation', 2, ''), + ('begin_operation', 2, 'count-green-jelly-beans'), + ] + self.assertEqual(expected, messages) if __name__ == '__main__': diff --git a/_package/xms/core/misc/observer.py b/_package/xms/core/misc/observer.py index 2f2d6c305..69ba3082a 100644 --- a/_package/xms/core/misc/observer.py +++ b/_package/xms/core/misc/observer.py @@ -21,7 +21,7 @@ def on_progress_status(self, percent_complete): Args: percent_complete (float): The percent complete """ - super().on_progress_status(percent_complete) + pass def on_begin_operation_string(self, operation): """ @@ -30,13 +30,13 @@ def on_begin_operation_string(self, operation): Args: operation: Name of the operation being monitored. """ - super().on_begin_operation_string(operation) + pass def on_end_operation(self): """ The end operation event. """ - super().on_end_operation() + pass def on_update_message(self, message): """ @@ -45,7 +45,7 @@ def on_update_message(self, message): Args: message: The new message """ - super().on_update_message(message) + pass def time_remaining_in_seconds(self, remaining_seconds): """ @@ -54,7 +54,7 @@ def time_remaining_in_seconds(self, remaining_seconds): Args: remaining_seconds (Float): The time remaining for the current operation that the class is observing. """ - super().time_remaining_in_seconds(remaining_seconds) + pass def time_elapsed_in_seconds(self, elapsed_seconds): """ @@ -63,4 +63,4 @@ def time_elapsed_in_seconds(self, elapsed_seconds): Args: elapsed_seconds (Float): The elapsed time since the operation began. """ - super().time_elapsed_in_seconds(elapsed_seconds) + pass diff --git a/_package/xms/core/misc/progress_listener.py b/_package/xms/core/misc/progress_listener.py index ca7ad814d..86e748c5f 100644 --- a/_package/xms/core/misc/progress_listener.py +++ b/_package/xms/core/misc/progress_listener.py @@ -27,6 +27,9 @@ def __init__(self): The __init__ function for the Observer class. """ super().__init__() + # The C++ listener numbers operations by how many are in flight, but only + # passes that number to the later events, so the depth is tracked here. + self._stack_depth = 0 def set_update_delay_seconds(self, delay): """ @@ -56,17 +59,21 @@ def on_begin_operation_string(self, operation): operation: Name of the operation being monitored. Returns: - (int): stack index for the operation + (int): stack index for the operation, 1 for the outermost operation """ - stack_index = super().on_begin_operation_string(operation) - self.call_back(('begin_operation', stack_index, operation)) - return stack_index + self._stack_depth += 1 + self.call_back(('begin_operation', self._stack_depth, operation)) + return self._stack_depth def on_end_operation(self, stack_index): """ The end operation event. + + Args: + stack_index (int): stack index for the operation that ended """ self.call_back(('end_operation', stack_index, '')) + self._stack_depth = max(0, self._stack_depth - 1) def on_update_message(self, stack_index, message): """ diff --git a/pydocs/source/getting_started.rst b/pydocs/source/getting_started.rst index 6c613ea73..c84301b11 100644 --- a/pydocs/source/getting_started.rst +++ b/pydocs/source/getting_started.rst @@ -80,7 +80,8 @@ report progress. Subclass it and pass an instance to the consumer: For applications that need to receive progress messages from nested operations, use :class:`ProgressListener `. It exposes a ``stack_index`` for each in-flight operation so that a UI can show a stack of -progress bars. Constructing a ``ProgressListener`` registers it as the active +progress bars. The outermost operation has stack index 1, and each operation +nested inside it adds one. Constructing a ``ProgressListener`` registers it as the active process-wide listener, so subsequent XMS operations route their events through its callback. The :func:`set_listener_callback ` helper builds and diff --git a/xmscore/dataio/daStreamIo.cpp b/xmscore/dataio/daStreamIo.cpp index 22d884776..d05bbd81c 100644 --- a/xmscore/dataio/daStreamIo.cpp +++ b/xmscore/dataio/daStreamIo.cpp @@ -496,7 +496,7 @@ bool DaStreamReader::ReadVecInt(const char* a_name, VecInt& a_vec) a_vec.resize(size); if (size != 0) - ReadBinaryBytes(reinterpret_cast(&a_vec[0]), size * sizeof(VecInt::value_type)); + return ReadBinaryBytes(reinterpret_cast(&a_vec[0]), size * sizeof(VecInt::value_type)); return true; } @@ -518,7 +518,7 @@ bool DaStreamReader::ReadVecDbl(const char* a_name, VecDbl& a_vec) a_vec.resize(size); if (size != 0) - ReadBinaryBytes(reinterpret_cast(&a_vec[0]), size * sizeof(VecDbl::value_type)); + return ReadBinaryBytes(reinterpret_cast(&a_vec[0]), size * sizeof(VecDbl::value_type)); return true; } @@ -540,7 +540,8 @@ bool DaStreamReader::ReadVecPt3d(const char* a_name, VecPt3d& a_vec) { a_vec.resize(size); if (size != 0) - ReadBinaryBytes(reinterpret_cast(&a_vec[0]), size * sizeof(VecPt3d::value_type)); + return ReadBinaryBytes(reinterpret_cast(&a_vec[0]), + size * sizeof(VecPt3d::value_type)); } else { @@ -702,9 +703,10 @@ bool DaStreamReader::ReadBinaryBytes(char* a_dest, long long a_destLength) std::string blockString; int32_t encodedLength = 0; int32_t blockLength = 0; - if (!ReadString(blockString) || !ReadInt(encodedLength) || !ReadInt(blockLength) || - !NextLine()) + if (!ReadString(blockString) || blockString != "BINARY_BLOCK" || !ReadInt(encodedLength) || + !ReadInt(blockLength) || !NextLine()) { + XM_LOG(xmlog::error, "Unable to read file. Invalid binary block header.") return false; } @@ -712,14 +714,17 @@ bool DaStreamReader::ReadBinaryBytes(char* a_dest, long long a_destLength) if (encodedLength < 0 || encodedLength > maxEncodeLength || blockLength <= 0 || blockLength > a_destLength) { - XM_LOG(xmlog::error, "Unable to read file. Invalid binary block length."); + XM_LOG(xmlog::error, "Unable to read file. Invalid binary block length.") return false; } // read and decode m_impl->m_inStream.read(encoded.get(), encodedLength); if (m_impl->m_inStream.gcount() != encodedLength || !NextLine()) + { + XM_LOG(xmlog::error, "Unable to read file. Binary block data is truncated.") return false; + } auto compressedLength = iBase64Decode(encoded.get(), encodedLength, compressed.get()); if (compressedLength < 0) @@ -2326,29 +2331,37 @@ void DaReaderWriterIoUnitTests::testReadWriteBinaryArrays() //------------------------------------------------------------------------------ void DaReaderWriterIoUnitTests::testReadBinaryBytesRejectsBadBlocks() { + // The first block written by testReadWriteBinaryArrays: 74 base64 characters + // holding 120 bytes. + const std::string goodHeader = "BINARY_BLOCK 74 120\n"; const std::string goodData = "eAENw4UNwDAAAKDO3d3+f3OQEEIIkbGJqZm5haWVtY2tnb2Do5Ozi6ubu4enl7ePr58/RrQBtA\n"; int32_t maxEncodeLength = iBase64EncodeSize(static_cast(compressBound(MAX_BLOCK_SIZE))); + const std::string badHeader = "Invalid binary block header."; const std::string badLength = "Invalid binary block length."; + const std::string truncated = "Binary block data is truncated."; const std::string badBase64Length = "Invalid base64 data length."; const std::string badBase64Char = "Invalid base64 character."; const std::string badCompressed = "Unable to uncompress data."; // Each bad input and the error logged by the check that should reject it. - // An empty error means the input is rejected without logging. + // A header with one bad field is written out because it is the input under + // test. const std::vector> badInputs = { - {"", ""}, // no header - {"BINARY_BLOCK\n", ""}, // truncated header - {"BINARY_BLOCK 74\n", ""}, // no block length - {"BINARY_BLOCK 999999 120\n" + goodData, badLength}, // encoded length too big - {"BINARY_BLOCK -1 120\n" + goodData, badLength}, // negative encoded length - {"BINARY_BLOCK 74 0\n" + goodData, badLength}, // zero block length - {"BINARY_BLOCK 74 -5\n" + goodData, badLength}, // negative block length - {"BINARY_BLOCK 74 121\n" + goodData, badLength}, // bigger than destination - {"BINARY_BLOCK 74 120\neAENw4UNwDAAAKDO\n", ""}, // truncated data + {"", badHeader}, // no header + {"BINARY_BLOCK\n", badHeader}, // truncated header + {"BINARY_BLOCK 74\n", badHeader}, // no block length + {"BINARY_BLOCK 74 120", badHeader}, // header ends the file + {"BLOCK 74 120\n" + goodData, badHeader}, // wrong block name + {"BINARY_BLOCK 999999 120\n" + goodData, badLength}, // encoded length too big + {"BINARY_BLOCK -1 120\n" + goodData, badLength}, // negative encoded length + {"BINARY_BLOCK 74 0\n" + goodData, badLength}, // zero block length + {"BINARY_BLOCK 74 -5\n" + goodData, badLength}, // negative block length + {"BINARY_BLOCK 74 121\n" + goodData, badLength}, // bigger than destination + {goodHeader + "eAENw4UNwDAAAKDO\n", truncated}, // truncated data {"BINARY_BLOCK 75 120\n" + goodData + "\n", badBase64Char}, // newline in the data - {"BINARY_BLOCK 5 120\nAAAAA\n", badBase64Length}, // not a base64 length - {"BINARY_BLOCK 3 120\nA==\n", badBase64Length}, // not a base64 length + {"BINARY_BLOCK 5 120\nAAAAA\n", badBase64Length}, // not a base64 length + {"BINARY_BLOCK 3 120\nA==\n", badBase64Length}, // not a base64 length // longest allowed block without base64 padding decodes past the // compressed length; must fail to uncompress without overflowing {"BINARY_BLOCK " + std::to_string(maxEncodeLength) + " 120\n" + @@ -2369,24 +2382,73 @@ void DaReaderWriterIoUnitTests::testReadBinaryBytesRejectsBadBlocks() } // a good block that is bigger than the destination - { - std::istringstream inputStream("BINARY_BLOCK 74 120\n" + goodData); - DaStreamReader reader(inputStream); - std::vector dest(120); - TS_ASSERT(!reader.ReadBinaryBytes(&dest[0], 60)); - } - - // the good block still reads - std::istringstream inputStream("BINARY_BLOCK 74 120\n" + goodData); + std::istringstream inputStream(goodHeader + goodData); DaStreamReader reader(inputStream); - VecInt values(30); - TS_ASSERT(reader.ReadBinaryBytes((char*)&values[0], 120)); + std::vector dest(120); + TS_ASSERT(!reader.ReadBinaryBytes(&dest[0], 60)); + std::string errors = XmLog::Instance().GetAndClearStackStr(); + TSM_ASSERT("logged: " + errors, errors.find(badLength) != std::string::npos); +} // DaReaderWriterIoUnitTests::testReadBinaryBytesRejectsBadBlocks +//------------------------------------------------------------------------------ +/// \brief Test that DaStreamReader::ReadBinaryBytes reads a good block whether +/// or not its data ends with a line ending. +//------------------------------------------------------------------------------ +void DaReaderWriterIoUnitTests::testReadBinaryBytesReadsGoodBlock() +{ + // The first block written by testReadWriteBinaryArrays: the ints 0 to 29. + const std::string goodBlock = + "BINARY_BLOCK 74 120\n" + "eAENw4UNwDAAAKDO3d3+f3OQEEIIkbGJqZm5haWVtY2tnb2Do5Ozi6ubu4enl7ePr58/RrQBtA"; VecInt expected(30); std::iota(expected.begin(), expected.end(), 0); - TS_ASSERT_EQUALS(expected, values); - + const std::vector> inputs = { + {"with line ending", goodBlock + "\n"}, + {"at end of file", goodBlock}, + }; + for (const auto& [label, input] : inputs) + { + std::istringstream inputStream(input); + DaStreamReader reader(inputStream); + VecInt values(30); + TSM_ASSERT(label, reader.ReadBinaryBytes((char*)&values[0], 120)); + TSM_ASSERT_EQUALS(label, expected, values); + } +} // DaReaderWriterIoUnitTests::testReadBinaryBytesReadsGoodBlock +//------------------------------------------------------------------------------ +/// \brief Test that DaStreamReader::ReadVecInt, ReadVecDbl, and ReadVecPt3d +/// return false when their binary block is malformed. +//------------------------------------------------------------------------------ +void DaReaderWriterIoUnitTests::testReadBinaryVecRejectsBadBlock() +{ + // 120 bytes, which is 30 ints, 15 doubles, or 5 points, with a '*' in the + // base64 data + const std::string badBlock = + "BINARY_BLOCK 74 120\n" + "eAENw4UNwDAAAKDO3d3+f3OQEEIIkbGJqZm5haWVtY2tnb2Do5Ozi6ubu4enl7ePr58/RrQB*A\n"; + const char* name = "VECTOR_NAME"; + const bool useBinaryArrays = true; XmLog::Instance().GetAndClearStackStr(); -} // DaReaderWriterIoUnitTests::testReadBinaryBytesRejectsBadBlocks + { + std::istringstream inputStream(std::string(name) + " 30\n" + badBlock); + DaStreamReader reader(inputStream, useBinaryArrays); + VecInt values; + TS_ASSERT(!reader.ReadVecInt(name, values)); + } + { + std::istringstream inputStream(std::string(name) + " 15\n" + badBlock); + DaStreamReader reader(inputStream, useBinaryArrays); + VecDbl values; + TS_ASSERT(!reader.ReadVecDbl(name, values)); + } + { + std::istringstream inputStream(std::string(name) + " 5\n" + badBlock); + DaStreamReader reader(inputStream, useBinaryArrays); + VecPt3d values; + TS_ASSERT(!reader.ReadVecPt3d(name, values)); + } + std::string errors = XmLog::Instance().GetAndClearStackStr(); + TSM_ASSERT("logged: " + errors, errors.find("Invalid base64 character.") != std::string::npos); +} // DaReaderWriterIoUnitTests::testReadBinaryVecRejectsBadBlock //------------------------------------------------------------------------------ /// \brief Test that DaStreamWriter::SetBinaryBlockSize ignores a non-positive /// size instead of making WriteBinaryBytes loop forever. diff --git a/xmscore/dataio/daStreamIo.t.h b/xmscore/dataio/daStreamIo.t.h index 73db7585f..f74f0bc68 100644 --- a/xmscore/dataio/daStreamIo.t.h +++ b/xmscore/dataio/daStreamIo.t.h @@ -65,6 +65,8 @@ class DaReaderWriterIoUnitTests : public CxxTest::TestSuite void testReadWriteLineParts(); void testReadWriteBinaryArrays(); void testReadBinaryBytesRejectsBadBlocks(); + void testReadBinaryBytesReadsGoodBlock(); + void testReadBinaryVecRejectsBadBlock(); void testSetBinaryBlockSizeRejectsNonPositive(); void testLineBeginsWith(); }; diff --git a/xmscore/python/misc/detail/Listener.cpp b/xmscore/python/misc/detail/Listener.cpp index a2203c2eb..b59764393 100644 --- a/xmscore/python/misc/detail/Listener.cpp +++ b/xmscore/python/misc/detail/Listener.cpp @@ -98,7 +98,7 @@ void Listener::SetUpdateDelaySeconds(int a_delay) } // Listener::SetUpdateDelaySeconds //------------------------------------------------------------------------------ /// \brief Listen to progress status -/// \param[in] a_stackIndex: the ID for progress stack (0 for first) +/// \param[in] a_stackIndex: the ID for progress stack (1 for the outermost) /// \param[in] a_fractionComplete: amount complete from 0.0 to 1.0 //------------------------------------------------------------------------------ void Listener::OnProgressStatus(int a_stackIndex, double a_fractionComplete) @@ -114,7 +114,7 @@ void Listener::OnProgressStatus(int a_stackIndex, double a_fractionComplete) //------------------------------------------------------------------------------ /// \brief Listen to when operation begins /// \param[in] a_operation: the name of the operation -/// \return the ID for progress stack (0 for first) +/// \return the ID for progress stack (1 for the outermost) //------------------------------------------------------------------------------ int Listener::OnBeginOperationString(const std::string& a_operation) { @@ -125,7 +125,7 @@ int Listener::OnBeginOperationString(const std::string& a_operation) } // Listener::OnBeginOperationString //------------------------------------------------------------------------------ /// \brief Listen to when operation ends -/// \param[in] a_stackIndex: the ID for progress stack (0 for first) +/// \param[in] a_stackIndex: the ID for progress stack (1 for the outermost) //------------------------------------------------------------------------------ void Listener::OnEndOperation(int a_stackIndex) { @@ -136,7 +136,7 @@ void Listener::OnEndOperation(int a_stackIndex) } // Listener::OnEndOperation //------------------------------------------------------------------------------ /// \brief Listen to when operation ends -/// \param[in] a_stackIndex: the ID for progress stack (0 for first) +/// \param[in] a_stackIndex: the ID for progress stack (1 for the outermost) /// \param[in] a_message: the new message for an operation //------------------------------------------------------------------------------ void Listener::OnUpdateMessage(int a_stackIndex, const std::string& a_message) diff --git a/xmscore/python/misc/detail/PublicProgressListener.cpp b/xmscore/python/misc/detail/PublicProgressListener.cpp index c610a0db7..6c10aa4ac 100644 --- a/xmscore/python/misc/detail/PublicProgressListener.cpp +++ b/xmscore/python/misc/detail/PublicProgressListener.cpp @@ -58,6 +58,8 @@ class PublicProgressListener::impl if (ProgressListener::GetListener() == m_child) ProgressListener::SetListener(BSHP()); } + impl(const impl&) = delete; + impl& operator=(const impl&) = delete; BSHP m_child; ///< listener installed globally, forwards to parent }; From b9625630c6f69bbb879b8e0cca609945989d2c92 Mon Sep 17 00:00:00 2001 From: Bill Dolinar Date: Tue, 6 Oct 2026 13:55:15 -0600 Subject: [PATCH 3/3] Correct the progress stack-index docs and disable listener copies The public ProgressListener docs said the stack index is 0 for the first operation, but Listener and the test mock both return 1 for the outermost, as getting_started.rst already says. The docs now say 1 and note that an operation begun before a listener was installed reports 0. The OnUpdateMessage briefs no longer describe the end event. PublicProgressListener and Listener delete a raw impl pointer in their destructors, so a copy of either would delete it twice. Both now disable copying. The Python ProgressListener documents that overrides of the begin and end handlers must call super(), and a new test pins that an end with no matching begin leaves the next operation at stack index 1. The binary vector test checks the log after each reader instead of once at the end. --- _package/tests/unit_tests/progress_listener_pyt.py | 9 +++++++++ _package/xms/core/misc/progress_listener.py | 7 ++++++- pydocs/source/getting_started.rst | 6 +++--- xmscore/dataio/daStreamIo.cpp | 9 +++++++-- xmscore/misc/Progress.cpp | 10 +++++----- xmscore/misc/Progress.h | 14 +++++++++----- xmscore/python/misc/PublicProgressListener.h | 2 ++ xmscore/python/misc/detail/Listener.cpp | 2 +- xmscore/python/misc/detail/Listener.h | 2 ++ 9 files changed, 44 insertions(+), 17 deletions(-) diff --git a/_package/tests/unit_tests/progress_listener_pyt.py b/_package/tests/unit_tests/progress_listener_pyt.py index dc894ae21..1ea74be4d 100644 --- a/_package/tests/unit_tests/progress_listener_pyt.py +++ b/_package/tests/unit_tests/progress_listener_pyt.py @@ -36,6 +36,15 @@ def test_nested_operations(self): ] self.assertEqual(expected, messages) + def test_unmatched_end_operation(self): + """ + Test that ending an operation that never began leaves the next operation at stack index 1. + """ + # A Progress made before the listener was installed ends with stack index 0. + listener = set_listener_callback(lambda msg: None) + listener.on_end_operation(0) + self.assertEqual(1, listener.on_begin_operation_string('count-jelly-beans')) + if __name__ == '__main__': unittest.main() diff --git a/_package/xms/core/misc/progress_listener.py b/_package/xms/core/misc/progress_listener.py index 86e748c5f..daa7872ec 100644 --- a/_package/xms/core/misc/progress_listener.py +++ b/_package/xms/core/misc/progress_listener.py @@ -55,11 +55,14 @@ def on_begin_operation_string(self, operation): """ The operation string. + An override must call super() so the stack index stays right. + Args: operation: Name of the operation being monitored. Returns: - (int): stack index for the operation, 1 for the outermost operation + (int): stack index for the operation, 1 for the outermost operation. The C++ + listener keeps its own count and ignores this value. """ self._stack_depth += 1 self.call_back(('begin_operation', self._stack_depth, operation)) @@ -69,6 +72,8 @@ def on_end_operation(self, stack_index): """ The end operation event. + An override must call super() so the next operation gets the right stack index. + Args: stack_index (int): stack index for the operation that ended """ diff --git a/pydocs/source/getting_started.rst b/pydocs/source/getting_started.rst index c84301b11..7b8a46d9b 100644 --- a/pydocs/source/getting_started.rst +++ b/pydocs/source/getting_started.rst @@ -81,9 +81,9 @@ For applications that need to receive progress messages from nested operations, use :class:`ProgressListener `. It exposes a ``stack_index`` for each in-flight operation so that a UI can show a stack of progress bars. The outermost operation has stack index 1, and each operation -nested inside it adds one. Constructing a ``ProgressListener`` registers it as the active -process-wide listener, so subsequent XMS operations route their events through -its callback. The :func:`set_listener_callback +nested inside it adds one. Constructing a ``ProgressListener`` registers it as +the active process-wide listener, so subsequent XMS operations route their +events through its callback. The :func:`set_listener_callback ` helper builds and returns one wired to a single callable: diff --git a/xmscore/dataio/daStreamIo.cpp b/xmscore/dataio/daStreamIo.cpp index d05bbd81c..5867d16ea 100644 --- a/xmscore/dataio/daStreamIo.cpp +++ b/xmscore/dataio/daStreamIo.cpp @@ -2427,27 +2427,32 @@ void DaReaderWriterIoUnitTests::testReadBinaryVecRejectsBadBlock() "eAENw4UNwDAAAKDO3d3+f3OQEEIIkbGJqZm5haWVtY2tnb2Do5Ozi6ubu4enl7ePr58/RrQB*A\n"; const char* name = "VECTOR_NAME"; const bool useBinaryArrays = true; + const std::string badBase64Char = "Invalid base64 character."; XmLog::Instance().GetAndClearStackStr(); { std::istringstream inputStream(std::string(name) + " 30\n" + badBlock); DaStreamReader reader(inputStream, useBinaryArrays); VecInt values; TS_ASSERT(!reader.ReadVecInt(name, values)); + std::string errors = XmLog::Instance().GetAndClearStackStr(); + TSM_ASSERT("ReadVecInt logged: " + errors, errors.find(badBase64Char) != std::string::npos); } { std::istringstream inputStream(std::string(name) + " 15\n" + badBlock); DaStreamReader reader(inputStream, useBinaryArrays); VecDbl values; TS_ASSERT(!reader.ReadVecDbl(name, values)); + std::string errors = XmLog::Instance().GetAndClearStackStr(); + TSM_ASSERT("ReadVecDbl logged: " + errors, errors.find(badBase64Char) != std::string::npos); } { std::istringstream inputStream(std::string(name) + " 5\n" + badBlock); DaStreamReader reader(inputStream, useBinaryArrays); VecPt3d values; TS_ASSERT(!reader.ReadVecPt3d(name, values)); + std::string errors = XmLog::Instance().GetAndClearStackStr(); + TSM_ASSERT("ReadVecPt3d logged: " + errors, errors.find(badBase64Char) != std::string::npos); } - std::string errors = XmLog::Instance().GetAndClearStackStr(); - TSM_ASSERT("logged: " + errors, errors.find("Invalid base64 character.") != std::string::npos); } // DaReaderWriterIoUnitTests::testReadBinaryVecRejectsBadBlock //------------------------------------------------------------------------------ /// \brief Test that DaStreamWriter::SetBinaryBlockSize ignores a non-positive diff --git a/xmscore/misc/Progress.cpp b/xmscore/misc/Progress.cpp index fad086489..555a33cbf 100644 --- a/xmscore/misc/Progress.cpp +++ b/xmscore/misc/Progress.cpp @@ -165,7 +165,7 @@ class MockProgressListener : public ProgressListener //------------------------------------------------------------------------------ /// \brief Listen to progress status - /// \param[in] a_stackIndex: the ID for progress stack (0 for first) + /// \param[in] a_stackIndex: the ID for progress stack (1 for the outermost) /// \param[in] a_fractionComplete: amount complete from 0.0 to 1.0 //------------------------------------------------------------------------------ void OnProgressStatus(int a_stackIndex, double a_fractionComplete) override @@ -177,7 +177,7 @@ class MockProgressListener : public ProgressListener //------------------------------------------------------------------------------ /// \brief Listen to when operation begins /// \param[in] a_operation: the name of the operation - /// \return the ID for progress stack (0 for first) + /// \return the ID for progress stack (1 for the outermost) //------------------------------------------------------------------------------ int OnBeginOperationString(const std::string& a_operation) override { @@ -190,7 +190,7 @@ class MockProgressListener : public ProgressListener //------------------------------------------------------------------------------ /// \brief Listen to when operation ends - /// \param[in] a_stackIndex: the ID for progress stack (0 for first) + /// \param[in] a_stackIndex: the ID for progress stack (1 for the outermost) //------------------------------------------------------------------------------ void OnEndOperation(int a_stackIndex) override { @@ -201,8 +201,8 @@ class MockProgressListener : public ProgressListener } //------------------------------------------------------------------------------ - /// \brief Listen to when operation ends - /// \param[in] a_stackIndex: the ID for progress stack (0 for first) + /// \brief Listen to a new message for an operation + /// \param[in] a_stackIndex: the ID for progress stack (1 for the outermost) /// \param[in] a_message: the new message for an operation //------------------------------------------------------------------------------ void OnUpdateMessage(int a_stackIndex, const std::string& a_message) override diff --git a/xmscore/misc/Progress.h b/xmscore/misc/Progress.h index 85c0c8c17..2e2ab4917 100644 --- a/xmscore/misc/Progress.h +++ b/xmscore/misc/Progress.h @@ -49,27 +49,31 @@ class Progress /// \class ProgressListener /// \brief Listen to progress reported from Progress class +/// +/// A stack index is the value OnBeginOperationString returned for an operation, +/// 1 for the outermost. An operation that began before a listener was installed +/// reports stack index 0. class ProgressListener { public: virtual ~ProgressListener(); /// \brief Listen to progress status - /// \param[in] a_stackIndex: the ID for progress stack (0 for first) + /// \param[in] a_stackIndex: the ID for progress stack (1 for the outermost) /// \param[in] a_fractionComplete: amount complete from 0.0 to 1.0 virtual void OnProgressStatus(int a_stackIndex, double a_fractionComplete) = 0; /// \brief Listen to when operation begins /// \param[in] a_operation: the name of the operation - /// \return the ID for progress stack (0 for first) + /// \return the ID for progress stack (1 for the outermost) virtual int OnBeginOperationString(const std::string& a_operation) = 0; /// \brief Listen to when operation ends - /// \param[in] a_stackIndex: the ID for progress stack (0 for first) + /// \param[in] a_stackIndex: the ID for progress stack (1 for the outermost) virtual void OnEndOperation(int a_stackIndex) = 0; - /// \brief Listen to when operation ends - /// \param[in] a_stackIndex: the ID for progress stack (0 for first) + /// \brief Listen to a new message for an operation + /// \param[in] a_stackIndex: the ID for progress stack (1 for the outermost) /// \param[in] a_message: the new message for an operation virtual void OnUpdateMessage(int a_stackIndex, const std::string& a_message) = 0; diff --git a/xmscore/python/misc/PublicProgressListener.h b/xmscore/python/misc/PublicProgressListener.h index 12b942efd..478ad29ea 100644 --- a/xmscore/python/misc/PublicProgressListener.h +++ b/xmscore/python/misc/PublicProgressListener.h @@ -13,6 +13,7 @@ // 3. Standard Library Headers // 4. External Library Headers +#include #include #include @@ -39,6 +40,7 @@ class PublicProgressListener virtual void on_update_message(int a_stackIndex, const std::string& a_message) {} private: + XM_DISALLOW_COPY_AND_ASSIGN(PublicProgressListener) class impl; impl *m_p; }; diff --git a/xmscore/python/misc/detail/Listener.cpp b/xmscore/python/misc/detail/Listener.cpp index b59764393..abd0ee1c2 100644 --- a/xmscore/python/misc/detail/Listener.cpp +++ b/xmscore/python/misc/detail/Listener.cpp @@ -135,7 +135,7 @@ void Listener::OnEndOperation(int a_stackIndex) m_p->m_operations.pop_back(); } // Listener::OnEndOperation //------------------------------------------------------------------------------ -/// \brief Listen to when operation ends +/// \brief Listen to a new message for an operation /// \param[in] a_stackIndex: the ID for progress stack (1 for the outermost) /// \param[in] a_message: the new message for an operation //------------------------------------------------------------------------------ diff --git a/xmscore/python/misc/detail/Listener.h b/xmscore/python/misc/detail/Listener.h index 3f9cbe777..b531f286d 100644 --- a/xmscore/python/misc/detail/Listener.h +++ b/xmscore/python/misc/detail/Listener.h @@ -13,6 +13,7 @@ // 3. Standard Library Headers // 4. External Library Headers +#include #include // 5. Shared Headers @@ -40,6 +41,7 @@ class Listener : public xms::ProgressListener void OnUpdateMessage(int a_stackIndex, const std::string& a_message) override; private: + XM_DISALLOW_COPY_AND_ASSIGN(Listener) class impl; impl* m_p; };