Bug 379660 Remove template parameters from flamegraph view#34
Bug 379660 Remove template parameters from flamegraph view#34khReichel wants to merge 1 commit intoKDE:masterfrom
Conversation
Added a textfilter which removes the long template parameters as described in the Bug ticket Bug 379660. Added unit-tests for filtering the entries and removing the template parameters
|
@milianw |
milianw
left a comment
milianw
left a comment
There was a problem hiding this comment.
hey, thanks for your contribution. there are some ways this could be simplified, but otherwise it's already looking good to me.
| return substituteAngleBrackets(s); | ||
| } | ||
|
|
||
| QString MiddleElide::substituteAngleBrackets(const QString& s) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I was not sure if util.h is the right place for the method.
But it is ok for me
|
|
||
| QString MiddleElide::substituteAngleBrackets(const QString& s) | ||
| { | ||
| static QChar startBracket = QChar(u'<'); |
There was a problem hiding this comment.
no need to make this static, and use QLatin1Char please:
const auto startBracket = QLatin1Char('<');
| static QChar stopBracket = QChar(u'>'); | ||
|
|
||
| int level = 0; | ||
| QString result; |
|
|
||
| int level = 0; | ||
| QString result; | ||
| for (int i=0, n = s.length(); i < n; i++) { |
There was a problem hiding this comment.
for (auto currentChar : s)
| QString result; | ||
| for (int i=0, n = s.length(); i < n; i++) { | ||
| QChar currentChar = s[i]; | ||
| if (currentChar == QChar(startBracket) && level == 0) { |
There was a problem hiding this comment.
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;
}
| #include <QString> | ||
| #include "analyze/gui/middleelide.h" | ||
|
|
||
| // add necessary includes here |
| @@ -0,0 +1,64 @@ | |||
| #include <QtTest> | |||
| ~test_initilaize() = default; | ||
|
|
||
| private slots: | ||
| void test_simple_case(); |
There was a problem hiding this comment.
use data driven tests instead (QTest::add{Column,Row} & friends)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| Q_OBJECT | ||
|
|
||
| public: | ||
| test_initilaize() = default; |
| Q_UNREACHABLE(); | ||
| }(); | ||
|
|
||
| QString middleElidedLabel = MiddleElide::elideAngleBracket(label); |
There was a problem hiding this comment.
this functionality needs to be optional, and should be off-by-default. please add a setting and a gui checkbox to enable it
There was a problem hiding this comment.
@milianw Is there some persistancy functionality available in heaptrac.
Otherwise I will use QSettings for storing that elide functionality
There was a problem hiding this comment.
Yes, see MainWindow::m_config
|
ping? any progress on this front? |
2 participants
Added a textfilter which removes the long template parameters as
described in the Bug ticket Bug 379660.
Added unit-tests for filtering the entries and removing the template parameters