ronantakizawa/codereview-bench huggingface dataset:
Repository: FreeCAD/FreeCAD
File: src/Mod/Test/Gui/UnitTestImp.cpp (C++)

Code before:
           "Click 'start', and the test thus produced will be run.\n\n"
           "Double click on an error in the tree view to see more information about it, "
           "including the stack trace.")
    );
}

/**
 * Shows the about dialog.
 */
void UnitTestDialog::onAboutButtonClicked()
{
    QMessageBox::information(
        this,
        tr("About FreeCAD UnitTest"),
        tr("Copyright (c) Werner Mayer\n\n"
           "FreeCAD UnitTest is part of FreeCAD and supports writing Unit Tests for "
           "ones own modules.")
    );
}

void UnitTestDialog::onCopyButtonClicked()
{
    QString text;
    QTreeWidgetItemIterator it(ui->treeViewFailure);
    while (*it) {
        text += (*it)->data(0, Qt::UserRole).toString() + "\n\n";
        ++it;
    }
    if (text.isEmpty()) {
        return;
    }
    QApplication::clipboard()->setText(text);
    setStatusText(tr("Errors copied to clipboard"));
}

/**
 * Runs the unit tests.
 */
void UnitTestDialog::onStartButtonClicked()
{
    reset();
    setProgressColor(QColor(40, 210, 43));  // a darker green
    ui->startButton->setDisabled(true);
    try {
        Base::Interpreter().runString(
            "import qtunittest, gc\n"
            "__qt_test__=qtunittest.QtTestRunner(0,\"\")\n"
            "__qt_test__.runClicked()\n"
            "del __qt_test__\n"
            "gc.collect()\n"
        );

Review comment (suggestion):
```suggestion
        text += (*it)->data(0, Qt::UserRole).toString() + QStringLiteral("\n\n");
```

Code after:
        tr("Help"),
        tr("Enter the name of a callable object which, when called, will return a TestCase.\n"
           "Click 'start', and the test thus produced will be run.\n\n"
           "Double click on an error in the tree view to see more information about it, "
           "including the stack trace.")
    );
}

/**
 * Shows the about dialog.
 */
void UnitTestDialog::onAboutButtonClicked()
{
    QMessageBox::information(
        this,
        tr("About FreeCAD UnitTest"),
        tr("Copyright (c) Werner Mayer\n\n"
           "FreeCAD UnitTest is part of FreeCAD and supports writing Unit Tests for "
           "ones own modules.")
    );
}

void UnitTestDialog::onCopyButtonClicked()
{
    QString text;
    QTreeWidgetItemIterator it(ui->treeViewFailure);
    while (*it) {
        text += (*it)->data(0, Qt::UserRole).toString() + QStringLiteral("\n\n");
        ++it;
    }
    if (text.isEmpty()) {
        return;
    }
    QApplication::clipboard()->setText(text);
    setStatusText(tr("Errors copied to clipboard"));
}

/**
 * Runs the unit tests.
 */
void UnitTestDialog::onStartButtonClicked()
{
    reset();
    setProgressColor(QColor(40, 210, 43));  // a darker green
    ui->startButton->setDisabled(true);
    try {
        Base::Interpreter().runString(
            "import qtunittest, gc\n"
            "__qt_test__=qtunittest.QtTestRunner(0,\"\")\n"
            "__qt_test__.runClicked()\n"
            "del __qt_test__\n"
Repository: FreeCAD/FreeCAD
File: src/App/DocumentObject.cpp (C++)

Code before:
    return hiddenMarker() == marker ? marker : nullptr;
}

bool DocumentObject::redirectSubName(std::ostringstream&, DocumentObject*, DocumentObject*) const
{
    return false;
}

void DocumentObject::onPropertyStatusChanged(const Property& prop, unsigned long oldStatus)
{
    (void)oldStatus;
    if (!Document::isAnyRestoring() && isAttachedToDocument() && getDocument()) {
        getDocument()->signalChangePropertyEditor(*getDocument(), prop);
    }
}

Base::Placement DocumentObject::getPlacementOf(const std::string& sub, DocumentObject* targetObj)
{
    Base::Placement plc;
    auto* propPlacement = dynamic_cast<App::PropertyPlacement*>(getPropertyByName("Placement"));
    if (!propPlacement) {
        return plc;
    }
    plc = propPlacement->getValue();

    std::vector<std::string> names = Base::Tools::splitSubName(sub);

    if (names.empty() || this == targetObj) {
        return plc;
    }

    DocumentObject* subObj = getDocument()->getObject(names.front().c_str());

    if (!subObj) {
        return plc;
    }

    std::vector<std::string> newNames(names.begin() + 1, names.end());
    std::string newSub = Base::Tools::joinList(newNames, ".");

    return plc * subObj->getPlacementOf(newSub, targetObj);
}


Review comment (suggestion):
We really need something like `Base::ObjectPath` that would basically capture the notion of that string. But this is of course for another time...

Code after:
    return hiddenMarker() == marker ? marker : nullptr;
}

bool DocumentObject::redirectSubName(std::ostringstream&, DocumentObject*, DocumentObject*) const
{
    return false;
}

void DocumentObject::onPropertyStatusChanged(const Property& prop, unsigned long oldStatus)
{
    (void)oldStatus;
    if (!Document::isAnyRestoring() && isAttachedToDocument() && getDocument()) {
        getDocument()->signalChangePropertyEditor(*getDocument(), prop);
    }
}

Base::Placement DocumentObject::getPlacementOf(const std::string& sub, DocumentObject* targetObj)
{
    Base::Placement plc;
    auto* propPlacement = freecad_cast<App::PropertyPlacement*>(getPropertyByName("Placement"));
    if (propPlacement) {
        // If the object has no placement (like a Group), plc stays identity so we can proceed.
        plc = propPlacement->getValue();
    }

    std::vector<std::string> names = Base::Tools::splitSubName(sub);

    if (names.empty() || this == targetObj) {
        return plc;
    }

    DocumentObject* subObj = getDocument()->getObject(names.front().c_str());

    if (!subObj) {
        return plc;
    }

    std::vector<std::string> newNames(names.begin() + 1, names.end());
    std::string newSub = Base::Tools::joinList(newNames, ".");

    return plc * subObj->getPlacementOf(newSub, targetObj);
}
Repository: FreeCAD/FreeCAD
File: src/App/DocumentObject.cpp (C++)

Code before:
    if (!marker) {
        marker = subname;
    }
    else {
        ++marker;
    }
    return hiddenMarker() == marker ? marker : nullptr;
}

bool DocumentObject::redirectSubName(std::ostringstream&, DocumentObject*, DocumentObject*) const
{
    return false;
}

void DocumentObject::onPropertyStatusChanged(const Property& prop, unsigned long oldStatus)
{
    (void)oldStatus;
    if (!Document::isAnyRestoring() && isAttachedToDocument() && getDocument()) {
        getDocument()->signalChangePropertyEditor(*getDocument(), prop);
    }
}

Base::Placement DocumentObject::getPlacementOf(const std::string& sub, DocumentObject* targetObj)
{
    Base::Placement plc;
    auto* propPlacement = dynamic_cast<App::PropertyPlacement*>(getPropertyByName("Placement"));
    if (!propPlacement) {
        return plc;
    }
    plc = propPlacement->getValue();

    std::vector<std::string> names = Base::Tools::splitSubName(sub);

    if (names.empty() || this == targetObj) {
        return plc;
    }

    DocumentObject* subObj = getDocument()->getObject(names.front().c_str());

    if (!subObj) {
        return plc;
    }

    std::vector<std::string> newNames(names.begin() + 1, names.end());
    std::string newSub = Base::Tools::joinList(newNames, ".");

    return plc * subObj->getPlacementOf(newSub, targetObj);
}


Review comment (suggestion):
```suggestion
    auto* propPlacement = freecad_cast<App::PropertyPlacement*>(getPropertyByName("Placement"));
```

Code after:
    if (!marker) {
        marker = subname;
    }
    else {
        ++marker;
    }
    return hiddenMarker() == marker ? marker : nullptr;
}

bool DocumentObject::redirectSubName(std::ostringstream&, DocumentObject*, DocumentObject*) const
{
    return false;
}

void DocumentObject::onPropertyStatusChanged(const Property& prop, unsigned long oldStatus)
{
    (void)oldStatus;
    if (!Document::isAnyRestoring() && isAttachedToDocument() && getDocument()) {
        getDocument()->signalChangePropertyEditor(*getDocument(), prop);
    }
}

Base::Placement DocumentObject::getPlacementOf(const std::string& sub, DocumentObject* targetObj)
{
    Base::Placement plc;
    auto* propPlacement = freecad_cast<App::PropertyPlacement*>(getPropertyByName("Placement"));
    if (propPlacement) {
        // If the object has no placement (like a Group), plc stays identity so we can proceed.
        plc = propPlacement->getValue();
    }

    std::vector<std::string> names = Base::Tools::splitSubName(sub);

    if (names.empty() || this == targetObj) {
        return plc;
    }

    DocumentObject* subObj = getDocument()->getObject(names.front().c_str());

    if (!subObj) {
        return plc;
    }

    std::vector<std::string> newNames(names.begin() + 1, names.end());
    std::string newSub = Base::Tools::joinList(newNames, ".");

    return plc * subObj->getPlacementOf(newSub, targetObj);
}
Repository: FreeCAD/FreeCAD
File: src/Mod/PartDesign/App/FeatureChamfer.h (C/C++)

Code before:
    //@{
    /// recalculate the feature
    App::DocumentObjectExecReturn* execute() override;
    short mustExecute() const override;
    /// returns the type name of the view provider
    const char* getViewProviderName() const override
    {
        return "PartDesignGui::ViewProviderChamfer";
    }
    //@}

    void onChanged(const App::Property* /*prop*/) override;

    void updateProperties();

protected:
    void Restore(Base::XMLReader& reader) override;
    void handleChangedPropertyType(
        Base::XMLReader& reader,
        const char* TypeName,
        App::Property* prop
    ) override;
    static const App::PropertyQuantityConstraint::Constraints floatSize;
    static const App::PropertyAngle::Constraints floatAngle;

    bool requiresSizeSwapping(const Base::XMLReader& reader);
    void migrateFlippedProperties(const Base::XMLReader& reader);
};

}  // namespace PartDesign


#endif  // PARTDESIGN_FEATURECHAMFER_H


Review comment (suggestion):
```suggestion
    bool requiresSizeSwapping(const Base::XMLReader& reader) const;
```

Code after:
    //@{
    /// recalculate the feature
    App::DocumentObjectExecReturn* execute() override;
    short mustExecute() const override;
    /// returns the type name of the view provider
    const char* getViewProviderName() const override
    {
        return "PartDesignGui::ViewProviderChamfer";
    }
    //@}

    void onChanged(const App::Property* /*prop*/) override;

    void updateProperties();

protected:
    void Restore(Base::XMLReader& reader) override;
    void handleChangedPropertyType(
        Base::XMLReader& reader,
        const char* TypeName,
        App::Property* prop
    ) override;
    static const App::PropertyQuantityConstraint::Constraints floatSize;
    static const App::PropertyAngle::Constraints floatAngle;

    bool requiresSizeSwapping(const Base::XMLReader& reader) const;
    void migrateFlippedProperties(const Base::XMLReader& reader);
};

}  // namespace PartDesign


#endif  // PARTDESIGN_FEATURECHAMFER_H
Repository: FreeCAD/FreeCAD
File: src/Gui/FileDialog.cpp (C++)

Code before:
#include <QMenuBar>
#include <QPushButton>
#include <QRadioButton>
#include <QRegularExpression>
#include <QRegularExpressionMatch>
#include <QResizeEvent>
#include <QStandardPaths>
#include <QStyle>
#include <QUrl>


#include <Base/Parameter.h>
#include <App/Application.h>

#include "FileDialog.h"
#include "MainWindow.h"
#include "Tools.h"


using namespace Gui;

// An raii-helper struct to disable actions while dialogs are open
// At least on macos, shortcuts for enabled actions will still trigger while dialogs are open
struct RaiiActionDisabler
{
    RaiiActionDisabler()
    {
        auto mainWin = Gui::getMainWindow();
        if (!mainWin) {
            return;
        }
        QMenuBar* menuBar = mainWin->menuBar();
        if (!menuBar) {
            return;
        }
        auto actions = menuBar->actions();
        actionsToReenable.reserve(actions.size());
        for (auto action : actions) {
            if (action->isEnabled()) {
                action->setEnabled(false);
                actionsToReenable.push_back(action);
            }
        }
    }

    ~RaiiActionDisabler()
    {
        for (auto action : actionsToReenable) {
            if (action) {
                action->setEnabled(true);
            }

Review comment (suggestion):
`ActionDisabler` would be good enough as name - no need to put `Raii` here - we expect people to know the pattern :)

Code after:
#include <QMenuBar>
#include <QPushButton>
#include <QRadioButton>
#include <QRegularExpression>
#include <QRegularExpressionMatch>
#include <QResizeEvent>
#include <QStandardPaths>
#include <QStyle>
#include <QUrl>


#include <Base/Parameter.h>
#include <App/Application.h>

#include "FileDialog.h"
#include "MainWindow.h"
#include "Tools.h"


using namespace Gui;

// An raii-helper struct to disable actions while dialogs are open
// At least on macos, shortcuts for enabled actions will still trigger while dialogs are open
struct ActionDisabler
{
    ActionDisabler()
    {
        auto mainWin = Gui::getMainWindow();
        if (!mainWin) {
            return;
        }
        QMenuBar* menuBar = mainWin->menuBar();
        if (!menuBar) {
            return;
        }
        auto actions = menuBar->actions();
        actionsToReenable.reserve(actions.size());
        for (auto action : actions) {
            if (action->isEnabled()) {
                action->setEnabled(false);
                actionsToReenable.push_back(action);
            }
        }
    }

    ~ActionDisabler()
    {
        for (auto action : actionsToReenable) {
            if (action) {
                action->setEnabled(true);
            }
Repository: FreeCAD/FreeCAD
File: src/Gui/FileDialog.cpp (C++)

Code before:


using namespace Gui;

// An raii-helper struct to disable actions while dialogs are open
// At least on macos, shortcuts for enabled actions will still trigger while dialogs are open
struct RaiiActionDisabler
{
    RaiiActionDisabler()
    {
        auto mainWin = Gui::getMainWindow();
        if (!mainWin) {
            return;
        }
        QMenuBar* menuBar = mainWin->menuBar();
        if (!menuBar) {
            return;
        }
        auto actions = menuBar->actions();
        actionsToReenable.reserve(actions.size());
        for (auto action : actions) {
            if (action->isEnabled()) {
                action->setEnabled(false);
                actionsToReenable.push_back(action);
            }
        }
    }

    ~RaiiActionDisabler()
    {
        for (auto action : actionsToReenable) {
            if (action) {
                action->setEnabled(true);
            }
        }
    }

    RaiiActionDisabler(const RaiiActionDisabler&) = delete;
    RaiiActionDisabler& operator=(const RaiiActionDisabler&) = delete;

    std::vector<QAction*> actionsToReenable {};
};


bool DialogOptions::dontUseNativeFileDialog()
{
#if defined(USE_QT_DIALOGS)
    constexpr bool notNativeDialog = true;
#else
    constexpr bool notNativeDialog = false;
#endif

Review comment (refactor):
non-blocking:

I do wonder if we can make this into more generic utility? Right now it is hardcoded to disable all actions but it could have additional constructor accepting QList<QAction*> (or std::vector, or std::list, anything) with actions that should be disabled and re-enabled later?

The current constructor can stay to basically disable all the actions.

Code after:


using namespace Gui;

// An raii-helper struct to disable actions while dialogs are open
// At least on macos, shortcuts for enabled actions will still trigger while dialogs are open
struct ActionDisabler
{
    ActionDisabler()
    {
        auto mainWin = Gui::getMainWindow();
        if (!mainWin) {
            return;
        }
        QMenuBar* menuBar = mainWin->menuBar();
        if (!menuBar) {
            return;
        }
        auto actions = menuBar->actions();
        actionsToReenable.reserve(actions.size());
        for (auto action : actions) {
            if (action->isEnabled()) {
                action->setEnabled(false);
                actionsToReenable.push_back(action);
            }
        }
    }

    ~ActionDisabler()
    {
        for (auto action : actionsToReenable) {
            if (action) {
                action->setEnabled(true);
            }
        }
    }

    FC_DISABLE_COPY(ActionDisabler)

    std::vector<QAction*> actionsToReenable {};
};


bool DialogOptions::dontUseNativeFileDialog()
{
#if defined(USE_QT_DIALOGS)
    constexpr bool notNativeDialog = true;
#else
    constexpr bool notNativeDialog = false;
#endif
Repository: FreeCAD/FreeCAD
File: src/Gui/FileDialog.cpp (C++)

Code before:
        }
        QMenuBar* menuBar = mainWin->menuBar();
        if (!menuBar) {
            return;
        }
        auto actions = menuBar->actions();
        actionsToReenable.reserve(actions.size());
        for (auto action : actions) {
            if (action->isEnabled()) {
                action->setEnabled(false);
                actionsToReenable.push_back(action);
            }
        }
    }

    ~RaiiActionDisabler()
    {
        for (auto action : actionsToReenable) {
            if (action) {
                action->setEnabled(true);
            }
        }
    }

    RaiiActionDisabler(const RaiiActionDisabler&) = delete;
    RaiiActionDisabler& operator=(const RaiiActionDisabler&) = delete;

    std::vector<QAction*> actionsToReenable {};
};


bool DialogOptions::dontUseNativeFileDialog()
{
#if defined(USE_QT_DIALOGS)
    constexpr bool notNativeDialog = true;
#else
    constexpr bool notNativeDialog = false;
#endif

    ParameterGrp::handle group = App::GetApplication()
                                     .GetUserParameter()
                                     .GetGroup("BaseApp")
                                     ->GetGroup("Preferences")
                                     ->GetGroup("Dialog");
    return group->GetBool("DontUseNativeDialog", notNativeDialog);
}

bool DialogOptions::dontUseNativeColorDialog()
{
#if defined(USE_QT_DIALOGS)
    constexpr bool notNativeDialog = true;

Review comment (suggestion):
You can use `FC_DISABLE_COPY_MOVE(RaiiActionDisabler)` here. Yes, it's a macro but it really well explains the intent.

Code after:
        }
        QMenuBar* menuBar = mainWin->menuBar();
        if (!menuBar) {
            return;
        }
        auto actions = menuBar->actions();
        actionsToReenable.reserve(actions.size());
        for (auto action : actions) {
            if (action->isEnabled()) {
                action->setEnabled(false);
                actionsToReenable.push_back(action);
            }
        }
    }

    ~ActionDisabler()
    {
        for (auto action : actionsToReenable) {
            if (action) {
                action->setEnabled(true);
            }
        }
    }

    FC_DISABLE_COPY(ActionDisabler)

    std::vector<QAction*> actionsToReenable {};
};


bool DialogOptions::dontUseNativeFileDialog()
{
#if defined(USE_QT_DIALOGS)
    constexpr bool notNativeDialog = true;
#else
    constexpr bool notNativeDialog = false;
#endif

    ParameterGrp::handle group = App::GetApplication()
                                     .GetUserParameter()
                                     .GetGroup("BaseApp")
                                     ->GetGroup("Preferences")
                                     ->GetGroup("Dialog");
    return group->GetBool("DontUseNativeDialog", notNativeDialog);
}

bool DialogOptions::dontUseNativeColorDialog()
{
#if defined(USE_QT_DIALOGS)
    constexpr bool notNativeDialog = true;
#else
Repository: FreeCAD/FreeCAD
File: src/Gui/SoFCDB.cpp (C++)

Code before:
                }
                else {
                    static_cast<SoVRMLIndexedFaceSet*>(geom)->creaseAngle.setValue(0.5f);
                }
            }
        }
        sa.reset();  // clear the internal cache
    }

    std::stringstream out;
    writeX3D(vrmlRoot, exportViewpoints, out);
    buffer = out.str();

    vrmlRoot->unref();  // release the memory as soon as possible

    // restore old settings
    vrml2.setOverrideMode(false);
    vrml2.apply(noSwitches);
    noSwitches->unref();

    return true;
}

namespace
{
std::string escapeXmlAttribute(const char* str)
{
    if (!str) {
        return std::string();
    }

    std::string result;
    result.reserve(strlen(str));

    for (const char* p = str; *p; ++p) {
        switch (*p) {
            case '"':
                result.append("&quot;");
                break;
            case '&':
                result.append("&amp;");
                break;
            case '<':
                result.append("&lt;");
                break;
            case '>':
                result.append("&gt;");
                break;
            case '\'':
                result.append("&apos;");
                break;

Review comment (suggestion):
1. Don't we have anything like that already provided? Escaping attributes should not be done manually but with some kind library - but not sure if it is available in libxerces.
2. can std::string_view be used here instead of `const char*`? It would make your life easier for the for loop

Code after:
                    }
                }
                else {
                    static_cast<SoVRMLIndexedFaceSet*>(geom)->creaseAngle.setValue(0.5f);
                }
            }
        }
        sa.reset();  // clear the internal cache
    }

    std::stringstream out;
    writeX3D(vrmlRoot, exportViewpoints, out);
    buffer = out.str();

    vrmlRoot->unref();  // release the memory as soon as possible

    // restore old settings
    vrml2.setOverrideMode(false);
    vrml2.apply(noSwitches);
    noSwitches->unref();

    return true;
}


void Gui::SoFCDB::writeX3DFields(
    SoNode* node,
    std::map<SoNode*, std::string>& nodeMap,
    bool isRoot,
    int& numDEF,
    int spaces,
    std::ostream& out
)
{
    // remove the VRML prefix from the type name
    std::string type(node->getTypeId().getName().getString());
    type = type.substr(4);

    out << Base::blanks(spaces) << "<" << type;
    if (node->getRefCount() > 1 && !isRoot) {
        SbName name = node->getName();
        std::stringstream str;
        if (name.getLength() == 0) {
            str << "o" << numDEF++;
        }
        else {
            str << name.getString();
        }

        nodeMap[node] = str.str();
        std::string escapedName = Base::Persistence::encodeAttribute(str.str());