MantisBT - Open CASCADE
View Issue Details
0030697Open CASCADE[OCCT] OCCT:DRAWpublic2019-05-06 23:092019-11-14 17:54
kgv 
osa 
normalminor 
resolvedopen 
[OCCT] 6.8.0 
[OCCT] 7.5.0* 
0030697: Draw Harness - Draw_Printer should not be set to Message::DefaultMessenger() by default
Currently 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.
Not required
No tags attached.
related to 0024644closed bugmaster Draw_Printer - provide the way to control messages gravity filter 
related to 0025748assigned msv Foundation Classes - Parallel version of progress indicator 
related to 0031036verified apn Foundation Classes, Message_PrinterOStream - add option printing colored text into console 
Issue History
2019-05-06 23:09kgvNew Issue
2019-05-06 23:09kgvAssigned To => kgv
2019-05-06 23:12gitNote Added: 0084205
2019-05-06 23:14kgvRelationship addedrelated to 0024644
2019-05-06 23:14kgvProduct Version => 6.8.0
2019-05-12 16:46kgvRelationship addedrelated to 0025748
2019-09-04 15:43abvTarget Version7.4.0 => 7.5.0*
2019-10-05 20:16kgvAssigned Tokgv => user897
2019-10-05 20:16kgvStatusnew => assigned
2019-10-05 22:16kgvRelationship addedrelated to 0031036
2019-11-06 11:30gitNote Added: 0088747
2019-11-06 11:47kgvNote Added: 0088748
2019-11-06 11:49kgvNote Added: 0088749
2019-11-06 12:24osaAssigned Touser897 => mzernova
2019-11-07 16:22gitNote Added: 0088777
2019-11-07 16:22gitNote Added: 0088778
2019-11-08 20:12gitNote Added: 0088816
2019-11-08 20:12gitNote Added: 0088817
2019-11-08 20:15kgvNote Added: 0088818
2019-11-08 20:21gitNote Added: 0088819
2019-11-14 10:41mzernovaNote Added: 0088953
2019-11-14 10:55kgvNote Added: 0088954
2019-11-14 15:41gitNote Added: 0088962
2019-11-14 15:41gitNote Added: 0088963
2019-11-14 17:39mzernovaNote Added: 0088966
2019-11-14 17:39mzernovaAssigned Tomzernova => osa
2019-11-14 17:39mzernovaStatusassigned => resolved
2019-11-14 17:39mzernovaSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=22197#r22197
2019-11-14 17:44mzernovaNote Added: 0088969
2019-11-14 17:46kgvNote Added: 0088970
2019-11-14 17:54kgvNote Added: 0088971

Notes
(0084205)
git   
2019-05-06 23:12   
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
(0088747)
git   
2019-11-06 11:30   
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

(0088748)
kgv   
2019-11-06 11:47   
@@ -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.
(0088749)
kgv   
2019-11-06 11:49   
+  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.
(0088777)
git   
2019-11-07 16:22   
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

(0088778)
git   
2019-11-07 16:22   
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
(0088816)
git   
2019-11-08 20:12   
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

(0088817)
git   
2019-11-08 20:12   
Branch CR30697_1 has been updated forcibly by mzernova.

SHA-1: 2fdf1fe2aac438c62fc4c6d2f29c5c7a023e8942
(0088818)
kgv   
2019-11-08 20:15   
+  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.
(0088819)
git   
2019-11-08 20:21   
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

(0088953)
mzernova   
2019-11-14 10:41   
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
(0088954)
kgv   
2019-11-14 10:55   
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 } {
(0088962)
git   
2019-11-14 15:41   
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

(0088963)
git   
2019-11-14 15:41   
Branch CR30697_1 has been updated forcibly by mzernova.

SHA-1: 05f1e2245b32b4fc69eb4bf8a6673b0b61d44fa0
(0088966)
mzernova   
2019-11-14 17:39   
The patch CR30697 is ready to review
(0088969)
mzernova   
2019-11-14 17:44   
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR30697_1-master-mzernova [^]
(0088970)
kgv   
2019-11-14 17:46   
+    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.
(0088971)
kgv   
2019-11-14 17:54   
@@ -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.