Bug 436958 - Diff view is transparent and flickers terribly on Wayland
Summary: Diff view is transparent and flickers terribly on Wayland
Status: RESOLVED FIXED
Alias: None
Product: kdiff3
Classification: Applications
Component: application (show other bugs)
Version: 1.9.0
Platform: Neon Linux
: NOR critical
Target Milestone: ---
Assignee: michael
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2021-05-12 07:51 UTC by geisserml
Modified: 2021-05-17 00:45 UTC (History)
8 users (show)

See Also:
Latest Commit:
Version Fixed In: 1.9.2


Attachments
KDiff3 with transparent diff view and visual artifacts. (161.45 KB, image/png)
2021-05-12 07:51 UTC, geisserml
Details
kdiff3 gdb (15.45 KB, text/plain)
2021-05-13 10:38 UTC, geisserml
Details
Proposed fix (946 bytes, patch)
2021-05-16 18:28 UTC, michael
Details

Note You need to log in before you can comment on or make changes to this bug.
Description geisserml 2021-05-12 07:51:38 UTC
Created attachment 138352 [details]
KDiff3 with transparent diff view and visual artifacts.

SUMMARY
Diff view is transparent and flickers terribly on Wayland since the latest stable release 1.9.x. Never happend with 1.8.x.

STEPS TO REPRODUCE
1. Log in to a Plasma Wayland session
2. Launch KDiff3 (for example as git difftool)

OBSERVED RESULT
Diff view is transparent, the window / wallpaper below is visible. As soon as any user action occurs, there will be the most terrible flickering and visual artifacts.

EXPECTED RESULT
Nothing should be transparent. Nothing should flicker. Diff view should show up properly as it used to in the previous releases.

SOFTWARE/OS VERSIONS
Operating System: KDE neon 5.21
KDE Plasma Version: 5.21.5
KDE Frameworks Version: 5.81.0
Qt Version: 5.15.2
Kernel Version: 5.4.0-73-generic
OS Type: 64-bit
Graphics Platform: Wayland
Processors: 4 × Intel® Core™ i5-3570K CPU @ 3.40GHz
Memory: 7.2 GiB of RAM
Graphics Processor: Mesa DRI Intel® HD Graphics 4000

ADDITIONAL INFORMATION
See https://bugs.kde.org/show_bug.cgi?id=433484 and subsequent comments.
This (sadly) is a common problem, usually caused by native widget siblings or deprecated QGLWidgets.
Comment 1 Jens Saak 2021-05-12 08:17:26 UTC
This appears to be a more general (as in "not limited to Wayland") problem with 1.9.x. On my Xorg based system the file-diff-canvas simply shows up black. The folder-diff renders correctly, though.

My system:

Operating System: KDE neon 5.21
KDE Plasma Version: 5.21.5
KDE Frameworks Version: 5.81.0
Qt Version: 5.15.2
Kernel Version: 5.8.0-50-generic
OS Type: 64-bit
Graphics Platform: X11
Processors: 12 × AMD Ryzen 5 2600 Six-Core Processor
Memory: 15.6 GiB of RAM
Graphics Processor: NV167

my laptop based on i7-8550U and UHD Graphics 620 ,and the same software versions, gives the exact same result.
Comment 2 geisserml 2021-05-12 09:01:27 UTC
Also, the buttons in the file selection dialog don't work anymore since version 1.9.x - should I open a separate report?
@ Jens Saak: Are you experiencing this problem as well?
Comment 3 Jens Saak 2021-05-12 09:04:52 UTC
Yes, the only things that react on mouse clicks are the drop-down file selectors. All buttons are dead.
Comment 4 geisserml 2021-05-12 09:08:56 UTC
I'm wondering why this all hasn't been found during pre-release tests. Maybe a wrong version got packaged in Neon?
Comment 5 geisserml 2021-05-12 10:52:46 UTC
For users who are looking for a temporary workaround, you can downgrade kdiff3 back to version 1.8.5 via muon package manager and then pin it using `sudo apt-mark hold`.
Comment 6 michael 2021-05-12 23:08:23 UTC
KDiff3 does not directly invoke OQGLWidgets or native widgets of any kind. If this happening with current 1.9 branch the bug is likely in Qt or or its supporting framework. I've sent a call out to have some else in kde look at this. Defining QT_QPA_PLATFORM=xcb has help wayland users in the past. None of this affects my test machine not even with a clean rebuild from source.
Comment 7 Antonio Rojas 2021-05-13 05:56:11 UTC
This is reproducible on Arch with our default build flags (-O2 and -DCMAKE_BUILD_TYPE=None). Adding NDEBUG to CFLAGS fixes the issue.
Comment 8 michael 2021-05-13 06:04:45 UTC
That may be the missing piece of the puzzle. If nothing else I can force that define by default. I'll have a closer look tomorrow.
Comment 9 David Edmundson 2021-05-13 08:05:24 UTC
If someone can reliably reproduce, it would help to rule out one common cause.

please run gdb kdiff3

break QPlatformWindow::QPlatformWindow
run

and type "bt" on all occurrences and attach here.
Comment 10 geisserml 2021-05-13 10:38:53 UTC
Created attachment 138383 [details]
kdiff3 gdb
Comment 11 geisserml 2021-05-13 10:45:12 UTC
(In reply to michael from comment #6)
> Defining QT_QPA_PLATFORM=xcb has help wayland users in the past.
As Jens Saak outlined, this problem affects X11 as well. There is no flickering or transparency on X, but the diff view is not visible either, it's just black.
I initially did not notice this since I only tried Wayland and thought it's very similar to the other Wayland-only issues I already had. This one apparently is an exception, however.
Comment 12 geisserml 2021-05-13 10:46:57 UTC
(In reply to David Edmundson from comment #9)
> If someone can reliably reproduce, it would help to rule out one common
> cause.
With the build from neon user, this is always reproducable for me.
Comment 13 geisserml 2021-05-13 10:55:12 UTC
(In reply to Manuel Geißer from comment #10)
> Created attachment 138383 [details]
> kdiff3 gdb
Might lack some symbols. If anything important is missing, please tell which package and I can redo the backtrace.
Comment 14 Kyle Johnson 2021-05-13 17:04:10 UTC
So this bug has also been reported in Gentoo here: https://bugs.gentoo.org/789330

Apparently the issue comes in with merge request 5 (https://invent.kde.org/sdk/kdiff3/-/merge_requests/5). If you look at the added chk_connect_x macros defined in src/defmac.h, they all rely on the QCONNECT_ASSERT macro which is defined as the following:

#ifndef NDEBUG
#define QCONNECT_ASSERT(COND_) Q_ASSERT(COND_)
#else
#define QCONNECT_ASSERT(COND_) COND_
#endif

I'm not particularly versed in KDE/QT development, but does Q_ASSERT(COND_) resolve to nothing in release code? If so, then it would seem plausible that not defining NDEBUG would result in Q_ASSERT being used in the QCONNECT_ASSERT macro, resulting in all of the chk_connect_x macros resolving to nothing and thus not connecting any of the signals.

In the Gentoo bug report, the user uploaded a patch reverting all of the chk_connect macros to straight up connect() calls and that seemed to resolve the issue. Presumably, defining NDEBUG would have the same result.
Comment 15 michael 2021-05-13 19:04:13 UTC
Yes that is correct if Qt thinks its a release build but NDEBUG is defined that would explain what's happpening. That should be an easy fix.
Comment 16 michael 2021-05-13 19:26:41 UTC
The offending Qt lines are here:
#if !defined(Q_ASSERT)
#  if defined(QT_NO_DEBUG) && !defined(QT_FORCE_ASSERTS)
#    define Q_ASSERT(cond) static_cast<void>(false && (cond))
#  else
#    define Q_ASSERT(cond) ((cond) ? static_cast<void>(0) : qt_assert(#cond, __FILE__, __LINE__))
#  endif
#endif

The expression static_cast<void>(false && (cond)) is produced when qt thinks this Q_ASSERT should be disabled. Since it seems to be problematic on some configurations Q_ASSERT should not be used for anything that has side effects.

I will push a fix shortly to replace Q_ASSERT with assert in the connection macros. This will prevent the issue from reoccurring. Build with an explicit CMAKE_BUILD_TYPE=Debug should workaround it until the hot fix 1.9.1 release because available later today or tomorrow. This bug will be closed when the patch is pushed to git.
Comment 17 michael 2021-05-13 20:13:46 UTC
Git commit 18d3db26dea575c1b8f418d23ccb93ecc9df026f by Michael Reeves.
Committed on 13/05/2021 at 20:09.
Pushed by mreeves into branch '1.9'.

Avoid connect issues with NDEBUG beging defined when Qt thinks we're in release mode.

The offending Qt lines are here:
#if !defined(Q_ASSERT)
#  if defined(QT_NO_DEBUG) && !defined(QT_FORCE_ASSERTS)
#    define Q_ASSERT(cond) static_cast<void>(false && (cond))
#  else
#    define Q_ASSERT(cond) ((cond) ? static_cast<void>(0) : qt_assert(#cond, __FILE__, __LINE__))
#  endif
#endif

The expression static_cast<void>(false && (cond)) is produced when qt thinks this Q_ASSERT should be disabled. Since it this is a nop Q_ASSERT should not containing anything that has side effects as a parameter.
FIXED-IN:1.9.1

M  +5    -1    src/defmac.h

https://invent.kde.org/sdk/kdiff3/commit/18d3db26dea575c1b8f418d23ccb93ecc9df026f
Comment 18 michael 2021-05-13 20:17:09 UTC
Git commit 5feaf425a92361a5e1e40640cd58399290fae46e by Michael Reeves.
Committed on 13/05/2021 at 20:15.
Pushed by mreeves into branch 'master'.

Avoid connect issues with NDEBUG beging defined when Qt thinks we're in release mode.

The offending Qt lines are here:

The expression static_cast<void>(false && (cond)) is produced when qt thinks this Q_ASSERT should be disabled. Since it this is a nop Q_ASSERT should not containing anything that has side effects as a parameter.

(cherry picked from commit 18d3db26dea575c1b8f418d23ccb93ecc9df026f)

M  +5    -1    src/defmac.h

https://invent.kde.org/sdk/kdiff3/commit/5feaf425a92361a5e1e40640cd58399290fae46e
Comment 19 Wolfgang Bauer 2021-05-16 15:40:46 UTC
The fix broke kdiff3 in openSUSE, while 1.9.0 worked fine here.
The diff viewer is just black here now, and even clicking OK/Cancel in the start dialog has no effect.

The kdiff3 package is built with NDEBUG defined, Qt is built in release mode.

Btw, I don't see how this can make sense:
#if defined(QT_NO_DEBUG)
#undef NDEBUG
#endif

Shouldn't it actually *define* NDEBUG if QT_NO_DEBUG is defined?
Otherwise NDEBUG will *enable* the assert, which seems to be counter-intuitive IMHO. (NDEBUG is supposed to mean "no debug", isn't it?)

And I think the problem is exactly because assert() is also a no-op if NDEBUG is defined. (i.e. the #undef apparently doesn't affect all necessary places, it seems)
Comment 20 Wolfgang Bauer 2021-05-16 15:42:29 UTC
(In reply to Wolfgang Bauer from comment #19)
> Otherwise NDEBUG will *enable* the assert, which seems to be
> counter-intuitive IMHO. (NDEBUG is supposed to mean "no debug", isn't it?)
Sorry, that was non-sense, I actually wanted to say:
Otherwise QT_NO_DEBUG will *enable* the assert, which seems to be counter-intuitive IMHO.
Comment 21 Wolfgang Bauer 2021-05-16 16:21:58 UTC
(In reply to Wolfgang Bauer from comment #19)
> #if defined(QT_NO_DEBUG)
> #undef NDEBUG
> #endif
I tried it, and kdiff3 works again (with the same build options) if I change it to this:
#if defined(QT_NO_DEBUG)
#define NDEBUG
#endif

But it probably makes even more sense to remove that completely I think (which also works fine here).
As it uses assert() instead of Q_ASSERT() now, the QT_NO_DEBUG define should not matter anymore I suppose. (and why should it not be possible to build kdiff3 with debug output just because Qt is built without?)
Comment 22 michael 2021-05-16 18:28:59 UTC
Created attachment 138489 [details]
Proposed fix

Does the attached patch fix this for everyone?
Comment 23 Wolfgang Bauer 2021-05-16 19:37:55 UTC
(In reply to michael from comment #22)
> Created attachment 138489 [details]
> Proposed fix
> 
> Does the attached patch fix this for everyone?

I tried the patch on openSUSE with and without -DNDEBUG, and it works fine in both cases.
Comment 24 michael 2021-05-17 00:39:44 UTC
Git commit 2ba1a3e96b748467f4214975dc9c3eef9686a37e by Michael Reeves.
Committed on 17/05/2021 at 00:34.
Pushed by mreeves into branch '1.9'.

Remove erroneous #ifdef
FIXED-IN:1.9.2

M  +5    -0    src/autotests/CMakeLists.txt
A  +43   -0    src/autotests/connectiontest.cpp     [License: GPL(v2.0+)]
M  +0    -5    src/defmac.h

https://invent.kde.org/sdk/kdiff3/commit/2ba1a3e96b748467f4214975dc9c3eef9686a37e
Comment 25 michael 2021-05-17 00:45:23 UTC
Git commit ce4d7a9139a359b156c129caca4a3197ad7e9b4e by Michael Reeves.
Committed on 17/05/2021 at 00:32.
Pushed by mreeves into branch 'master'.

Remove erroneous #ifdef
FIXED-IN:1.9.2

M  +5    -0    src/autotests/CMakeLists.txt
A  +43   -0    src/autotests/connectiontest.cpp     [License: GPL(v2.0+)]
M  +0    -5    src/defmac.h

https://invent.kde.org/sdk/kdiff3/commit/ce4d7a9139a359b156c129caca4a3197ad7e9b4e