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

Bug 379660 Remove template parameters from flamegraph view #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/analyze/gui/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ add_definitions(-Wall
add_library(heaptrack_gui_private STATIC
util.cpp
parser.cpp
middleelide.cpp
)
target_link_libraries(heaptrack_gui_private PUBLIC
KF5::I18n
Expand Down Expand Up @@ -76,6 +77,8 @@ set(LIBRARIES
heaptrack_gui_private
)

add_subdirectory(tests)

if (KChart_FOUND)
list(APPEND SRCFILES
chartwidget.cpp
Expand Down
5 changes: 4 additions & 1 deletion src/analyze/gui/flamegraph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@

#include "resultdata.h"
#include "util.h"
#include "middleelide.h"

enum CostType
{
Expand Down Expand Up @@ -192,10 +193,12 @@ void FrameGraphicsItem::paint(QPainter* painter, const QStyleOptionGraphicsItem*
Q_UNREACHABLE();
}();

QString middleElidedLabel = MiddleElide::elideAngleBracket(label);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this functionality needs to be optional, and should be off-by-default. please add a setting and a gui checkbox to enable it

Copy link
Author

Choose a reason for hiding this comment

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

Ok, got it ;-)

Copy link
Author

Choose a reason for hiding this comment

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

@milianw Is there some persistancy functionality available in heaptrac.
Otherwise I will use QSettings for storing that elide functionality

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, see MainWindow::m_config


const int height = rect().height();
painter->drawText(margin + rect().x(), rect().y(), width, height,
Qt::AlignVCenter | Qt::AlignLeft | Qt::TextSingleLine,
option->fontMetrics.elidedText(label, Qt::ElideRight, width));
option->fontMetrics.elidedText(middleElidedLabel, Qt::ElideRight, width));

if (m_searchMatch == NoMatch) {
painter->setPen(oldPen);
Expand Down
42 changes: 42 additions & 0 deletions src/analyze/gui/middleelide.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#include "middleelide.h"
#include <QChar>

MiddleElide::MiddleElide()
{

}

QString MiddleElide::elideAngleBracket(const QString& s)
{
return substituteAngleBrackets(s);
}

QString MiddleElide::substituteAngleBrackets(const QString& s)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should just be a free function within analyze/gui/util.h - there's no need to put this into a separate file and into a class namespace

Copy link
Author

Choose a reason for hiding this comment

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

I was not sure if util.h is the right place for the method.
But it is ok for me

{
static QChar startBracket = QChar(u'<');
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need to make this static, and use QLatin1Char please:

const auto startBracket = QLatin1Char('<');

static QChar stopBracket = QChar(u'>');

int level = 0;
QString result;
Copy link
Collaborator

Choose a reason for hiding this comment

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

result.reserve(s.size())

for (int i=0, n = s.length(); i < n; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

for (auto currentChar : s)

QChar currentChar = s[i];
if (currentChar == QChar(startBracket) && level == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

simplify:

if (currentChar == startBracket) {
    if (level == 0) {
        result += startBracket;
    }
    ++level;
} else if (currentChar == stopBracket) {
    if (level == 1) {
        result += stopBracket;
    }
    --level;
} else if (level == 0) {
    result += currentChar;
}

result += QChar(startBracket);
level++;
}
else if (currentChar == QChar(startBracket) && level > 0) {
level++;
}
else if (currentChar == QChar(stopBracket) && level == 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

dito

result += QString::fromUtf8("...>");
level--;
}
else if (currentChar == QChar(stopBracket) && level > 1) {
level--;
}
else if (level == 0) {
result += currentChar;
}
}
return result;
}
17 changes: 17 additions & 0 deletions src/analyze/gui/middleelide.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#ifndef MIDDLEELIDE_H
#define MIDDLEELIDE_H

#include <QString>

class MiddleElide
{
public:
MiddleElide();

static QString elideAngleBracket(const QString& s);

private:
static QString substituteAngleBrackets(const QString& s);
};

#endif // MIDDLEELIDE_H
21 changes: 21 additions & 0 deletions src/analyze/gui/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
project(test_middle_elide LANGUAGES CXX)
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this, instead add the test to tests/auto/CMakeLists.txt


find_package(QT NAMES Qt5 Qt6 COMPONENTS Test REQUIRED)
find_package(Qt${QT_VERSION_MAJOR} COMPONENTS Test REQUIRED)

set(CMAKE_INCLUDE_CURRENT_DIR ON)
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

enable_testing()

add_executable(test_middle_elide
tst_middle_elide.cpp
)

add_test(NAME test_middle_elide COMMAND test_middle_elide)

target_link_libraries(test_middle_elide PRIVATE
heaptrack_gui_private
Qt${QT_VERSION_MAJOR}::Test)

64 changes: 64 additions & 0 deletions src/analyze/gui/tests/tst_middle_elide.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
#include <QtTest>
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing license header

#include <QString>
#include "analyze/gui/middleelide.h"

// add necessary includes here
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove


const char* simple_case = "MainWindow::onLoadingFinish(unsigned int&)";
const char* one_bracket = "std::vector<test type in bracket> MainWindow::onLoadingFinish(unsigned int&)";
const char* one_bracket_fixed = "std::vector<...> MainWindow::onLoadingFinish(unsigned int&)";
const char* two_brackets = "std::vector<test type in bracket> MainWindow<vector_a>::onLoadingFinish(unsigned int&)";
const char* two_brackets_fixed = "std::vector<...> MainWindow<...>::onLoadingFinish(unsigned int&)";
const char* nested_brackets = "std::vector<test type <int> in bracket> MainWindow::onLoadingFinish(unsigned int&)";
const char* nested_brackets_fixed = "std::vector<...> MainWindow::onLoadingFinish(unsigned int&)";


class test_initilaize : public QObject
{
Q_OBJECT

public:
test_initilaize() = default;
Copy link
Collaborator

Choose a reason for hiding this comment

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

not needed, remove

~test_initilaize() = default;

private slots:
void test_simple_case();
Copy link
Collaborator

Choose a reason for hiding this comment

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

use data driven tests instead (QTest::add{Column,Row} & friends)

Copy link
Author

Choose a reason for hiding this comment

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

I am familiar with the data-driven tests of the QtTest framework.
However, I wanted to name the individual tests explicitly.
For me, it is much clearer to have individual tests that precisely describe the individual test case.
(e.g. test_single_bracket(), test_multiple_brackets()).

This approach (originally from TDD) describes the tests much more clearly than a data-driven approach with only one method.
What would still be possible in any case is a summary of the redundant code of the individual test methods.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't follow, the rows get a label which would have the same explanation, no? i.e. to me, the following has the same level of detail:

testElideTemplates:single_bracket

testElideTemplates_singleBracket

void test_single_bracket();
void test_multiple_brackets();
void test_nested_brackets();
};

void test_initilaize::test_simple_case()
{
QString testString = QString::fromUtf8(simple_case);
QString result = MiddleElide::elideAngleBracket(testString);
QVERIFY(result == testString);
}

void test_initilaize::test_single_bracket()
{
QString testString = QString::fromUtf8(one_bracket);
QString testStringFixed = QString::fromUtf8(one_bracket_fixed);
QString result = MiddleElide::elideAngleBracket(testString);
QVERIFY(result == testStringFixed);
}

void test_initilaize::test_multiple_brackets()
{
QString testString = QString::fromUtf8(two_brackets);
QString testStringFixed = QString::fromUtf8(two_brackets_fixed);
QString result = MiddleElide::elideAngleBracket(testString);
QVERIFY(result == testStringFixed);
}

void test_initilaize::test_nested_brackets()
{
QString testString = QString::fromUtf8(nested_brackets);
QString testStringFixed = QString::fromUtf8(nested_brackets_fixed);
QString result = MiddleElide::elideAngleBracket(testString);
QVERIFY(result == testStringFixed);
}

QTEST_APPLESS_MAIN(test_initilaize)

#include "tst_middle_elide.moc"