D8964: Fix two bugs in KMessageWidget

[Albert Astals Cid]

This revision was not accepted when it landed; it landed in state "Changes 
Planned".
This revision was automatically updated to reflect the committed changes.
Closed by commit R236:e3368be660e3: Fix two bugs in KMessageWidget (authored by 
aacid).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D8964?vs=22827=39774#toc

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8964?vs=22827=39774

REVISION DETAIL
  https://phabricator.kde.org/D8964

AFFECTED FILES
  autotests/kmessagewidgetautotest.cpp
  autotests/kmessagewidgetautotest.h
  src/kmessagewidget.cpp

To: aacid, #frameworks, dhaumann
Cc: kde-frameworks-devel, ngraham, dhaumann, anthonyfieroni, #frameworks, 
michaelh, bruns

D8964: Fix two bugs in KMessageWidget

[Dominik Haumann]

dhaumann accepted this revision.
dhaumann added a comment.


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

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D8964

To: aacid, #frameworks, dhaumann
Cc: kde-frameworks-devel, ngraham, dhaumann, anthonyfieroni, #frameworks, 
michaelh, bruns

D8964: Fix two bugs in KMessageWidget

[Albert Astals Cid]

aacid added a comment.
Restricted Application added a subscriber: kde-frameworks-devel.


  In D8964#175503 , @dhaumann wrote:
  
  > 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?

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D8964

To: aacid, #frameworks
Cc: kde-frameworks-devel, ngraham, dhaumann, anthonyfieroni, #frameworks, 
michaelh, bruns

D8964: Fix two bugs in KMessageWidget

[Albert Astals Cid]

aacid planned changes to this revision.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D8964

To: aacid, #frameworks
Cc: ngraham, dhaumann, anthonyfieroni, #frameworks, michaelh

D8964: Fix two bugs in KMessageWidget

[Dominik Haumann]

dhaumann added a comment.


  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 :-)

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D8964

To: aacid, #frameworks
Cc: ngraham, dhaumann, anthonyfieroni, #frameworks

D8964: Fix two bugs in KMessageWidget

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D8964#173472, @dhaumann wrote:
  
  > 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?

INLINE COMMENTS

> dhaumann wrote in kmessagewidgetautotest.cpp:32-40
> Hm, possibly an inline function instead of macros?

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.

> anthonyfieroni wrote in kmessagewidget.cpp:348
> Those it should be
> 
>   setFixedHeight(sizeHint().height());

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?

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D8964

To: aacid, #frameworks
Cc: ngraham, dhaumann, anthonyfieroni, #frameworks

D8964: Fix two bugs in KMessageWidget

[Dominik Haumann]

dhaumann added a comment.


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

INLINE COMMENTS

> kmessagewidgetautotest.cpp:32-40
> +#define CHECK_FULLY_VISIBLE \
> +QVERIFY(w.isVisible()); \
> +QCOMPARE(w.height(), w.sizeHint().height()); \
> +QCOMPARE(w.findChild(QStringLiteral("contentWidget")) ->pos(), 
> QPoint(0, 0));
> +
> +#define CHECK_FULLY_NOT_VISIBLE \
> +QCOMPARE(w.height(), 0); \

Hm, possibly an inline function instead of macros?

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D8964

To: aacid, #frameworks
Cc: dhaumann, anthonyfieroni, #frameworks

D8964: Fix two bugs in KMessageWidget

[Anthony Fieroni]

anthonyfieroni added a comment.


  It looks good.

INLINE COMMENTS

> kmessagewidget.cpp:348
> +d->content->move(0, 0);
> +setFixedHeight(d->content->height());
> +}

Those it should be

  setFixedHeight(sizeHint().height());

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D8964

To: aacid, #frameworks
Cc: anthonyfieroni, #frameworks

D8964: Fix two bugs in KMessageWidget

[Albert Astals Cid]

aacid added a comment.


  Ping

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D8964

To: aacid, #frameworks
Cc: #frameworks

D8964: Fix two bugs in KMessageWidget

[Albert Astals Cid]

aacid updated this revision to Diff 22827.
aacid added a comment.


  Small tweak to the test

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8964?vs=22825=22827

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D8964

AFFECTED FILES
  autotests/kmessagewidgetautotest.cpp
  autotests/kmessagewidgetautotest.h
  src/kmessagewidget.cpp

To: aacid
Cc: #frameworks

D8964: Fix two bugs in KMessageWidget

[Albert Astals Cid]

aacid created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION 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]

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D8964

AFFECTED FILES
  autotests/kmessagewidgetautotest.cpp
  autotests/kmessagewidgetautotest.h
  src/kmessagewidget.cpp

To: aacid
Cc: #frameworks

D8964: Fix two bugs in KMessageWidget

[Albert Astals Cid]

aacid added a reviewer: Frameworks.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D8964

To: aacid, #frameworks
Cc: #frameworks