TObj_Application unicode path issue.

Branch [archived branch] has been created by Participant.

[revision removed]


Detailed log of new commits:

Author: mpv
Date: Fri Aug 19 17:56:09 2016 +0300

    0027558: restore support for reading legacy XCAF persistence format. Changed API to support extended strings file paths both internally and externally by TObj_Model and TObj_Application.

I removed "char*" API from Model and Application classes of TObj. So, this fixes the request of Vico in more general way (now it is the specific application part now to check the conversion from its representation of file path to the ExtendedString argument).
Also this fix removes the necessity to convert from ExtendedString to Acscii string and vice versa in many cases. So, it becomes simpler.

Thank you mpv, that's a big improvement.

Reviewed.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Dear Commenter 1,
Branch CR25534 was rebased on IR-2016-08-25 of occt git-repository.
[revision removed]

Dear Commenter 1,
Branch CR25534 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested on Release mode.
[revision removed]

Number of compiler warnings:

occt component :
Linux: 0 (0 on master)
Windows: 0 (0 on master)
MacOS : 0 (0 on master)

products component :
Linux: 64 (64 on master)
Windows: 0 (0 on master)
MacOS : 1155

Regressions/Differences/Improvements:
No regressions/differences

Testing cases:
Not needed

Testing on Linux:
occt component :
Total MEMORY difference: 89966944 / 89981040 [-0.02%]
Total CPU difference: 19210.07999999999 / 19309.729999999934 [-0.52%]
products component :
Total MEMORY difference: 30010832 / 30015953 [-0.02%]
Total CPU difference: 5079.109999999979 / 5043.729999999974 [+0.70%]

Testing on Windows:
occt component :
Total MEMORY difference: 57099450 / 57107084 [-0.01%]
Total CPU difference: 18181.089744698817 / 18053.73052829886 [+0.71%]
products component :
Total MEMORY difference: 21259115 / 21223623 [+0.17%]
Total CPU difference: 4904.562239299948 / 4857.465537399941 [+0.97%]

There are no differences in images found by testdiff.

Dear Commenter 1,
Branch CR25534 is TESTED

Dear mpv,

--- a/src/TObj/TObj_Model.hxx
+++ b/src/TObj/TObj_Model.hxx
   //! Load the OCAF model from a file
-  virtual Standard_EXPORT Standard_Boolean Load (const char* theFile);
+  virtual Standard_EXPORT Standard_Boolean Load (const TCollection_ExtendedString theFile);
 
   //! Save the model to a file
-  virtual Standard_EXPORT Standard_Boolean SaveAs (const char* theFile);
+  virtual Standard_EXPORT Standard_Boolean SaveAs (const TCollection_ExtendedString theFile);

the breaking changes in public API should be documented in dox/dev_guides/upgrade/upgrade.md

Branch [archived branch] has been updated by Participant.

[revision removed]


Detailed log of new commits:

Author: mpv
Date: Thu Sep 1 17:04:36 2016 +0300

    0025534: Document the upgrade of TObj_Model API.

Documentation is updated.

+Methods *TObj_Model::SaveAs* and *TObj_Model::Load* receive *TCollection_ExtendedString* filename arguments instead of char*. This allows to manage non-ASCII symbols explicitly from the application

Description is confusing from my point of view.
All other file APIs taking "const char*" / TCollection_AsciiString in OCCT consider them in UTF-8 (and asking user to pass UTF-16 file name on UNIX platforms sounds like a redundant effort).

Branch [archived branch] has been updated by Participant.

[revision removed]


Detailed log of new commits:

Author: mpv
Date: Thu Sep 1 17:34:21 2016 +0300

    0025534: update the description due to kgv feedback.

In OCAF it is normally TCollection_ExtendedString (for an example TDocStd_Application::SaveAs - similar to this method). So, this change makes API conform.
Also internally it converted to ExtendedString the input char* anyway. So, this change clarifies what it is allowed in the argument, since char* (and even TCollection_AsciiString) has no notion of UTF support.

The description is updated.

   //! Check whether the document contains the OCAF data.
   //! This implementation checks theFile on NULL only.
-  Standard_EXPORT virtual Standard_Boolean checkDocumentEmpty (const char* theFile);
+  Standard_EXPORT virtual Standard_Boolean 
+    checkDocumentEmpty(const TCollection_ExtendedString theFile);
...
    Handle(TObjDRAW_Model) aModel = new TObjDRAW_Model();
    aModel->Load(0);

Please update description of the method and places where modified methods have been used (e.g. TObjDRAW.cxx).

-  Standard_EXPORT virtual Standard_Boolean checkDocumentEmpty (const char* theFile);
+  Standard_EXPORT virtual Standard_Boolean 
+    checkDocumentEmpty(const TCollection_ExtendedString theFile);
...
   //! Load the OCAF model from a file
-  virtual Standard_EXPORT Standard_Boolean Load (const char* theFile);
+  virtual Standard_EXPORT Standard_Boolean Load (const TCollection_ExtendedString theFile);
 
   //! Save the model to a file
-  virtual Standard_EXPORT Standard_Boolean SaveAs (const char* theFile);
+  virtual Standard_EXPORT Standard_Boolean SaveAs (const TCollection_ExtendedString theFile);
...
   //! Saving the OCAF document to a file
   virtual Standard_EXPORT Standard_Boolean SaveDocument
-                         (const Handle(TDocStd_Document)& theSourceDoc,
-                          const char*                     theTargetFile);
+                         (const Handle(TDocStd_Document)&  theSourceDoc,
+                          const TCollection_ExtendedString theTargetFile);
 
   //! Loading the OCAF document from a file
   virtual Standard_EXPORT Standard_Boolean LoadDocument
-                         (const char*                     theSourceFile,
-                          Handle(TDocStd_Document)&       theTargetDoc);
+                         (const TCollection_ExtendedString theSourceFile,
+                          Handle(TDocStd_Document)&        theTargetDoc);

It is preferred to pass TCollection_ExtendedString by reference (TCollection_ExtendedString&).

0027558: restore support for reading legacy XCAF persistence format. Changed API to support extended strings file paths both internally and externally by TObj_Model and TObj_Application.

Commit description does not match bug description - please create a new branch with properly defined description.

Note that file path passed as const char* to methods load/save will be implicitly converted to TCollection_ExtendedString considering multi-byte flag is not set (TCollection_ExtendedString() constructor has no "explicit" keyword).
So existing code will remain buggy without any notice from compiler.

But existing methods TDocStd_Application::Open() and TDocStd_Application::SaveAs() have the same issue.
I would say that it might be better changing TDocStd_Application to take TCollection_AsciiString for consistency with other file APIs in OCCT instead of changing TObj.
But this would be aggressive change.

Maybe it does not worth changing TObj API and just fix file name conversion inside the methods?

Branch [archived branch] has been created by Participant.

[revision removed]


Detailed log of new commits:

Author: mpv
Date: Mon Sep 5 13:27:40 2016 +0300

    0025534: TObj_Application unicode path issue.

Done: branch CR25534_1 is created, some comments are added/modified.

Please test updated patch.

Dear Commenter 1,
Branch CR25534_1 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested on Release mode.
[revision removed]

Number of compiler warnings:

occt component :
Linux: 0 (0 on master)
Windows: 0 (0 on master)
MacOS : 0 (0 on master)

products component :
Linux: 64 (64 on master)
Windows: 0 (0 on master)
MacOS : 1132

Regressions/Differences/Improvements:
No regressions/differences

Testing cases:
Not needed

Testing on Linux:
occt component :
Total MEMORY difference: 89551574 / 89476793 [+0.08%]
Total CPU difference: 19277.29999999999 / 19241.370000000014 [+0.19%]
products component :
Total MEMORY difference: 30005204 / 29985574 [+0.07%]
Total CPU difference: 5078.389999999977 / 5115.309999999973 [-0.72%]

Testing on Windows:
occt component :
Total MEMORY difference: 57150435 / 57155154 [-0.01%]
Total CPU difference: 18272.66233169886 / 18057.115749998855 [+1.19%]
products component :
Total MEMORY difference: 21261562 / 21225231 [+0.17%]
Total CPU difference: 4893.23656669995 / 4849.119483899957 [+0.91%]

There are no differences in images found by testdiff.

Dear Commenter 1,
Branch CR25534_1 is TESTED.

Dear mpv,

I see all interfaces are changed to "const TCollection_ExtendedString", it should be better to use "const TCollection_ExtendedString&" with reference version.

There are two reasons to do so:

firstly, reference should be more efficient since it's without temporary object when passing argument.

secondly, it aligns with interface inside TDocStd_Application, TDocStd_Application use reference version "const TCollection_ExtendedString&".

Dear Vico,

unfortunately the patch has been already integrated, and within our bug advancement workflow we do not reopen bugs.

Therefore, if you want this to be corrected - register dedicated bug (and patches are welcome as well).

Branch [archived branch] has been deleted by Commenter 3.

[revision removed]

Branch [archived branch] has been deleted by Commenter 3.

[revision removed]