Don't show tooltip when search field has text
ClosedPublic

Authored by shubham on Tue, Feb 5, 6:13 PM.

Details

Summary

Earlier, the tooltip was continuosly shown even though search field had text.

Test Plan

Hover over search field

Diff Detail

Repository
R134 Discover Software Store
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
shubham created this revision.Tue, Feb 5, 6:13 PM
Restricted Application added a project: Plasma. · View Herald TranscriptTue, Feb 5, 6:13 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
shubham requested review of this revision.Tue, Feb 5, 6:13 PM
apol added a comment.Tue, Feb 5, 6:19 PM

the tooltip should be shown especially when the view is empty.
Also the timeout is set already by the style.

I don't understand what you are trying to fix.

broulik added a subscriber: broulik.Tue, Feb 5, 6:19 PM
broulik added inline comments.
discover/qml/SearchField.qml
47

Prefer using length as that avoids copying a string into JS space

shubham updated this revision to Diff 50977.Tue, Feb 5, 6:21 PM

Use length

shubham marked an inline comment as done.EditedTue, Feb 5, 6:23 PM

@apol I have noticed that the tooltip is seen even when there is text present, also the duration for which it appears is very long
I messed up with summary.

shubham edited the summary of this revision. (Show Details)Tue, Feb 5, 6:24 PM
apol added a comment.Tue, Feb 5, 6:39 PM

What's wrong with seeing the tooltip when there's text?

When user is typing text, the tooltip continuosly shown above distracts and looks ugly

apol accepted this revision.Tue, Feb 5, 6:43 PM

Meh, okay.

This revision is now accepted and ready to land.Tue, Feb 5, 6:43 PM
ngraham requested changes to this revision.Tue, Feb 5, 6:49 PM
ngraham added a subscriber: ngraham.

When user is typing text, the tooltip continuosly shown above distracts and looks ugly

Agreed. There's no need to see the keyboard shortcut to focus the search field after the user has already focused it and is currently using it.

However this is currently very broken: the tooltip always disappears after like a quarter of a second now. That won't fly. Please test for behavior as well as code before publishing a patch. I'm not sure you even need to add the timeout: line at all.

This revision now requires changes to proceed.Tue, Feb 5, 6:49 PM

I'm not sure you even need to add the timeout: line at all.

A tooltip with a negative timeout does not hide automatically. The default value is -1.

apol requested changes to this revision.Wed, Feb 6, 1:50 PM

Nate is right.

@apol about the timeout?

apol added a comment.Wed, Feb 6, 4:30 PM

@apol about the timeout?

yes

shubham updated this revision to Diff 51272.Sat, Feb 9, 4:46 PM

Remove timeout period

It compiles but I could not test this. When I run the plasma-discover segmentation fault prevails.

Works for me, no segfault. I don't see any reason why this change could cause that. @shubham, does the segfault stop happening when you build Discover from source without this change?

shubham added a comment.EditedSun, Feb 10, 3:26 AM

Works for me, no segfault. I don't see any reason why this change could cause that. @shubham, does the segfault stop happening when you build Discover from source without this change?

I should have told that the segafault is unrelated to this change. Even without this change, some kind of segafault appears, because of some dependencies etc maybe.

shubham retitled this revision from Don't show tooltip when search field has text and set a timeout period to Don't show tooltip when search field has text.Sun, Feb 10, 3:28 AM
apol accepted this revision.Sun, Feb 10, 12:26 PM
ngraham accepted this revision.Sun, Feb 10, 2:38 PM

Then let's land this. :)

This revision is now accepted and ready to land.Sun, Feb 10, 2:38 PM

@ngraham Is it now behaving as it should?

Yes, let's land it. Please land it on the 5.15 branch and then merge to master. If you don't know how to do that, I can help you.

shubham added a comment.EditedSun, Feb 10, 2:41 PM

Yes, let's land it. Please land it on the 5.15 branch and then merge to master. If you don't know how to do that, I can help you.

Help will be welcome. And what's the difference between the two?(master and branch)

This revision was automatically updated to reflect the committed changes.