Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exceptions: Enable translations of dynamic strings #9709

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Base/CMakeLists.txt
Expand Up @@ -288,6 +288,7 @@ SET(FreeCADBase_HPP_SRCS
Debugger.h
DualNumber.h
DualQuaternion.h
EnhancedPythonExceptions.h
Exception.h
ExceptionFactory.h
Factory.h
Expand Down
96 changes: 96 additions & 0 deletions src/Base/EnhancedPythonExceptions.h
@@ -0,0 +1,96 @@
/***************************************************************************
* Copyright (c) 2023 Abdullah Tahiri <abdullah.tahiri.yo@gmail.com> *
* *
* This file is part of the FreeCAD CAx development system. *
* *
* This program is free software; you can redistribute it and/or modify *
* it under the terms of the GNU Library General Public License (LGPL) *
* as published by the Free Software Foundation; either version 2 of *
* the License, or (at your option) any later version. *
* for detail see the LICENCE text file. *
* *
* FreeCAD is distributed in the hope that it will be useful, *
* but WITHOUT ANY WARRANTY; without even the implied warranty of *
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the *
* GNU Library General Public License for more details. *
* *
* You should have received a copy of the GNU Library General Public *
* License along with FreeCAD; if not, write to the Free Software *
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 *
* USA *
* *
***************************************************************************/


#ifndef BASE_ENHANCED_PYTHON_EXCEPTIONS_H
#define BASE_ENHANCED_PYTHON_EXCEPTIONS_H

#include <fmt/core.h>
#include <CXX/Objects.hxx>

namespace Base
{

/** Triggers an enhanced Python Exception
*
* An enhanced Python Exception stores separately a formatter string and arguments. This
* allows it to produce untranslated and translated dynamic strings (provided that the
* strings provided are marked for translation using the QT_TRANSLATE_NOOP with the "Notifications"
* context).
*
* @param exceptiontype, one of the types of Python exception (e.g. PyExc_ValueError)
* @param format string following the syntax of fmt::format
* @param args one or more args to be substituted in the format string (as in fmt::format)
*
* This method expects that any translatable string in format and args is marked with
* QT_TRANSLATE_NOOP with the context "Notifications". It marks that the string CAN be
* translated. Failure to provide it with marked strings is a bug by client code and will
* result in untranslated strings appearing to the user.
*
* When a Base::Exception is constructed from an enhanced Python Exception, the what() method
* provides the formatted untranslated string. The formatted translated string can be obtained
* using the translateEnhancedMessage method, requiring a translating function. See Base::Exception
* for more details.
*
* Examples:
* PyError_SetEnhancedString(PyExc_ValueError,
* QT_TRANSLATE_NOOP("Notifications", "Invalid constraint index: {}"), Index);
*
*
*/
template<typename... Args>
inline void PyError_SetEnhancedString(PyObject * exceptiontype, const char* format, Args&&... args)
{
std::string swhat = fmt::format(format, args...);

Py::Dict d;

Check warning on line 66 in src/Base/EnhancedPythonExceptions.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

variable name 'd' is too short, expected at least 2 characters [readability-identifier-length]

d.setItem("swhat", Py::String(swhat));

// if it has arguments, then we need to store formater and arguments separately

Check warning on line 70 in src/Base/EnhancedPythonExceptions.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

formater ==> formatter
if constexpr(sizeof...(args) > 0) {
d.setItem("sformatter", Py::String(format));

Py::Tuple t(static_cast<size_t>(sizeof...(args)));

Check warning on line 74 in src/Base/EnhancedPythonExceptions.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

variable name 't' is too short, expected at least 2 characters [readability-identifier-length]

int i = 0;

Check warning on line 76 in src/Base/EnhancedPythonExceptions.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

variable name 'i' is too short, expected at least 2 characters [readability-identifier-length]

(
(
t.setItem(i, Py::String(fmt::format("{}", (std::forward<decltype(args)>(args))))),
i++
),
...
);

d.setItem("sformatterArguments",t);
}

d.setItem("btranslatable", Py::Boolean(true));

PyErr_SetObject(exceptiontype, Py::new_reference_to(d));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wwmayer

I would welcome a trained second set of eyes on this function and in particular on whether I may be leaking (ref counter issues).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks correct to me.


} //namespace Base

#endif // BASE_ENHANCED_PYTHON_EXCEPTIONS_H
41 changes: 40 additions & 1 deletion src/Base/Exception.cpp
Expand Up @@ -24,17 +24,18 @@

#include "PreCompiled.h"

#include <fmt/args.h>

#include "Exception.h"
#include "Console.h"
#include "PyObjectBase.h"


FC_LOG_LEVEL_INIT("Exception", true, true)

Check warning on line 33 in src/Base/Exception.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

non-POD static (LogLevel) [-Wclazy-non-pod-global-static]

Check warning on line 33 in src/Base/Exception.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

variable '_s_fclvl' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

using namespace Base;


TYPESYSTEM_SOURCE(Base::Exception,Base::BaseClass)

Check warning on line 38 in src/Base/Exception.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

variable 'classTypeId' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]


Exception::Exception()
Expand All @@ -53,6 +54,8 @@
, _function(inst._function)
, _isTranslatable(inst._isTranslatable)
, _isReported(inst._isReported)
, _sformatter(inst._sformatter)
, _sformatterArguments(inst._sformatterArguments)
{
}

Expand Down Expand Up @@ -138,6 +141,17 @@
_isTranslatable = static_cast<bool>(Py::Boolean(edict.getItem("btranslatable")));
if (edict.hasKey("breported"))
_isReported = static_cast<bool>(Py::Boolean(edict.getItem("breported")));

if(edict.hasKey("sformatter"))
_sformatter = static_cast<std::string>(Py::String(edict.getItem("sformatter")));

if(edict.hasKey("sformatterArguments")) {
Py::Tuple tuple(edict.getItem("sformatterArguments"));

for(auto i = 0; i < tuple.size(); i++) {
_sformatterArguments.push_back(Py::String(tuple[i]));
}
}
}
}
catch (Py::Exception& e) {
Expand All @@ -160,6 +174,31 @@
PyErr_SetString(exc, what());
}


std::string Exception::translateMessage(std::function<std::string(const std::string &)> translator) const
{
if(_sformatter.empty()) {
return translator(_sErrMsg);
}
else { //
std::string translatedFormatter = translator(_sformatter);

std::vector<std::string> translatedArguments;

std::transform(_sformatterArguments.cbegin(), _sformatterArguments.cend(),
std::back_inserter(translatedArguments),
translator);

auto dynamicstore = fmt::dynamic_format_arg_store<fmt::format_context>();

for (auto translatedArgument : translatedArguments) {

Check warning on line 194 in src/Base/Exception.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

Missing reference in range-for with non trivial type (std::basic_string<char>) [-Wclazy-range-loop-reference]
dynamicstore.push_back(translatedArgument);
}

return fmt::vformat(translatedFormatter, dynamicstore);
}
}

// ---------------------------------------------------------

TYPESYSTEM_SOURCE(Base::AbortException,Base::Exception)
Expand Down
33 changes: 31 additions & 2 deletions src/Base/Exception.h
Expand Up @@ -37,14 +37,16 @@

/// the macros do NOT mark any message for translation
/// If you want to mark text for translation, use the QT_TRANSLATE_NOOP macro
/// with the context "Exceptions" and the right throwing macro from below (the one ending in T)
/// with the context "Notifications" and the right throwing macro from below (the one ending in T)
/// example:
/// THROWMT(Base::ValueError,QT_TRANSLATE_NOOP("Exceptions","The multiplicity cannot be increased beyond the degree of the B-Spline."));
/// THROWMT(Base::ValueError,QT_TRANSLATE_NOOP("Notifications","The multiplicity cannot be increased beyond the degree of the B-Spline."));
///
/// N.B.: The QT_TRANSLATE_NOOP macro won't translate your string. It will just allow lupdate to identify that string for translation so that
/// if you ask for a translation (and the translator have provided one) at that time it gets translated (e.g. in the UI before showing the message
/// of the exception).

//TODO: Move these macros to std::source_location as soon as we move to c++20

#ifdef _MSC_VER

# define THROW(exception) {exception myexcp; myexcp.setDebugInformation(__FILE__,__LINE__,__FUNCSIG__); throw myexcp;}
Expand Down Expand Up @@ -120,6 +122,30 @@

inline void setReported(bool reported) { _isReported = reported; }

/** A function providing a translated exception message.
* @param translator a function or lambda allowing to translate exception strings
*
* This function will attempt to translate formatter and arguments with the provided translator
* function, if formatter exists (if it originates from an enhanced python exception). If not,
* it will try to translate the exception message. The former is intended for dynamically
* generated streams, whereas the latter works fine for static strings.
*
* The only requirement on the translator is that it is able to translate the exception strings
* it is fed with. Any translation framework and context is technically possible.
*
* In practice, the QT translation framework is used in FreeCAD, and translations of exceptions
* generally end up in the console interface, either in original English when intended for
* developers, or translated when intended for the user. Thus, when translated, the exceptions
* are, in essence, notifications. Because the console interface uses the "Notifications" context
* it is best, for coherence, to use QT_TRANSLATE_NOOP with the "Notifications" context for
* exception messages, and then use a translator function that uses the "Notifications" context
* to provide the translations.
*
* Notification's framework (Gui/Notifications.h) provides one such translator. The framework also
* allows to notify directly exceptions by passing the exception object.
*/
std::string translateMessage(std::function<std::string(const std::string &)> translator) const;

/// returns a Python dictionary containing the exception data
PyObject * getPyObject() override;
/// returns sets the exception data from a Python dictionary
Expand All @@ -136,8 +162,8 @@
* - a very technical message not intended to be translated or shown to the user in the UI
* The preferred way of throwing an exception is using the macros above.
* This way, the file, line, and function are automatically inserted. */
Exception(const char * sMessage);

Check warning on line 165 in src/Base/Exception.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

Single-parameter constructors should be marked explicit. [runtime/explicit] [5]
Exception(const std::string& sMessage);

Check warning on line 166 in src/Base/Exception.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

Single-parameter constructors should be marked explicit. [runtime/explicit] [5]
Exception();
Exception(const Exception &inst);

Expand All @@ -148,6 +174,9 @@
std::string _function;
bool _isTranslatable;
mutable bool _isReported;

std::string _sformatter;
std::vector<std::string> _sformatterArguments;
};


Expand All @@ -160,7 +189,7 @@
TYPESYSTEM_HEADER_WITH_OVERRIDE();
public:
/// Construction
AbortException(const char * sMessage);

Check warning on line 192 in src/Base/Exception.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

Single-parameter constructors should be marked explicit. [runtime/explicit] [5]
/// Construction
AbortException();

Expand All @@ -181,8 +210,8 @@
public:
/// Construction
XMLBaseException();
XMLBaseException(const char * sMessage);

Check warning on line 213 in src/Base/Exception.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

Single-parameter constructors should be marked explicit. [runtime/explicit] [5]
XMLBaseException(const std::string& sMessage);

Check warning on line 214 in src/Base/Exception.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

Single-parameter constructors should be marked explicit. [runtime/explicit] [5]

/// Destruction
~XMLBaseException() throw() override = default;
Expand All @@ -197,9 +226,9 @@
{
public:
/// Construction
XMLParseException(const char * sMessage);

Check warning on line 229 in src/Base/Exception.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

Single-parameter constructors should be marked explicit. [runtime/explicit] [5]
/// Construction
XMLParseException(const std::string& sMessage);

Check warning on line 231 in src/Base/Exception.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

Single-parameter constructors should be marked explicit. [runtime/explicit] [5]
/// Construction
XMLParseException();

Expand All @@ -218,9 +247,9 @@
{
public:
/// Construction
XMLAttributeError(const char * sMessage);

Check warning on line 250 in src/Base/Exception.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

Single-parameter constructors should be marked explicit. [runtime/explicit] [5]
/// Construction
XMLAttributeError(const std::string& sMessage);

Check warning on line 252 in src/Base/Exception.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

Single-parameter constructors should be marked explicit. [runtime/explicit] [5]
/// Construction
XMLAttributeError();

Expand All @@ -239,7 +268,7 @@
{
public:
/// With massage and file name
FileException(const char * sMessage, const char * sFileName=nullptr);

Check warning on line 271 in src/Base/Exception.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

Constructors callable with one argument should be marked explicit. [runtime/explicit] [5]
/// With massage and file name
FileException(const char * sMessage, const FileInfo& File);
/// standard construction
Expand Down