[WIP] KMessageWidget: Set widget height on resize event
Needs ReviewPublic

Authored by SGOrava on Jan 18 2020, 8:52 PM.


Group Reviewers

When the resize event was triggered only the content
was resized and the widget itself was kept with same
height. This results in either huge gap between other
widgets or text which is cut of and rendered behind
other widgets.

BUG: 412829

Test Plan

Show a message in KMessageWidget and resize
the widget. I made an application to test this error:

Diff Detail

R236 KWidgetsAddons
more-proper-height (branched from master)
No Linters Available
No Unit Test Coverage
Build Status
Buildable 21346
Build 21364: arc lint + arc unit
SGOrava created this revision.Jan 18 2020, 8:52 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 18 2020, 8:52 PM
SGOrava requested review of this revision.Jan 18 2020, 8:52 PM
SGOrava edited the summary of this revision. (Show Details)Jan 18 2020, 8:59 PM

Since KTextEditor uses KMessageWidget extensively we have to make sure the unit test messagetest still passes with this change.

SGOrava added a comment.EditedJan 19 2020, 9:13 AM

OK, I will try to write a test for it.
This change looks like it lacks some things to make it work properly in some cases.
I am not sure if I am the right person to fiddle with this code (but it is broken...)

The problem which I am faxing is that sizeHint is not changed when I resize the window.

dhaumann accepted this revision.Jan 19 2020, 11:34 AM

I just tested with KTextEditor's messagetest and KTextEditor's usage of KMessageWidget in the top and bottom bar, and all this looks still good.

+1 from my side. Another review would be welcome, though :-) Anyone?

This revision is now accepted and ready to land.Jan 19 2020, 11:34 AM

No, my tests say no.
This does not update sizeHint which can cause some issue later.
It sure looks nice in my test app but it is wrong in the core.
This is not the right solution.

SGOrava retitled this revision from KMessageWidget: Set widget height on resize event to [WIP] KMessageWidget: Set widget height on resize event.Jan 19 2020, 11:41 AM

That's same issue when file is open then externally modified then updated by kmessagewidget button cause a frame to not resize correct?

dhaumann resigned from this revision.Jan 19 2020, 12:28 PM

In that case I withdraw my review for now. I can only speak for Kate that I could not find a regression with my tests. But if you find other regressions, then this is likely not correct.

PS: KMessageWidget is a very unfortunate widget in this sense: We try in the QWidgets world to have nice animations with contents that resizes correctly. To do the animations, we use fixed sizes, however, at the same time the host application's size constraints could change, but if KMessageWidget is still in its animation phase, the wrong old sizes are used for the animation etc... That said, KMessageWidget is likely fundamentally broken, and it needs very careful testing to make sure we don't introduce regressions. That's why I did the tests for Kate/KTextEditor. But of course, also my tests test just a very limited amount of our reald-world scenarios.

This revision now requires review to proceed.Jan 19 2020, 12:28 PM

It looks good.
But I wrote a test as you suggested which still fails.
The test creates parent widget and puts the kmesagewidget and qlistwidget in it in vertical order (as in my test app)
and than it resizes the parent widget and compares the height (the available macros in *autotest.cpp)

I will also upload this test.

SGOrava updated this revision to Diff 73872.Jan 19 2020, 1:00 PM
  • KMessageWidget: Add resize autotest

This is the test I used to check the validity of my change, it kind of always fails.
I only hope someone can fix this widget at least for KF6.

Well, it definitely would be very nice if you find a patch that fixes this.

it would, but I am not such great guru.
I tried few ways to change the behaviour but it was even worse,
and since this is used in many programs I do not want to break them.