D9289: KCompletionBox: restore proper layering behaviour on Mac

[René J.V. Bertin]

rjvbb created this revision.
rjvbb added a reviewer: Framework: Syntax Highlighting.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
This restores proper layering behaviour on Mac, fixing the regression described in the linked bug report (which has screenshots).

BUG: 387796

TEST PLAN
Changing the window type from Qt::Window to Qt::Dialog was the last tweak I tried and the only one that causes the completion list to display in front of the grandparent window in all known situations on Mac/Cocoa, Mac/X11 and Linux/X11 (even an explicit `raise()` in the evidently locations had no effect).
It seems thus safe to apply unconditionally, or else with a simple #ifdef Q_OS_MACOS .

REPOSITORY
R284 KCompletion

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

AFFECTED FILES
src/kcompletionbox.cpp

To: rjvbb, #framework_syntax_highlighting
Cc: kde-mac, #frameworks

[René J.V. Bertin]

rjvbb edited the summary of this revision.

REPOSITORY
R284 KCompletion

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

To: rjvbb, #framework_syntax_highlighting
Cc: kde-mac, #frameworks

[René J.V. Bertin]

rjvbb edited reviewers, added: Frameworks; removed: Framework: Syntax Highlighting.
rjvbb removed a subscriber: Frameworks.

REPOSITORY
R284 KCompletion

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

To: rjvbb, #frameworks
Cc: kde-mac

[Luigi Toscano]

ltoscano edited the summary of this revision.

REPOSITORY
R284 KCompletion

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

To: rjvbb, #frameworks
Cc: kde-mac

[René J.V. Bertin]

rjvbb added a comment.


kurlrequestertest_gui on Mac, unpatched frameworks 5.38, Qt 5.8.0:
F5542277: Screen Shot 2017-12-11 at 15.40.13.png <https://phabricator.kde.org/F5542277>

idem, with this patch:
F5542279: Screen Shot 2017-12-11 at 21.09.08.png <https://phabricator.kde.org/F5542279>

REPOSITORY
R284 KCompletion

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

To: rjvbb, #frameworks
Cc: kde-mac

[René J.V. Bertin]

ping?

Should I just commit this?

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

kcompletionbox.cpp:68
q->setAttribute(Qt::WA_ShowWithoutActivating);
- q->setWindowFlags(Qt::Window | Qt::FramelessWindowHint | Qt::BypassWindowManagerHint); // calls setVisible, so must be done after initializations
+ q->setWindowFlags(Qt::Dialog | Qt::FramelessWindowHint | Qt::BypassWindowManagerHint); // calls setVisible, so must be done after initializations
q->setUniformItemSizes(true);

This looks wrong to me, it's not a dialog. Can you try

q->setWindowFlags(q->windowFlags() | Qt::FramelessWindowHint | Qt::BypassWindowManagerHint);

or Qt::Popup | ...
When you test with open completion box minimize or switch window (alt+tab) should dismiss it.

REPOSITORY
R284 KCompletion

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

To: rjvbb, #frameworks
Cc: anthonyfieroni, kde-mac

[René J.V. Bertin]

rjvbb added a comment.

Post by Anthony Fieroni
This looks wrong to me, it's not a dialog

But how often is the parent not a dialog?
I don't notice any difference under X11 whether I use Qt::Window or Qt::Dialog, apparently those are effectively equivalent in this situation. But they're clearly not on Mac.

My guess would be that if the parent has some sort of modal character the completion box should have at least that too ... or else it will remain behind the parent.

Post by Anthony Fieroni
q->setWindowFlags(q->windowFlags() | Qt::FramelessWindowHint | Qt::BypassWindowManagerHint);

With this the completion box doesn't even appear.

Post by Anthony Fieroni
or Qt::Popup | ...

This steals focus (on Mac/Cocoa and even grabs the keyboard under Mac/X11).

Did you try either of your suggestions yourself? It'd be more efficient if you didn't ask me to try all kinds of alternatives that don't work on your end ;)

Post by Anthony Fieroni
When you test with open completion box minimize or switch window (alt+tab) should dismiss it.

Esc should too.

REPOSITORY
R284 KCompletion

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

To: rjvbb, #frameworks
Cc: anthonyfieroni, kde-mac

[Anthony Fieroni]

anthonyfieroni added a comment.

Post by René J.V. Bertin
Did you try either of your suggestions yourself? It'd be more efficient if you didn't ask me to try all kinds of alternatives that don't work on your end ;)

No :) I suggest you because you're on.

REPOSITORY
R284 KCompletion

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

To: rjvbb, #frameworks
Cc: anthonyfieroni, kde-mac

[René J.V. Bertin]

rjvbb added a comment.

Post by Anthony Fieroni
No :) I suggest you because you're on.

Well, I could test anything but there isn't much point in testing something we already know won't work elsewhere. It's not like I can test multiple systems at once either.

REPOSITORY
R284 KCompletion

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

To: rjvbb, #frameworks
Cc: anthonyfieroni, kde-mac