View Issue Details

IDProjectCategoryView StatusLast Update
0030697Open CASCADEOCCT:DRAWpublic2020-12-02 17:11
Reporterkgv Assigned Tobugmaster  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
Product Version6.8.0 
Target Version7.5.0Fixed in Version7.5.0 
Summary0030697: Draw Harness - Draw_Printer should not be set to Message::DefaultMessenger() by default
DescriptionCurrently Draw Harness removes std::cout printer from Message::DefaultMessenger() and appends Draw_Printer instead. Draw_Printer redirects output to Tcl command result, which results in the following issues:
- DefaultMessenger() is a global callback which can be used by any OCCT code, not exactly designed for Tcl output of particular command. This might include errors, failures and warnings, so that command output might become unexpectedly broken. Example - vreadpixel command which uses a workaround clearing Tcl output to avoid breaking tests (as result, messages from TKOpenGl are discarded and might remain unnoticed).
- Feeding Tcl interpreter is not thread safe, while DefaultMessenger() can be used for emitting messages from working threads resulting in Draw Harness crash.
- Draw_Printer accumulates messages internally and prints them into console only after command execution completion. This is annoying behavior for a long process reporting some intermediate messages - so that important problems can be seen only at the very end of command.

It is proposed reverted patch making Draw_Printer as default printer ad Draw Harness start and reconsider its usage in particular commands. The origin of Draw_Printer are Data Exchange plugins, so that them should be checked carefully.
Steps To ReproduceNot required
TagsNo tags attached.
Test case number

Relationships

related to 0024644 closedbugmaster Draw_Printer - provide the way to control messages gravity filter 
related to 0025748 closedabv Foundation Classes - Parallel version of progress indicator 
related to 0031036 closedapn Foundation Classes, Message_PrinterOStream - add option printing colored text into console 

Activities

git

2019-05-06 23:12

administrator   ~0084205

Branch CR30697 has been created by kgv.

SHA-1: 80145f0110178da4585ea764914f24bfef7abbac


Detailed log of new commits:

Author: kgv
Date: Mon May 6 23:05:13 2019 +0300

    0030697: Draw Harness - Draw_Printer should not be set to Message::DefaultMessenger() by default

git

2019-11-06 11:30

administrator   ~0088747

Branch CR30697 has been updated by mzernova.

SHA-1: 9f6e5d82deb44c9cafd530d15775fb30aaf641cc


Detailed log of new commits:

Author: mzernova
Date: Thu Oct 31 16:03:29 2019 +0300

    Fixed some bugs that occurred when using the default std::cout from Message::DefaultMessenger() instead of Draw_Printer

kgv

2019-11-06 11:47

developer   ~0088748

@@ -147,9 +147,9 @@ Standard_Boolean Draw_ProgressIndicator::Show(const Standard_Boolean force)
   }
 
   // Print textual progress info
-  if ( myTextMode )
-    Message::DefaultMessenger()->Send (text, Message_Info);
-  
+  if (myTextMode && myDraw)
+    (*(Draw_Interpretor*)myDraw) << text << "\n";

This should be a new dedicated option - only for test cases of progress indicator itself (their is a couple of them).
Default implementation of Text Mode should print into std::cout (not Messenger) so that progress will be seen during task.

kgv

2019-11-06 11:49

developer   ~0088749

+  const Handle(Message_Messenger)& aMsgMgr = Message::DefaultMessenger();
+  if (!aMsgMgr.IsNull())
+  {
+    aMsgMgr->RemovePrinters (STANDARD_TYPE (Message_PrinterOStream));
+    aMsgMgr->RemovePrinters (STANDARD_TYPE (Draw_Printer));
+    aMsgMgr->AddPrinter (new Draw_Printer (di));
+  }

It is better copying the whole list of printers and adding them back.

git

2019-11-07 16:22

administrator   ~0088777

Branch CR30697 has been updated by mzernova.

SHA-1: 72bc28711fb2e2ede6204eb84a8ae4b79c7abc2e


Detailed log of new commits:

Author: mzernova
Date: Thu Nov 7 14:12:31 2019 +0300

    remarks from kgv

git

2019-11-07 16:22

administrator   ~0088778

Branch CR30697_1 has been created by mzernova.

SHA-1: b1b14ad2d234606961b0f8757c1e586ad5aedf64


Detailed log of new commits:

Author: kgv
Date: Mon May 6 23:05:13 2019 +0300

    0030697: Draw Harness - Draw_Printer should not be set to Message::DefaultMessenger() by default
    
    Fixed bugs that occurred when using the default std::cout from Message::DefaultMessenger() instead of Draw_Printer

git

2019-11-08 20:12

administrator   ~0088816

Branch CR30697 has been updated by mzernova.

SHA-1: 14286674ea365a12d3ac873639dba2d7d243b5b5


Detailed log of new commits:

Author: mzernova
Date: Fri Nov 8 14:10:14 2019 +0300

    fixed some problems caused by changes in Draw_ProgressIndicator::Show method

git

2019-11-08 20:12

administrator   ~0088817

Branch CR30697_1 has been updated forcibly by mzernova.

SHA-1: 2fdf1fe2aac438c62fc4c6d2f29c5c7a023e8942

kgv

2019-11-08 20:15

developer   ~0088818

+  aProgress->SetTclMode (Standard_True);

I think "SetTclOutput" would be better than "SetTclMode".

> fixed some problems caused by changes in Draw_ProgressIndicator::Show method
> @@ -454,6 +454,7 @@ static Standard_Integer sewing (Draw_Interpretor& theDi,
> @@ -484,6 +484,7 @@ static Standard_Integer fixshape (Draw_Interpretor& di,
> @@ -312,6 +312,7 @@ static Standard_Integer createmesh
Could you please refer to test cases where this modification is necessary?
The location of these modifications looks suspicion to me.

git

2019-11-08 20:21

administrator   ~0088819

Branch CR30697 has been updated by mzernova.

SHA-1: 1342dab2e895ec545aff0fcf48753d90d167079c


Detailed log of new commits:

Author: mzernova
Date: Fri Nov 8 20:18:54 2019 +0300

    A dedicated option was added to Draw_ProgressIndicator, for outputting data to the tcl when performing tests

mzernova

2019-11-14 10:41

developer   ~0088953

test bugs modalg_5 bug22747
test bugs moddata_2 bug22572
test bugs moddata_2 bug22746_1
test bugs moddata_2 bug22746_2
test bugs moddata_2 bug22746_3

kgv

2019-11-14 10:55

developer   ~0088954

These are tests for progress indicator used within the algorithm.
Expected change is modifying test case to use "XProgress -tclOutput" rather than modifying "sewing" command.
# Progress indicator in sewing algorithm
vinit
XProgress -t
set List1 [sewing result 0.1 a]
if { [string compare $List1 ""] != 0 } {

git

2019-11-14 15:41

administrator   ~0088962

Branch CR30697 has been updated by mzernova.

SHA-1: b0eb3f1777d43168d763c7f455580a4d43c2914a


Detailed log of new commits:

Author: mzernova
Date: Thu Nov 14 15:15:10 2019 +0300

    Added -tclOutput parameter to XProgress command

git

2019-11-14 15:41

administrator   ~0088963

Branch CR30697_1 has been updated forcibly by mzernova.

SHA-1: 05f1e2245b32b4fc69eb4bf8a6673b0b61d44fa0

mzernova

2019-11-14 17:39

developer   ~0088966

The patch CR30697 is ready to review

mzernova

2019-11-14 17:44

developer   ~0088969

http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR30697_1-master-mzernova

kgv

2019-11-14 17:46

developer   ~0088970

+    if (!strcmp (argv[i], "-tclOutput"))

It is preferred used case-insensitive comparison of Tcl command arguments.
> TCollection_AsciiString anArgCase (argv[i]);
> anArgCase.LowerCase();
> if (anArgCase == "-tcloutput")

+Standard_Boolean &Draw_ProgressIndicator::DefaultTclOutput ()

Standard_Boolean& Draw_ProgressIndicator::DefaultTclOutput()

+void Draw_ProgressIndicator::SetTclOutput (const Standard_Boolean theTclOutput)
+{
...
+Standard_Boolean Draw_ProgressIndicator::GetTclOutput() const
+{
+  return myTclOutput;

Inline definition would be better.

+      (*(Draw_Interpretor*) myDraw) << aText.str().c_str() << "\n";

myDraw declaration can be improved to avoid type cast.

kgv

2019-11-14 17:54

developer   ~0088971

@@ -136,7 +138,9 @@ void BOPTest::ReportAlerts(const Handle(Message_Report)& theReport)
       }
 
       // output message with list of shapes
-      Message::DefaultMessenger()->Send (aText, anAlertTypes[iGravity]);
+      Draw_Interpretor& aDrawInterpretor = Draw::GetInterpretor();
+      Handle(Message_Messenger) aMessenger = new Message_Messenger (new Draw_Printer (aDrawInterpretor));
+      aMessenger->Send (aText, anAlertTypes[iGravity]);

Could be just "aDrawInterpretor << aText" in this context.

--- a/src/XSDRAWIGES/XSDRAWIGES.cxx
+++ b/src/XSDRAWIGES/XSDRAWIGES.cxx
@@ -106,6 +106,7 @@ static Standard_Integer igesbrep (Draw_Interpretor& di, Standard_Integer argc, c
 
   // Progress indicator
   Handle(Draw_ProgressIndicator) progress = new Draw_ProgressIndicator ( di, 1 );
+  progress->SetTclOutput (Standard_True);
...
--- a/src/XSDRAWSTEP/XSDRAWSTEP.cxx
+++ b/src/XSDRAWSTEP/XSDRAWSTEP.cxx
@@ -96,6 +96,7 @@ static Standard_Integer stepread (Draw_Interpretor& di/*theCommands*/, Standard_
 
   // Progress indicator
   Handle(Draw_ProgressIndicator) progress = new Draw_ProgressIndicator ( di, 1 );
+  progress->SetTclOutput (Standard_True);

These are unexpected - I suppose that only specific test cases should be "XProgress -tclOutput" if there are any.

git

2019-11-15 11:06

administrator   ~0088983

Branch CR30697_1 has been updated forcibly by mzernova.

SHA-1: c68f6ba996451f5aa9455669ca4f920d6acf8f31

git

2019-11-15 11:48

administrator   ~0088987

Branch CR30697_1 has been updated forcibly by mzernova.

SHA-1: 7f81e983b549ff9019d3778d8dc6641c30f68a2e

git

2019-11-15 11:53

administrator   ~0088988

Branch CR30697_1 has been updated forcibly by mzernova.

SHA-1: 64845d8b74c4df6f70770ce3400d932b5452c6ac

kgv

2019-11-15 12:01

developer   ~0088989

--- a/src/Draw/Draw_VariableCommands.cxx
+++ b/src/Draw/Draw_VariableCommands.cxx
@@ -156,6 +156,7 @@ static Standard_Integer save(Draw_Interpretor& di,
+    progress->SetTclOutput (Standard_True);
@@ -221,6 +222,7 @@ static Standard_Integer restore(Draw_Interpretor& di,      
+    progress->SetTclOutput (Standard_True);

Same remark for these two commands.

git

2019-11-15 12:53

administrator   ~0088993

Branch CR30697_1 has been updated forcibly by mzernova.

SHA-1: 9d15f4d254c4e676cd3b455506dd92e2f7c2bf95

git

2019-11-15 19:09

administrator   ~0089016

Branch CR30697_1 has been updated forcibly by mzernova.

SHA-1: 9a09616c58ea76a9944c6b565155ba71f837e767

git

2019-11-18 10:20

administrator   ~0089039

Branch CR30697_1 has been updated forcibly by mzernova.

SHA-1: aaf2ac08860940f0d0972e78646feb406d9527a5

kgv

2019-11-18 11:58

developer   ~0089045

+  //! Sets tcl output mode (on/off)
+  Standard_EXPORT inline void SetTclOutput (const Standard_Boolean theTclOutput)
+  {
+    myTclOutput = theTclOutput;
+  }
+
+  //! Gets tcl output mode (on/off)
+  Standard_EXPORT inline Standard_Boolean GetTclOutput() const
+  {
+    return myTclOutput;
+  }

Unexpected Standard_EXPORT within inline functions.

+    aPrinters.Append (aMsgMgr->ChangePrinters());
+    for (Message_SequenceOfPrinters::Iterator aPrinterIter (aMsgMgr->Printers());
+         aPrinterIter.More(); aPrinterIter.Next())
+    {
+      aMsgMgr->RemovePrinter (aPrinterIter.Value());
+    }

It would be more natural iterating through copied aPrinters instead of modified sequence.

+  const Handle(Message_Messenger)& aMsgMgr = Message::DefaultMessenger();
+  Message_SequenceOfPrinters aPrinters;
+  if (!aMsgMgr.IsNull())

NULL checks are redundant here - in no event Message::DefaultMessenger() should return a NULL pointer.

git

2019-11-18 13:43

administrator   ~0089048

Branch CR30697 has been updated forcibly by mzernova.

SHA-1: db9c3d101b998c8f4d8fd2acb27651bddf5ef747

git

2019-11-18 13:44

administrator   ~0089049

Branch CR30697_1 has been updated forcibly by mzernova.

SHA-1: d50855e5b762553bc6b049e5ff7b1b0e1fb54784

git

2019-11-20 11:06

administrator   ~0089091

Branch CR30697 has been updated by mzernova.

SHA-1: a7a15df3c0b7736fdfa60d4304e08c6745e851d7


Detailed log of new commits:

Author: mzernova
Date: Wed Nov 20 10:56:42 2019 +0300

    remarks from osa

git

2019-11-20 11:06

administrator   ~0089092

Branch CR30697_1 has been updated forcibly by mzernova.

SHA-1: 1a75e584fac58893bd88c9678a16041db7050b7a

mzernova

2019-11-21 11:21

developer   ~0089116

The patch CR30697 is ready to review

osa

2019-11-21 12:02

developer   ~0089121

The patch was reviewed

bugmaster

2019-11-22 10:20

administrator   ~0089144

Combination -
OCCT branch : WEEK-47
master SHA - 51ee6a7dbb3a740ba0cfc693ce5e7a83663f05f4
5f5b1aed1c6e139bbd34314eca77ae7abcd8895c
Products branch : WEEK-47 SHA - 1fe84f5f3e15bca9d1c6f8e9646341c4df675aec
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
Debian80-64:
OCCT
Total CPU difference: 16799.850000000122 / 16823.120000000046 [-0.14%]
Products
Total CPU difference: 10810.050000000052 / 10787.080000000053 [+0.21%]
Windows-64-VC14:
OCCT
Total CPU difference: 18294.203125 / 18299.59375 [-0.03%]
Products
Total CPU difference: 12837.546875 / 12853.21875 [-0.12%]


Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention

git

2019-11-24 11:35

administrator   ~0089175

Branch CR30697_1 has been deleted by inv.

SHA-1: 1a75e584fac58893bd88c9678a16041db7050b7a

git

2019-11-24 11:35

administrator   ~0089176

Branch CR30697 has been deleted by inv.

SHA-1: a7a15df3c0b7736fdfa60d4304e08c6745e851d7

Related Changesets

occt: master caee80f3

2019-05-06 20:05:13

mzernova


Committer: bugmaster Details Diff
0030697: Draw Harness - Draw_Printer should not be set to Message::DefaultMessenger() by default

Fixed bugs that occurred when using the default std::cout from Message::DefaultMessenger() instead of Draw_Printer

A dedicated option was added to Draw_ProgressIndicator, for outputting data to the tcl when performing tests

Added -tclOutput parameter to XProgress command
Affected Issues
0030697
mod - src/BOPTest/BOPTest.cxx Diff File
mod - src/DBRep/DBRep.cxx Diff File
mod - src/Draw/Draw_Commands.cxx Diff File
mod - src/Draw/Draw_ProgressIndicator.cxx Diff File
mod - src/Draw/Draw_ProgressIndicator.hxx Diff File
mod - src/QABugs/QABugs_11.cxx Diff File
mod - src/QABugs/QABugs_19.cxx Diff File
mod - src/XSDRAW/XSDRAW.cxx Diff File
mod - tests/bugs/modalg_5/bug22747 Diff File
mod - tests/bugs/moddata_2/bug22572 Diff File
mod - tests/bugs/moddata_2/bug22746_1 Diff File
mod - tests/bugs/moddata_2/bug22746_2 Diff File
mod - tests/bugs/moddata_2/bug22746_3 Diff File

Issue History

Date Modified Username Field Change
2019-05-06 23:09 kgv New Issue
2019-05-06 23:09 kgv Assigned To => kgv
2019-05-06 23:12 git Note Added: 0084205
2019-05-06 23:14 kgv Relationship added related to 0024644
2019-05-06 23:14 kgv Product Version => 6.8.0
2019-05-12 16:46 kgv Relationship added related to 0025748
2019-09-04 15:43 abv Target Version 7.4.0 => 7.5.0
2019-10-05 20:16 kgv Assigned To kgv => user897
2019-10-05 20:16 kgv Status new => assigned
2019-10-05 22:16 kgv Relationship added related to 0031036
2019-11-06 11:30 git Note Added: 0088747
2019-11-06 11:47 kgv Note Added: 0088748
2019-11-06 11:49 kgv Note Added: 0088749
2019-11-06 12:24 osa Assigned To user897 => mzernova
2019-11-07 16:22 git Note Added: 0088777
2019-11-07 16:22 git Note Added: 0088778
2019-11-08 20:12 git Note Added: 0088816
2019-11-08 20:12 git Note Added: 0088817
2019-11-08 20:15 kgv Note Added: 0088818
2019-11-08 20:21 git Note Added: 0088819
2019-11-14 10:41 mzernova Note Added: 0088953
2019-11-14 10:55 kgv Note Added: 0088954
2019-11-14 15:41 git Note Added: 0088962
2019-11-14 15:41 git Note Added: 0088963
2019-11-14 17:39 mzernova Note Added: 0088966
2019-11-14 17:39 mzernova Assigned To mzernova => osa
2019-11-14 17:39 mzernova Status assigned => resolved
2019-11-14 17:39 mzernova Steps to Reproduce Updated
2019-11-14 17:44 mzernova Note Added: 0088969
2019-11-14 17:46 kgv Note Added: 0088970
2019-11-14 17:54 kgv Note Added: 0088971
2019-11-15 11:06 git Note Added: 0088983
2019-11-15 11:48 git Note Added: 0088987
2019-11-15 11:53 git Note Added: 0088988
2019-11-15 12:01 kgv Note Added: 0088989
2019-11-15 12:53 git Note Added: 0088993
2019-11-15 19:09 git Note Added: 0089016
2019-11-18 10:20 git Note Added: 0089039
2019-11-18 11:58 kgv Note Added: 0089045
2019-11-18 13:43 git Note Added: 0089048
2019-11-18 13:44 git Note Added: 0089049
2019-11-19 18:33 osa Assigned To osa => mzernova
2019-11-19 18:33 osa Status resolved => assigned
2019-11-20 11:06 git Note Added: 0089091
2019-11-20 11:06 git Note Added: 0089092
2019-11-21 11:21 mzernova Note Added: 0089116
2019-11-21 11:21 mzernova Assigned To mzernova => osa
2019-11-21 11:21 mzernova Status assigned => resolved
2019-11-21 12:02 osa Note Added: 0089121
2019-11-21 12:02 osa Assigned To osa => bugmaster
2019-11-21 12:02 osa Status resolved => reviewed
2019-11-22 10:20 bugmaster Note Added: 0089144
2019-11-22 10:20 bugmaster Status reviewed => tested
2019-11-24 11:21 bugmaster Changeset attached => occt master caee80f3
2019-11-24 11:21 bugmaster Status tested => verified
2019-11-24 11:21 bugmaster Resolution open => fixed
2019-11-24 11:35 git Note Added: 0089175
2019-11-24 11:35 git Note Added: 0089176
2020-12-02 16:40 emo Fixed in Version => 7.5.0
2020-12-02 17:11 emo Status verified => closed