ENH: Add translation function for logic classes #7188

mhdiop · 2023-08-23T02:35:40Z

jcfr · 2023-08-23T04:02:31Z

As outlined in the build log¹, the linking is currently failing on Linux:

[1537/5178] Linking CXX shared library bin/libMRMLLogic.so
FAILED: bin/libMRMLLogic.so 
...
Libs/MRML/Logic/CMakeFiles/MRMLLogic.dir/vtkMRMLApplicationLogic.cxx.o: In function `vtkMRMLApplicationLogic::GetTranslatorInstance()':
vtkMRMLApplicationLogic.cxx:(.text+0x1c97): undefined reference to `vtkMRMLApplicationLogic::TranslatorInstance'
Libs/MRML/Logic/CMakeFiles/MRMLLogic.dir/vtkMRMLApplicationLogic.cxx.o: In function `vtkMRMLApplicationLogic::SetTranslatorInstance(vtkMRMLTranslator*)':
vtkMRMLApplicationLogic.cxx:(.text+0x1cd7): undefined reference to `vtkMRMLApplicationLogic::TranslatorInstance'
Libs/MRML/Logic/CMakeFiles/MRMLLogic.dir/vtkMRMLApplicationLogic.cxx.o: In function `vtkMRMLApplicationLogic::Translate(char const*, char const*, char const*, int)':
vtkMRMLApplicationLogic.cxx:(.text+0x1d1b): undefined reference to `vtkMRMLApplicationLogic::TranslatorInstance'
collect2: error: ld returned 1 exit status
[1538/5178] Linking CXX shared library lib/Slicer-5.5/ITKFactories/libMRMLIDIOPlugin.so
[1539/5178] Building CXX object Libs/MRML/Core/CMakeFiles/MRMLCorePython.dir/MRMLCorePythonInit.cxx.o
ninja: build stopped: subcommand failed.

https://github.com/Slicer/Slicer/actions/runs/5946372990/job/16126820100?pr=7188 ↩

Libs/MRML/Logic/vtkMRMLTranslator.h

jcfr · 2023-08-23T04:05:17Z

Libs/MRML/Logic/vtkMRMLTranslator.h

+#ifndef vtkMRMLTranslator_h
+#define vtkMRMLTranslator_h
+
+class vtkMRMLTranslator


Suggested change

class vtkMRMLTranslator

class VTK_MRML_LOGIC_EXPORT vtkMRMLTranslator

Thank you for this comment, the update is done. May I know why we need to add VTK_MRML_LOGIC_EXPORT before the class name ?

Adding VTK_MRML_LOGIC_EXPORT designates the class to be visible on the public API of the shared library. VTK_MRML_LOGIC_EXPORT is a compiler/linker-specific macro defined in an automatically generated .h file.

Thank you, I better understand now !

Base/QTCore/qMRMLTranslator.h

jcfr · 2023-08-23T04:10:33Z

Base/QTCore/qMRMLTranslator.h

+
+#include "vtkMRMLTranslator.h"
+
+class qMRMLTranslator: public vtkMRMLTranslator


Suggested change

class qMRMLTranslator: public vtkMRMLTranslator

class Q_SLICER_BASE_QTCORE_EXPORT qMRMLTranslator: public vtkMRMLTranslator

Base/QTCore/qMRMLTranslator.h

Libs/MRML/Logic/vtkMRMLTranslator.cxx

Libs/MRML/Logic/vtkMRMLTranslator.h

Base/QTCore/qSlicerCoreApplication.cxx

Libs/MRML/Logic/vtkMRMLApplicationLogic.h

* Slicer/SlicerLanguagePacks#12 * Slicer#6647 * Slicer#6177

lassoan · 2023-08-23T12:24:51Z

Base/QTCore/qMRMLTranslator.h

+
+#include "qSlicerBaseQTCoreExport.h"
+
+class Q_SLICER_BASE_QTCORE_EXPORT qMRMLTranslator: public vtkMRMLTranslator


Since this class is derived from a VTK base class, it has to be a VTK class, i.e., vtkMRMLTranslator.

This particular class should probably be called qSlicerQtVTKTranslationBridge ?

Note that since its role is to establish a "bridge" so that VTK logic classes can indirectly leverage Qt translation capabilities through polymorphism, it doesn't have to strictly start with the prefix vtk.

That said, may be the name vtkSlicerQtVTKTranslationBridge would be better ?

vtk prefix is an important hint about how the class is allowed to be constructed and used. vtk... classes are based on vtkObject and rely on internal reference counting. So, for clarity and consistency we either use a proper VTK class (with appropriate naming, internal reference counting, private constructor, etc.) or we don't make the translator a VTK class at all. We could make it a Qt class (derived from QObject, following Qt conventions) then, or just make it a simple C++ class (we establishing all rules).

Since initialization and destruction of singleton classes is a difficult topic, I would use already established singleton patterns in Slicer or VTK. The current design seems to be some custom implementation, which may not work correctly in a large application, composed of many shared libraries.

re: memory

Inheriting from vtkObject make sense and will allow to use vtkNew (that way we are consistent and we don't need to make use of std::shared_ptr)

re: established singleton patterns in Slicer or VTK

I am not using our established pattern made available through vtkSingleton.h is needed here.

Simply instantiating and setting using the following should be sufficient:

vtkNew<vtkMRMLTranslator> translator; this->AppLogic->SetTranslatorInstance(translator);

lassoan · 2023-08-23T12:28:36Z

Base/QTCore/qSlicerCoreApplication.cxx

+  // Create the translation object used for string translation in logic classes
+  if (!this->AppLogic->GetTranslatorInstance())
+  {
+    this->AppLogic->SetTranslatorInstance(new qMRMLTranslator());


qMRMLTranslator is not a Qt class, and it is not taking care of destroying itself if it is created by new. Instead, it is a VTK class, which can be created for example by vtkNew:

vtkNew<vtkMRMLTranslator> translator; this->AppLogic->SetTranslatorInstance(translator);

lassoan · 2023-08-23T12:31:26Z

Libs/MRML/Logic/vtkMRMLApplicationLogic.h

@@ -316,6 +327,8 @@ class VTK_MRML_LOGIC_EXPORT vtkMRMLApplicationLogic
  class vtkInternal;
  vtkInternal* Internal;

+  static vtkMRMLTranslator* TranslatorInstance;


When is this going to be deleted?
How can you ensure that it is deleted at the right time (not too early - when something may still call it; but not too late - after the VTK leak detection mechanism already counted all the non-deleted VTK objects)?

How would we implement this correctly on Windows, Linux, and macOS? We should not reinvent this from scratch but follow a singleton pattern already used in Slicer or VTK.

In this particular case, I don't think we need to use vtkSingleton.h helper class.

Since the lifecycle of this class is driven by vtkMRMLApplicationLogic (itself instantiated and deleted by the main application), the reference to vtkMRMLTranslator will be released upon destruction of vtkMRMLApplicationLogic.

+1 for making a simple vtk class that is owned by the application logic.

lassoan · 2023-09-02T11:33:28Z

Superseded by #7210

This comment was marked as outdated.

Sign in to view

jcfr requested changes Aug 23, 2023

View reviewed changes

ENH: Add translation function for logic classes

711c669

* Slicer/SlicerLanguagePacks#12 * Slicer#6647 * Slicer#6177

mhdiop force-pushed the add-translation-function-for-logic-classes branch from e6f2233 to 711c669 Compare August 23, 2023 11:11

lassoan reviewed Aug 23, 2023

View reviewed changes

lassoan closed this Sep 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Add translation function for logic classes #7188

ENH: Add translation function for logic classes #7188

mhdiop commented Aug 23, 2023 •

edited

This comment was marked as outdated.

jcfr commented Aug 23, 2023

jcfr Aug 23, 2023

mhdiop Aug 23, 2023

lassoan Aug 23, 2023

mhdiop Aug 23, 2023

jcfr Aug 23, 2023

lassoan Aug 23, 2023

jcfr Aug 23, 2023

lassoan Aug 24, 2023

jcfr Aug 24, 2023 •

edited

lassoan Aug 23, 2023

lassoan Aug 23, 2023

jcfr Aug 23, 2023 •

edited

pieper Aug 24, 2023

lassoan commented Sep 2, 2023

	class vtkMRMLTranslator
	class VTK_MRML_LOGIC_EXPORT vtkMRMLTranslator


		#include "vtkMRMLTranslator.h"

		class qMRMLTranslator: public vtkMRMLTranslator

	class qMRMLTranslator: public vtkMRMLTranslator
	class Q_SLICER_BASE_QTCORE_EXPORT qMRMLTranslator: public vtkMRMLTranslator


		#include "qSlicerBaseQTCoreExport.h"

		class Q_SLICER_BASE_QTCORE_EXPORT qMRMLTranslator: public vtkMRMLTranslator

ENH: Add translation function for logic classes #7188

ENH: Add translation function for logic classes #7188

Conversation

mhdiop commented Aug 23, 2023 • edited

This comment was marked as outdated.

jcfr commented Aug 23, 2023

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcfr Aug 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcfr Aug 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lassoan commented Sep 2, 2023

mhdiop commented Aug 23, 2023 •

edited

jcfr Aug 24, 2023 •

edited

jcfr Aug 23, 2023 •

edited