Archived issue #0030697

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

Open CASCADEOCCT:DRAWclosed35 public notes

Description

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.

Steps to reproduce

Not required

Public activity

35 archived notes

Participants are labeled by their role within this record.

01Commenter 32019-05-06 23:12

Branch [archived branch] has been created by Author.

[revision removed]

Detailed log of new commits:

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

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

02Commenter 32019-11-06 11:30

Branch [archived branch] has been updated by Commenter 4.

[revision removed]

Detailed log of new commits:

Author: Commenter 4
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

03Author2019-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.

04Author2019-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.

05Commenter 32019-11-07 16:22

Branch [archived branch] has been updated by Commenter 4.

[revision removed]

Detailed log of new commits:

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

remarks from kgv

06Commenter 32019-11-07 16:22

Branch [archived branch] has been created by Commenter 4.

[revision removed]

Detailed log of new commits:

Author: Author
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

07Commenter 32019-11-08 20:12

Branch [archived branch] has been updated by Commenter 4.

[revision removed]

Detailed log of new commits:

Author: Commenter 4
Date: Fri Nov 8 14:10:14 2019 +0300

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

08Commenter 32019-11-08 20:12

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

09Author2019-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.

10Commenter 32019-11-08 20:21

Branch [archived branch] has been updated by Commenter 4.

[revision removed]

Detailed log of new commits:

Author: Commenter 4
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

11Commenter 42019-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

12Author2019-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 } {

13Commenter 32019-11-14 15:41

Branch [archived branch] has been updated by Commenter 4.

[revision removed]

Detailed log of new commits:

Author: Commenter 4
Date: Thu Nov 14 15:15:10 2019 +0300

Added -tclOutput parameter to XProgress command

14Commenter 32019-11-14 15:41

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

15Commenter 42019-11-14 17:39

The patch CR30697 is ready to review

16Commenter 42019-11-14 17:44

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

17Author2019-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.

18Author2019-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.

19Commenter 32019-11-15 11:06

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

20Commenter 32019-11-15 11:48

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

21Commenter 32019-11-15 11:53

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

22Author2019-11-15 12:01

--- 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.

23Commenter 32019-11-15 12:53

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

24Commenter 32019-11-15 19:09

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

25Commenter 32019-11-18 10:20

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

26Author2019-11-18 11:58

+  //! 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.

27Commenter 32019-11-18 13:43

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

28Commenter 32019-11-18 13:44

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

29Commenter 32019-11-20 11:06

Branch [archived branch] has been updated by Commenter 4.

[revision removed]

Detailed log of new commits:

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

remarks from osa

30Commenter 32019-11-20 11:06

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

31Commenter 42019-11-21 11:21

The patch CR30697 is ready to review

32Commenter 322019-11-21 12:02

The patch was reviewed

33Commenter 12019-11-22 10:20

Combination -
OCCT branch : [archived branch]
master SHA - [revision removed]
[revision removed]
Products branch : [archived branch] SHA - [revision removed]
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

34Commenter 32019-11-24 11:35

Branch [archived branch] has been deleted by Participant.

[revision removed]

35Commenter 32019-11-24 11:35

Branch [archived branch] has been deleted by Participant.

[revision removed]

Related records