Fix two bugs in KMessageWidget
ClosedPublic

Authored by aacid on Nov 23 2017, 1:27 PM.

Details

Summary

Bug #1: If you call show() or setVisible() after animatedHide() nothing would happen

This is because animatedHide() does
d->content->move(0, -d->content->height());
so it's moving the contents outside the view. To fix that we what the Show event
and if the height or the contents position are not the "correct" ones we set them.
There's an exception for that which is when show comes from animatedShow() so we
use a guard variable for that.

Also added a test for this

Bug #2: If you call animatedShow() while animation for animatedHide() was running things would break

This is because animationShow() was did. If hideAnimmation running, stop it,
then if visible and no animation running, asume we're done. The problem is that isVisible is not
enough for "it's totally visible", now we also check for height and content pos to be the fully
visible one before saying it's done and doing an early return.

There was a test for this already, but the test wasn't good enough since it was only checking for
visible and not for height and content position.

Along with this i also improved the tests making the visible/not visible checks more thorough
and replacing some busy waits with QTRY_VERIFY

Test Plan

Tests pass
Now in okular the message widgets show correctly after calling show() on them after the user pressed the [x]

Diff Detail

Repository
R236 KWidgetsAddons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
aacid created this revision.Nov 23 2017, 1:27 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 23 2017, 1:27 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
aacid updated this revision to Diff 22827.Nov 23 2017, 1:30 PM

Small tweak to the test

It looks good.

src/kmessagewidget.cpp
338

Those it should be

setFixedHeight(sizeHint().height());

In general this patch should be OK.
Could you try with Kate search warp behavior as well? KTextEditor relies on correct behaviors here heavily.

autotests/kmessagewidgetautotest.cpp
32–40

Hm, possibly an inline function instead of macros?

aacid added a comment.Dec 3 2017, 9:26 PM

In general this patch should be OK.
Could you try with Kate search warp behavior as well? KTextEditor relies on correct behaviors here heavily.

What should i be looking at? that it appears and disappers correctly? Anything else specifically?

autotests/kmessagewidgetautotest.cpp
32–40

inline function sucks here because when it fails it gives you the line number of the inline function instead of the line it actually fails in the test.

src/kmessagewidget.cpp
338

I'm 99% sure it's the same value but since i'm comparing against content->height() in the if it makes more sense logically to set to the same value, why do you think sizeHint is better?

Please test the following:

  1. load an existing file in kwrite: kwrite myFile
  2. touch it on console, e.g. echo "asdf" >> myFile
  3. Now a message widghet should appear on top of the view with a notification text.
  4. This is animated. Is the final size correct?
  5. If you then resize kwrite with the message widget visible - is the message widget also resized correctly?

If this looks good, then I am OK with this patch.

PS: And yes, I knew your argument about the macro would come :-)

aacid planned changes to this revision.Mar 4 2018, 5:52 PM
aacid added a comment.Jun 26 2018, 3:04 PM

Please test the following:

  1. load an existing file in kwrite: kwrite myFile
  2. touch it on console, e.g. echo "asdf" >> myFile
  3. Now a message widghet should appear on top of the view with a notification text.
  4. This is animated. Is the final size correct?
  5. If you then resize kwrite with the message widget visible - is the message widget also resized correctly?

    If this looks good, then I am OK with this patch.

    PS: And yes, I knew your argument about the macro would come :-)

I've tried this and i don't get a messagewidget when doing that, just a dialog saying if i want to reload the file or not. Which settings do i need to have enabled for the messagewidget to be used?

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptJun 26 2018, 3:04 PM
dhaumann accepted this revision.Aug 14 2018, 3:00 PM

I just tested again with KWrite - I think it behaves good there.

This revision was not accepted when it landed; it landed in state Changes Planned.Aug 15 2018, 9:19 AM
This revision was automatically updated to reflect the committed changes.