[Dolphin] Hide tooltip instantly on key press
ClosedPublic

Authored by pdabrowski on Jul 17 2019, 3:26 PM.

Details

Summary

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.

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
pdabrowski created this revision.Jul 17 2019, 3:26 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptJul 17 2019, 3:26 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
pdabrowski requested review of this revision.Jul 17 2019, 3:26 PM
elvisangelaccio requested changes to this revision.Jul 17 2019, 8:17 PM
elvisangelaccio added a subscriber: elvisangelaccio.

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)

This revision now requires changes to proceed.Jul 17 2019, 8:17 PM
broulik added inline comments.
src/views/dolphinview.cpp
1427–1435

or rather some flag :)

pdabrowski marked an inline comment as done.Jul 18 2019, 8:46 PM
pdabrowski added inline comments.
src/views/dolphinview.cpp
1427–1435

Of course a single function was my initial version.
But then I got errors for connect()s:

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.
But to no avail.

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?

pdabrowski added inline comments.Jul 18 2019, 8:52 PM
src/views/dolphinview.cpp
1427–1435

To be precise:
QOverload plus still a separate overloaded DolphinView::hideToolTip() function with no arguments,
which then calls the DolphinView::hideToolTip(...) with the default flag (in .cpp).

pdabrowski updated this revision to Diff 62021.Jul 19 2019, 1:04 AM

Uploaded diff with version using QOverload.

pdabrowski updated this revision to Diff 62022.Jul 19 2019, 1:06 AM

Uploaded (final?) diff with version using duplicated hideToolTip()/hideToolTipInstantly() slots.

elvisangelaccio requested changes to this revision.Sun, Jul 28, 5:08 PM
elvisangelaccio added inline comments.
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

This revision now requires changes to proceed.Sun, Jul 28, 5:08 PM
pdabrowski updated this revision to Diff 62902.Thu, Aug 1, 2:58 PM
pdabrowski marked 4 inline comments as done.
pdabrowski set the repository for this revision to R318 Dolphin.
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())

pdabrowski updated this revision to Diff 63021.Sat, Aug 3, 11:41 AM
pdabrowski marked 4 inline comments as done.

Done.

elvisangelaccio accepted this revision.Sat, Aug 3, 12:01 PM

Thanks, LGTM now.

You don't have commit access, do you? Is "Piotr Henryk Dabrowski <phd@phd.re>" fine as git authorship?

This revision is now accepted and ready to land.Sat, Aug 3, 12:01 PM

You don't have commit access, do you?

No, I don't.

Is "Piotr Henryk Dabrowski <phd@phd.re>" fine as git authorship?

Yes.

This revision was automatically updated to reflect the committed changes.