Summary: | KFileDialog Options drop-down menu grabs keyboard and mouse with QtCurve style | ||
---|---|---|---|
Product: | [Frameworks and Libraries] QtCurve | Reporter: | RJVB <rjvbertin> |
Component: | qt5 | Assignee: | Yichao Yu <yyc1992> |
Status: | REOPENED --- | ||
Severity: | major | CC: | egorov_egor, hein, jazzvoid, kde, kdelibs-bugs, leszek.lesner, tsujan2000, yyc1992 |
Priority: | NOR | ||
Version: | git | ||
Target Milestone: | --- | ||
Platform: | Compiled Sources | ||
OS: | All | ||
Latest Commit: | Version Fixed In: | ||
Sentry Crash Report: | |||
Attachments: |
workaround patch
corner effect on Mac x11 menus translucent/rounded menus patch MkII |
Description
RJVB
2016-12-27 19:08:14 UTC
I fear this is actually an issue in QFileDialog ... After some more digging around it seems that the Options menu is provided by KIO's KFileWidget (?) Created attachment 103038 [details]
workaround patch
For now I'm applying this workaround patch which forces a different widget style on the options menu, instead of (and only of) qtcurve. I made it possible to override the workaround because it'd be nice to figure out what is going on here.
Is it possible that this issue is caused by something in the aboutToShow delegate?
Comment on attachment 103038 [details]
workaround patch
the same hack has to be applied to `KDirOperator::setupMenu(int whichActions)` (at the end of the method).
Over my dead body ;-) This needs debugging, and it sounds like the bug is in QtCurve itself, if it doesn't happen with other widget styles. I'm not saying those patches should go into the code... but they *can* be used by anyone who wants to look into this. Debugging yes, but how (and with that I don't only mean "how do you get back keyboard and mouse control")? The freeze/whatever happens nowhere near QtCurve code, and could be triggered well before QtCurve code is called. To make this even more subtle and beard-pulling at the same time: the freeze only occurs with the toplevel context menus. Its submenus are fine, provided you can get at them. In addition, the KDirOperator context menu only seems to hang when used from within a KFileWidget. That is presuming that it's the same context menu that is being shown when you right-click an entry in KDevelop's file manager toolview; that particular menu just works. And I think that this observation put much of the ball back in the KIO camp ;) IOW, this may not be a bug in the usual sense of the term. There are many context menus around KDE apps, but these 2 are the only ones I discovered that fail to work properly. @Martin Gräßlin: are you seeing this? I'm on Linux (Manjaro and Debian). This problem happens only when QtCurve menus are translucent. Then, a freeze occurs when an options dropdown submenu of KFileDialog is supposed to be shown by selecting a menu-item. For me, the only way out of this situation has been ctrl+atl+f2 and restarting sddm manually. Qt5 window translucency requires setting of RGBA format before windows have native handles. QtCurve does it in `Style::prePolish()` and `addAlphaChannel()`, using private headers. Kvantum does the same thing in `Style::setSurfaceFormat()` but without private headers. Now, in Kvantum, I encountered exactly the same freeze ONLY WITH KFileDialog. As a workaround, I excluded menus from `setSurfaceFormat()` because most menus didn't need it for translucency (there are rare exceptions when both WA_WState_Created and WA_NativeWindow are set for a menu, in which case, they don't have translucency in Kvantum). It isn't strange that this only happens with QtCurve because no other Qt5 style engine (except for Kvantum, that has a workaround) supports translucent menus. I haven't read the code of KFileDialog but as far as I know, QtCurve doesn't do anything invalid. Therefore, I think the problem is in KFileDialog. Ah, thanks! This sounds like a reasonable beginning of explanation that just leaves 2 questions open: - why is only the toplevel pop-up menu concerned, i.e. why can I open its submenus normally when rendered in QtCurve as long as I use another widget style for the parent menu? - I don't have translucency configured in my QtCurve settings (all 3 parameters are 100% opaque). That suggests that the operations required to obtain translucency are what triggers the behaviour, not the translucency itself, and offers the possibility of another workaround (behave as if translucency isn't supported when not requested). @RJVB Actually, I was lead to your report by google, while trying to know why KFileDialog is an exception and which source file I should read. Your patch says "kfilewidget.cpp". That will be a good start :) Heh, and your reaction taught about a widget style I never heard about, and which might be even more appropriate to implement an "OS X" lookalike that makes KDE applications actually looks good on Mac (without specific tuning) and not as if they're designed for the visually impaired ;) You'll also have to look in kdiroperator.cpp, that's where the context menu (for right-clicking on items in the file list) is defined. I think I understand why I'm running into issues without using menu translucency. I have rounded corners configured for popup menus, and that required some translucency. You don't happen to have a suggestion how to obtain that without calling addAlphaChannel() (or let it do only safe things), by chance? > You'll also have to look in kdiroperator.cpp, that's where the context menu (for right-clicking on items in the file list) is defined. Thanks! I will soon. > I have rounded corners configured for popup menus, and that required some translucency. That's correct. Anyway, I'll report back as soon as I find something in those files. I haven't found anything in kio yet but I'm pretty sure about this: The freeze happens when a window handle is going to be created for those menus, whether by setting WA_NativeWindow to `true` temporarily or by using `createTLExtra()` and `createTLSysExtra()`, as QtCurve does. I mean, the freeze isn't related to setting the format -- `setFormat()` -- but to the creation of a window handle for menus. OK, I may be wrong but I couldn't find any problem in kio. Of course, someone knowledgeable about kio would know better. I tested with Kvantum and found that ensurePolished() (only at Style::styleHint()) is enough for all menus to have translucency and/or rounded corners (will make a commit after some tests). I think QtCurve can do the same, i.e. it can use ensurePolished() instead of addAlphaChannel() for menus -- although, as I mentioned above, most menus wouldn't need it. In other words, addAlphaChannel() should be used only for window translucency. All this trouble is because of https://bugreports.qt.io/browse/QTBUG-34064 P.S. This question isn't answered yet: why does the creation of a window handle for KFileDialog's menu cause a freeze. You sire (sic) are a saviour! I rewrote addAlphaChannel() so basically it does if (qobject_cast<QMenu*>(widget)) { widget->ensurePolished(); } else { // previous addAlphaChannel code } and that works around the issue on X11. I have yet to test on Mac. Interesting observation: adding a qWarning() print probe before calling widget->ensurePolished() shows that this was called during kdialog's startup, regardless of whether I used the menu or not. No output was generated when using the menu. Glad to know it works for QtCurve too! The developer(s) of QtCurve should be informed about the whole problem and its solution. They are ;) Created attachment 103233 [details]
corner effect on Mac
Well, the fix does have some side-effects on Mac; rounded corners don't look so nice anymore there, but show their background behind the rounded-off corners (I've set the corners to max. rounded so it shows better in this screenshot).
It usually doesn't happen under X11 but more testing appears to be required.
Strange! The whole point of ensurePolished() is to call `QStyle::polish()` before creation of native windows. Here, on x11, it works not only with menus but also with most top level widgets, although it's safe only with menus. And in "qwidget.cpp" → `QWidget::ensurePolished()`, there isn't any Mac-specific line. Created attachment 103238 [details]
x11 menus
All menus are OK on Linux (x11) now. Qt's behavior should be different in Mac.
This is probably more an issue with the underlying OpenGL implementation, maybe even the drivers for my GPU (a 2011 i7 with an Intel HD3000). I see the same glitch when I use the XCB (X11) plugin on Mac. FWIW, I just tried under Wayland, using Qt 5.7.1's qtwayland/examples/wayland/qwindow-compositor . Same effect without the patch as on Mac: the menu freezes but can be unlocked by clicking on the desktop background. Any extra CPU usage under wayland or Mac? I have a relatively new laptop with Intel and an old desktop computer with nVidia. All menus are OK on both computers. Wayland only works on Linux, i.e. a 2016 Intel N3150 with "Cherryview" graphics. No idea if there's extra CPU usage that isn't simply due to inefficiency in the simple Qt Wayland compositor. On the Mac: compared to what? Even at 5y old, a 2.7Ghz i7 is still very powerful for rendering just a drop-down menu... OK, I just wanted to know if the freeze was caused by a loop, in which case you'd see a difference. Before using ensurePolished(), I didn't see a high CPU usage either. The freeze -- without ensurePolished() -- is still a mystery to me. I'm not good at tolerating mysteries ;) No, if the freeze were a deadloop one couldn't get out of it by clicking swapping focus back and forth. Although satisfied with the "ensurePolished" solution on x11, I reverted to the freeze code (with Kvantum) under KDE to study the problem a little and found that actually there was a way to close the menu without killing any app. I'd assigned the super key to the Plama main menu; so by pressing that key, the main menu was shown and the KFileDialog menu was closed. I used that opportunity to see which QEvents were triggered on opening the freezing menu and to compare them with those of a normal menu. Freezing menus lacked ChildPolished, PolishRequest and MouseButtonRelease events and instead, had an extra ActionChanged event. Much to my surprise, after playing with various codes, I found out that this is really a QtCurve bug! Let me summarize the problem and its new solution: To make Qt windows translucent, we should set the surface format of their native handles BEFORE they're created. For Qt4, that could be done in "Style::polish()". But Qt5 windows are polished AFTER they're created. Therefore, setting of "WA_TranslucentBackground" in "Style::polish()" would have no effect with Qt5. Early creation of native handles (as is done by QtCurve) could have unpredictable side effects -- for menus if not for windows -- because we don't know how it would affect the complex process of widget creation. However, setting of "WA_TranslucentBackground" in an early stage BEFORE the widget creation sets the alpha buffer size to 8 automatically and safely. When I implemented this logic in Kvantum (offline for now), all problems, that I'd considered as translucency bugs of Qt >= 5.7, were gone :) Doing the same thing for QtCurve isn't so easy because it requires changing of the code structure. (Fortunately, in Kvantum, I'd used "QSet<const QWidget*>" to track translucent widgets.) I don't know whether setting of "Qt::WA_TranslucentBackground" to "true" before widget creation has the same effect on Mac too but if the answer is no, there won't be a safe way to make Qt5 windows/menus translucent/rounded on Mac. @RJVB Thank you for this report, without which I wouldn't have enough motivation to wrestle with this problem! The usage is basically unsupported by Qt and is a hack from the very beginning so unless Qt provide a proper way to do it this is always a bug of the style. The only question is that what's a better way to hack it. Setting `WA_TranslucentBackground` was indeed what I started with https://github.com/QtCurve/qtcurve/blame/58a4438b901311914d0d453c3987bfabbf258047/qt5/style/qtcurve.cpp#L406 . However, (at least in Qt 5.0/5.1) it causes many side effects that needs to be undone and calling the private API/interact with the backend directly caused much less trouble so I decided to switch to the current approach in https://github.com/QtCurve/qtcurve/commit/c1543d2f27e5ac6f6424537a53e3764233967e68 The main issue with setting `WA_TranslucentBackground` is that applications actually test/set it and messing with it creates lots of confusions in the application/high level libs (iirc kwin and plasma together with their widget libraries caused a lot of issues). OTOH, there are much fewer backends so backend specific hacks are much easier to write without worrying about application specific behaviors. Therefore, IMHO, the best solution (other than having deeper support from Qt) is to interact with the backend directly and make sure the native windows created have the correct events etc. @ Yichao Yu I didn't work with QtCurve's code but, both practically and theoretically, I'm sure that setting "WA_TranslucentBackground" before widget creation is the safest solution. The code structures of QtCurve and Kvantum are very different from each other. By "before creation" I mean only at "Style::styleHint()". Here, it works flawlessly with Kvantum (I'll push the changes after testing for 2 days). I can't think of a reason why it shouldn't work with QtCurve after some changes to the code structure. I forgot to say that I successfully tested this solution on two machines (one very old and with nvidia, the other new and with Intel) and also on virtualbox, with Qt-5.7.1 and Qt-5.5.x. All tests are done with Linux+x11. I happily changed my active Kvantum themes to translucent ones on both machines -- no fear of Qt5 window+menu translucency anymore ;) Do you think you could try to get Kvantum to work on Mac? It currently builds on my development system but doesn't work properly (doesn't show any SVG, I'd guess), neither with the native Cocoa QPA nor with the XCB QPA. Yichao, since you clearly know this part of the code much better than I and actually understand what it does: do you think it could be possible to set WA_TranslucentBackground` early in addition to the current approach, at least for recent enough Qt versions where this solves our issue? Also, you mention issues with Qt 5.0/5.1 - what would be a reasonable earliest version to require? I'd guess 5.3.2 would be a good compromise but I don't know if that helps us here? @Tsu Jan: now that you know that the KFileWidget triggers a QtCurve bug, do you also know why it only happens with its 2 context menus? I'd guess it must be something KFileWidget does in setting up those menus because the one that's actually from KDirOperator works fine in other contexts. > Do you think you could try to get Kvantum to work on Mac? I don't have Mac and changing a code blindly isn't a good practice at all. However, any Mac PR that works fine and whose logic is understandable to me will be welcome :) > ... do you also know why it only happens with its 2 context menus? In this special case, my knowledge is based on experiment and reasonable guesses. On the one hand, the code definitely works without problem. On the other hand, setting "WA_TranslucentBackground" should be quite harmless because the creation of native handles is left to Qt, while an early creation of a native handle cannot be so. In fact, the problem discussed here didn't exist in Kvantum but I still saw weird reproducible effects with window translucency since Qt-5.7.0. And now, with the new method, they're all gone :) That being said, I don't know what part of the kio code (or perhaps Qt's code) causes that freeze; I just guess, with a high probability, that the freeze is is a result of an untimely creation of native handles. KIO developers might know better (if the problem isn't deep in Qt). > I don't have Mac and changing a code blindly isn't a good practice at all.
Of course, just like starting to hack blindly in unknown code that interacts with Qt on a rather low/internal level isn't very recommendable either ;)
To be honest, I'll need to convince myself first that the style might be of interest on Mac before I start investing time in it. I'm spread out thin enough as it is.
Created attachment 103308 [details]
translucent/rounded menus patch MkII
The hint about setting WA_TranslucentBackground in time was actually very helpful! Setting it in addAlphaChannel() resolves the corner issue that made the patch useless on Mac.
NB: there's a comment above the other place where WA_TranslucentBackground is set in the Qt5 style suggesting to move it to addAlphaChannel(). Maybe we can address that in a single go?
> Setting it [WA_TranslucentBackground] in addAlphaChannel() resolves the corner issue that made the patch useless on Mac.
If so, the solution should work there too. Good news for Mac!
I pushed the necessary changes to Kvantum. All apps seem happy with translucency :) -- of course, except for those that should have opaque windows (like many video players). I'd like to add that THERE ARE some Qt5 windows that are polished BEFORE being created (like in Qt4) and that most Qt5 menus are so too. The difference between Qt4 and Qt5 regarding translucency is mainly that Qt4 windows were ALWAYS polished before creation. Whether this is a bug in Qt5, I'm not sure now. However, it has surely complicated the translucency support in style plugins. (In reply to RJVB from comment #36) > Created attachment 103308 [details] > translucent/rounded menus patch MkII Yichao, what do you think, can I commit this patch? @RJVB My two cents: this may be good for menus on Mac but (1) It's too much, IMO; (2) The windows are made translucent in the old way. I assure you that the are (random) problems with that. You may not use window tanslucency but many Linux users choose QtCurve because of it ;) You said somewhere earlier on this ticket that menus don't need what addAlphaChannel does to be translucent, so that's why I modified the function *for menus*. Windows should still be treated the same way. I just checked with QtCurve's style editor: translucent menus behave as I would expect (= become unreadable quickly). > You said somewhere earlier on this ticket that menus don't need what addAlphaChannel does to be translucent I said that on x11 and for MOST menus, not all. > Windows should still be treated the same way. If you mean translucent windows, the old way is buggy, as I tried to explain before. > translucent menus behave as I would expect (= become unreadable quickly). I didn't get this part. What becaomes unreadable if they they behave as expected? The intention of the patch is only to address the issue of translucent menus or menus with rounded corners in a KFileWidget. Buggy or not, the "old way" has been working well enough apparently. I'm leaving it to Yichao to test the patch and decide what he wants to do, reject it, incorporate it or rewrite the whole code once more (or a combination of the last 2 options).
If we have to choose between preventing freezes like we've been seeing in KFileWidgets and preserving translucency in all menus I think it's clear which is more important ...
> What becaomes unreadable if they they behave as expected?
Translucent menus when opacity nears 50% or lower and the background isn't homogeneous.
OK. I just wanted to share all facts that I found because, as I said earlier, I owe them to this report. I can't say what should be done in QtCurve (not my responsibility). So, I have nothing to add anymore. > Yichao, what do you think, can I commit this patch?
Using WA_TranslucentBackground for QMenu sounds safe. Be sure to check if flipping the menu opacity setting to 100% after application started doesn't regress. (I can't remember which case(s) are correctly handled and which are not but I'm pretty sure it's one of the side effects related to setting the attribute)
The interaction between WA_TranslucentBackground with other settings is AFAIK still present in latest version of qtbase so I wouldn't use it for all widgets. That said I have main window opacity setting to 100% for a while so I didn't noticed that there are a few regressions there (most noticeably menubars) so more investigation is needed.
I assume the freeze only happens in kdialog 16.12? (I can't reproduce it on 16.08).
Is what you're asking in any way different from launching the QtCurve theme editor via `kcmshell5 style`, setting menu opacity to 50%, confirming they indeed become translucent (I had to detach the preview for that) and then putting the opacity back to 100%? Flipping back to 100% after starting the app with menu opacity at 50% works on Mac as far as I can tell. I'll test under X11 tomorrow. It would be good if this patch was tested on more than a single system before going in. I started noticing the freeze after updating to 16.12.0 and then a few days later, to Qt 5.7.1 . I think I downgraded to see if it was related to the upgrade but can't remember the outcome of that test. I use the menus involved only occasionally. > Is what you're asking in any way different from launching the QtCurve theme editor via `kcmshell5 style`, setting menu opacity to 50%, confirming they indeed become translucent (I had to detach the preview for that) and then putting the opacity back to 100%?
I usually use a different application. The effect to avoid is a completely transparent background and IIRC this can happen if the application/qt stops rendering the background since the style tells it not to by setting the attribute and then QtCurve also stops rendering it since it's the application/qt's job for non-translucent window. This should only happen for homogeneous background.
That makes it a bit difficult to check then... menu backgrounds are rarely completely homogeneous because you have a bit of whatever the menu is attached to (like the menubar), and the surrounding background. That's in fact what I meant with preferring not to be the only one to test test: there are too many possible combinations to test here. I now checked with a few applications on Mac and Linux/X11. On Linux the system did lock up for a bit when I flipped the menu opacity back to 100% (from 50) but I think that was because of swapping each and every QtCurve user back in. The application of interest had perfectly opaque menus afterwards. I wonder though, if this is a tricky thing to change at runtime, why not make it a change that requires an application restart? It's not like that is unheard of... By homogeneous I mean the setting is a single color. Recently I started to prepare Kvantum for wayland and found something that might be relevant to Qtcurve too: Although setting WA_TranslucentBackground works fine on X11 (with some precautions), it causes a lot of artifacts on wayland. Instead, on wayland, the alpha channel should be forced on the native handle (with or without private headers). I don't know the reason yet but, at least for now, wayland isn't happy with WA_TranslucentBackground. The problem discussed here still exists with KFileDialog menus - and only with them -- on wayland but, fortunately, there's no freeze anymore; the items of those menus can't be selected when menu translucency is enabled by forcing alpha channel. My tests are done under plasma-wayland-session-5.9.2 and also Weston (both on X11 and with its wayland session) on Linux. (In reply to RJVB from comment #39) > (In reply to RJVB from comment #36) > > Created attachment 103308 [details] > > translucent/rounded menus patch MkII > > Yichao, what do you think, can I commit this patch? This patch solve the https://bugs.kde.org/show_bug.cgi?id=368672 Thanks for your job! I think we'll need to have another look at this one before the release gets out. Hm. Now when I press CTRL+O in chome kdialog has craching with Application: kdialog (kdialog), signal: Segmentation fault Using host libthread_db library "/lib64/libthread_db.so.1". [Current thread is 1 (Thread 0x7f82aa1b8840 (LWP 7786))] Thread 2 (Thread 0x7f82a8097700 (LWP 7787)): #0 0x00007f82b6f6872d in poll () from /lib64/libc.so.6 #1 0x00007f82b45c78f2 in ?? () from /usr/lib64/libxcb.so.1 #2 0x00007f82b45c96f7 in xcb_wait_for_event () from /usr/lib64/libxcb.so.1 #3 0x00007f82a9dc9c69 in ?? () from /usr/lib64/libQt5XcbQpa.so.5 #4 0x00007f82b76c0e3c in ?? () from /usr/lib64/libQt5Core.so.5 #5 0x00007f82b300f3b4 in start_thread () from /lib64/libpthread.so.0 #6 0x00007f82b6f7176d in clone () from /lib64/libc.so.6 Thread 1 (Thread 0x7f82aa1b8840 (LWP 7786)): [KCrash Handler] #6 0x00007f82b76b9454 in QMutex::lock() () from /usr/lib64/libQt5Core.so.5 #7 0x00007f82baa39aaf in ?? () from /usr/lib64/libQt5DBus.so.5 #8 0x00007f82baa3ab5a in QDBusConnection::sessionBus() () from /usr/lib64/libQt5DBus.so.5 #9 0x00007f82a6d0fd60 in ?? () from /usr/lib64/qt5/plugins/styles/qtcurve.so #10 0x00007f82a6d15c70 in ?? () from /usr/lib64/qt5/plugins/styles/qtcurve.so #11 0x00007f82a6d15d49 in ?? () from /usr/lib64/qt5/plugins/styles/qtcurve.so #12 0x00007f82a6d478cb in ?? () from /usr/lib64/qt5/plugins/styles/qtcurve.so #13 0x00007f82a6d479e9 in ?? () from /usr/lib64/qt5/plugins/styles/qtcurve.so #14 0x00007f82b784c993 in ?? () from /usr/lib64/libQt5Core.so.5 #15 0x00007f82b7843782 in ?? () from /usr/lib64/libQt5Core.so.5 #16 0x00007f82b78438a9 in ?? () from /usr/lib64/libQt5Core.so.5 #17 0x00007f82b7882221 in QObject::~QObject() () from /usr/lib64/libQt5Core.so.5 #18 0x00007f82b7842cba in QFactoryLoader::~QFactoryLoader() () from /usr/lib64/libQt5Core.so.5 #19 0x00007f82b80f71a9 in ?? () from /usr/lib64/libQt5Widgets.so.5 #20 0x00007f82b6ebfaa8 in ?? () from /lib64/libc.so.6 #21 0x00007f82b6ebfaf5 in exit () from /lib64/libc.so.6 #22 0x00007f82b76cc263 in QCommandLineParser::showVersion() () from /usr/lib64/libQt5Core.so.5 #23 0x00007f82b76ce97b in QCommandLineParser::process(QStringList const&) () from /usr/lib64/libQt5Core.so.5 #24 0x000000000040bcc4 in ?? () #25 0x00007f82b6eaa670 in __libc_start_main () from /lib64/libc.so.6 #26 0x0000000000410029 in _start () That's an unrelated bug. Are you running a recent build from the master or 1.9 branch? We fixed a few DBus-related issues not long ago. Yes, you are right. Current master without this patch crached too. I'll open new issue... *** Bug 393462 has been marked as a duplicate of this bug. *** |