Instantly hide tooltip shown over an element when a key is pressed.
Currently, when pressing an alphanum key to select a different file,
the tooltip continues to cover much of the window - often hiding that newly selected file from view.
ngraham | |
elvisangelaccio |
Dolphin |
Instantly hide tooltip shown over an element when a key is pressed.
Currently, when pressing an alphanum key to select a different file,
the tooltip continues to cover much of the window - often hiding that newly selected file from view.
Lint Skipped |
Unit Tests Skipped |
Thanks for the patch! The fix looks correct, but I have suggestions for the coding style.
src/views/dolphinview.cpp | ||
---|---|---|
1427–1435 | I don't like this duplicate function. We could just add the bool parameter to DolphinView::hideToolTip(). | |
src/views/tooltips/tooltipmanager.h | ||
59 | Please call this parameter hideIstantly. Bonus point if you use an enum instead of a bool ;) (see http://www.informit.com/articles/article.aspx?p=1392524) |
src/views/dolphinview.cpp | ||
---|---|---|
1427–1435 | or rather some flag :) |
src/views/dolphinview.cpp | ||
---|---|---|
1427–1435 | Of course a single function was my initial version. connect(m_container->horizontalScrollBar(), &QScrollBar::valueChanged, this, &DolphinView::hideToolTip); connect(m_container->verticalScrollBar(), &QScrollBar::valueChanged, this, &DolphinView::hideToolTip); error: static assertion failed: Signal and slot arguments are not compatible. Tried solving this with SLOT(), default argument, separate overloaded function with no arguments. It seems that will require QOverload in every connect() call: connect(m_container->horizontalScrollBar(), &QScrollBar::valueChanged, this, QOverload<>::of(&DolphinView::hideToolTip)); connect(m_container->verticalScrollBar(), &QScrollBar::valueChanged, this, QOverload<>::of(&DolphinView::hideToolTip)); Are you OK with this? |
src/views/dolphinview.cpp | ||
---|---|---|
1427–1435 | To be precise: |
Uploaded (final?) diff with version using duplicated hideToolTip()/hideToolTipInstantly() slots.
src/views/dolphinview.cpp | ||
---|---|---|
1427–1435 | We can fix this issue by using a lamba ;) connect(m_container->horizontalScrollBar(), &QScrollBar::valueChanged, this, [=] { hideToolTip(); }); | |
src/views/dolphinview.h | ||
26 | Please drop this include from dolphinview.cpp then. | |
src/views/tooltips/tooltipmanager.cpp | ||
120–123 | Coding style: case should not be more indented than switch | |
src/views/tooltips/tooltipmanager.h | ||
45 | I'd call this enum HideBehavior. It's already implied from the ToolTipManager class that we are talking about tooltips. | |
47 | Comma not needed |
src/views/dolphinview.cpp | ||
---|---|---|
1427–1435 | Have you tried to use this lambda in order to bring back the approach of https://phabricator.kde.org/D22512?id=62021 ? (which I prefer because we avoid to duplicate DolphinView::hideToolTip()) |
Thanks, LGTM now.
You don't have commit access, do you? Is "Piotr Henryk Dabrowski <phd@phd.re>" fine as git authorship?
You don't have commit access, do you?
No, I don't.
Is "Piotr Henryk Dabrowski <phd@phd.re>" fine as git authorship?
Yes.