Tooltip Framework for m_statsTable in HypothesisTestView
ClosedPublic

Authored by devanshuagarwal on Jul 17 2019, 11:45 AM.

Details

Reviewers
sgerlach
asemke
Summary

I have tried adding tooltips framework which can show tooltip separately for each word in QTextEdit.

Test Plan

In result view (stats summary) left-click and hold on the word you want to see the tooltip. Release the button when you are done reading the tip.

Diff Detail

Repository
R262 LabPlot
Lint
Lint Skipped
Unit
Unit Tests Skipped
Restricted Application added a project: KDE Edu. · View Herald TranscriptJul 17 2019, 11:45 AM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
devanshuagarwal requested review of this revision.Jul 17 2019, 11:45 AM
asemke added inline comments.Jul 17 2019, 8:21 PM
src/backend/hypothesisTest/HypothesisTest.cpp
1578

is currCell->data unique or can we have collisions here?

src/backend/hypothesisTest/HypothesisTest.h
85

why do you want to work with a pointer here? You create the map with new in the constructor and never delete it. Just create the map on the stack, which is ok for this use-case, and you don't need to bother with the memory management.

src/backend/hypothesisTest/HypothesisTestPrivate.h
59

you have quite a lot of parameters already in the constructor. Consider setting the tooltip via setToolTip() function. No need to add yet another empty parameter to the constructor.

devanshuagarwal marked an inline comment as done.Jul 18 2019, 10:07 AM
devanshuagarwal added inline comments.
src/backend/hypothesisTest/HypothesisTest.cpp
1578

Yes, we can have collisions here but this is the best way I could come up yet. Another thing that we can do is to store tooltip with word position (local or global). But the challenge will be to find the position of the word in huge strings.

src/backend/hypothesisTest/HypothesisTestPrivate.h
59

Ok, It can be done. But I think it will make code unreadable and confusing:
For example this table horizontal header:

rowMajor.append(new Cell("", level, true));
rowMajor.append(new Cell("SS", level, true));
rowMajor.append(new Cell("df", level, true, "degree of freedom");
rowMajor.append(new Cell("MS", level, true));

will get changed to:

rowMajor.append(new Cell("", level, true));
rowMajor.append(new Cell("SS", level, true));
Cell* newCell = new Cell("DF", level, true); newCell->setToolTip("Degree of Freedom"); rowMajor.append(newCell);
rowMajor.append(new Cell("MS", level, true));
asemke added inline comments.Jul 28 2019, 4:25 PM
src/backend/hypothesisTest/HypothesisTest.cpp
1578

You can create in the constructor of the cell and ID, a random number, and work with cell->id() here to avoid collisions.

src/backend/hypothesisTest/HypothesisTest.h
85

I don't see this done. It's still a pointer here with the object created with new in the constructor and without any delete in the destructor.

src/backend/hypothesisTest/HypothesisTestPrivate.h
90

add i18n for this string.

103

we have already AbstractColumn::isNumeric() returning true for AbstractColumn::Numeric and AbstractColumn::Integer. Please check whether you can use this functions.

devanshuagarwal marked an inline comment as done.Jul 28 2019, 7:18 PM
devanshuagarwal added inline comments.
src/backend/hypothesisTest/HypothesisTest.cpp
1578

No, I don't think that can be done. Because when the user clicks on the word, the information we get is the global position and data of the selected word. While creating the map we didn't know apriori the global position of the word where the tooltip is to be set.

So, to avoid collisions, we have to work on getting the global position of each word apriori.

src/backend/hypothesisTest/HypothesisTest.h
85

Ok, I had done removed pointer from tooltips as I also found it unnecessary. Now, for all pointers I created, I have deleted them.

src/backend/hypothesisTest/HypothesisTestPrivate.h
103

Yes, I am using that only to actually the function definition is:

bool GeneralTest::isNumericOrInteger(const Column* column) {
    return (column->columnMode() == AbstractColumn::Numeric || column->columnMode() == AbstractColumn::Integer);
}

Its is just that this is used many times in code so I wanted to use something short.
Is using macro for this purpose more appropriate?

asemke added inline comments.Jul 28 2019, 7:43 PM
src/backend/hypothesisTest/HypothesisTest.cpp
1578

I didn't refer to the word or to the global position. My suggestion was to create in Cell::Cell() an unique identifier (use a random number maybe) and use it via cell-id() as the key in the map.

src/backend/hypothesisTest/HypothesisTest.h
85

What I meant was use QMap<QString, QString> tooltips instead of QMap<QString, QString>* tooltips.

src/backend/hypothesisTest/HypothesisTestPrivate.h
103

instead of this function you can simply use column->isNumeric().

sgerlach accepted this revision.Aug 29 2019, 3:33 PM

superseeded by D22935

This revision is now accepted and ready to land.Aug 29 2019, 3:33 PM
sgerlach closed this revision.Aug 29 2019, 4:10 PM