D6531: Sublime::Container : set sorted open document list as Dock menu

[René J.V. Bertin]

rjvbb created this revision.
rjvbb added a project: KDevelop.
Restricted Application added a subscriber: kdevelop-devel.

REVISION SUMMARY
Another small improvement of Mac integration: this change designates the sorted `documentListMenu` as the Dock menu. The context menu under the KDevelop's Dock tile will then show the list of open documents and provide an additional way of selecting them

TEST PLAN
works as expected on Mac. The method is not available elsewhere so requires an #ifdef.

REPOSITORY
R33 KDevPlatform

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

AFFECTED FILES
sublime/container.cpp

To: rjvbb, #kdevelop
Cc: kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[Kevin Funk]

kfunk accepted this revision.
kfunk added a comment.
This revision is now accepted and ready to land.


Cool, thanks!

Potentially useful to Kate/KWrite as well?

REPOSITORY
R33 KDevPlatform

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

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[Kevin Funk]

kfunk added a comment.


Ah: How does this behave if we have multiple containers? That happens when we use split views I think.

REPOSITORY
R33 KDevPlatform

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

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb added a comment.

Post by Kevin Funk
Potentially useful to Kate/KWrite as well?

Yes, to any application that already provides a list of open documents in a menu. Same applies to the windowFilePath change. In both cases I hook on to features that aren't inherited but implemented completely in KDevPlatform, so the Kate devs will have to roll their own support.

REPOSITORY
R33 KDevPlatform

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

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb added a comment.

Post by Kevin Funk
Ah: How does this behave if we have multiple containers? That happens when we use split views I think.

You don't want to know ;)

Requesting a vertical split:

frame #3: 0x000000010ef94aa5 libKF5Crash.5.dylib`KCrash::defaultCrashHandler(sig=6) + 1061 at kcrash.cpp:530
frame #4: 0x00007fff8b4d25aa libsystem_platform.dylib`_sigtramp + 26
frame #5: 0x00007fff8870a867 libsystem_kernel.dylib`__pthread_kill + 11
frame #6: 0x00007fff927edb26 libsystem_c.dylib`abort + 125
frame #7: 0x00007fff9061e07f libsystem_malloc.dylib`free + 411
frame #8: 0x0000000111b58e48 QtCore`QObjectPrivate::deleteChildren(this=0x00007fe6bef4b8c0) + 200 at qobject.cpp:1970
frame #9: 0x0000000110b8885d QtWidgets`QWidget::~QWidget(this=0x00007fe6bef4b6d0) + 1053 at qwidget.cpp:1694
frame #10: 0x000000010f573c6e libKDevPlatformSublime.10.dylib`Sublime::ContainerTabBar::~ContainerTabBar() [inlined] Sublime::ContainerTabBar::~ContainerTabBar(this=0x00007fe6bef4b6d0) + 14 at container.cpp:50
frame #11: 0x000000010f573c69 libKDevPlatformSublime.10.dylib`Sublime::ContainerTabBar::~ContainerTabBar() [inlined] Sublime::ContainerTabBar::~ContainerTabBar(this=0x00007fe6bef4b6d0) at container.cpp:50
frame #12: 0x000000010f573c69 libKDevPlatformSublime.10.dylib`Sublime::ContainerTabBar::~ContainerTabBar(this=0x00007fe6bef4b6d0) + 9 at container.cpp:50
frame #13: 0x0000000111b58e48 QtCore`QObjectPrivate::deleteChildren(this=0x00007fe6bef49cf0) + 200 at qobject.cpp:1970
frame #14: 0x0000000110b8885d QtWidgets`QWidget::~QWidget(this=0x00007fe6bef49240) + 1053 at qwidget.cpp:1694
frame #15: 0x000000010f57184e libKDevPlatformSublime.10.dylib`Sublime::Container::~Container() [inlined] Sublime::Container::~Container(this=0x00007fe6bef49240) + 14 at container.cpp:350
frame #16: 0x000000010f571849 libKDevPlatformSublime.10.dylib`Sublime::Container::~Container(this=0x00007fe6bef49240) + 9 at container.cpp:350
frame #17: 0x000000010f5831b9 libKDevPlatformSublime.10.dylib`Sublime::MainWindowPrivate::viewAdded(this=<unavailable>, index=<unavailable>, view=0x00007fe6abbe46a0) + 521 at mainwindow_p.cpp:558

I don't see a direct link to the dockmenu. Probably the safest would be to unregister the dock menu before updating the documentListMenu, and register it afterwards. I'd have to test that.

REPOSITORY
R33 KDevPlatform

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

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[Kevin Funk]

kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.


Okay, please investigate.

REPOSITORY
R33 KDevPlatform

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

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

In fact, that crash happens when I activate (top/bottom) split mode through the context menu obtained by right-clicking on the document tab and thus requires a separate investigation.

[Kevin Funk]

kfunk added a comment.

Post by René J.V. Bertin
In fact, that crash happens when I activate (top/bottom) split mode through the context menu obtained by right-clicking on the document tab and thus requires a separate investigation.

Can reproduce, I'll have a look.

REPOSITORY
R33 KDevPlatform

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

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb added a comment.


I must admit I use split mode so infrequently that I never really registered that each view has its own document list menu.

How can we integrate these two features? I see two global approaches:

1. Let the Dock menu reflect the doc list of the active view
2. Somehow merge all document list menus and show an exhaustive menu in the Dock

Option 1) will be the simpler one to implement provided there is (or can be) a Container method/slot that gets called when you activate one of its documents. I tried `Container::widgetActivated()` but that one apparently doesn't get called when you click in a document that was already open (but in a different container than the current one). It could also be the more confusing implementation.

Option 2) will depend on whether user Dock menus can have submenus (I think but to be confirmed) or else we'd need to use separators between the lists from different containers. Do these containers have internal names that have meaning for the user or else how to name the submenus? And of course, who will own the merged menu (or do we make it a global static)?

REPOSITORY
R33 KDevPlatform

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

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[Kevin Funk]

kfunk added a comment.


I'd just go for option (1) for now. Simpler, and I don't think split views are *that* popular.

Question is, can `QMenu::setAsDockMenu()` be invoked multiple times? Will it just replace the current dock menu?

REPOSITORY
R33 KDevPlatform

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

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb added a comment.


Yes, it just replacesinternally this sets just an ObjC class variable (the internal NSMenu* corresponding to the QMenu) which is returned by [NSApplicationDelegate applicationDockMenu], retaining the new instance and releasing the previous.

The real question is if there's a method that gets called reliably whenever you click in a view belonging to a container (that's not the current). Do you think it would be exhaustive enough to implement a Container::mousePressEvent() override, or is there a more generic (Container) function that always gets called whenever a document gets the focus? We'd also need to cover the case when a Container is closed with its last open document, and another one becomes active. `Container::focusInEvent()`, maybe?

REPOSITORY
R33 KDevPlatform

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

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb added a comment.


I just noticed something.

Does the document list menu work reliably for you when you select a document that is open in multiple containers?

For me, this will show the document in the corresponding container, but the keyboard focus doesn't follow reliably. Typically it remains where it was, or else is put somewhere else entirely (not in a document at all).

REPOSITORY
R33 KDevPlatform

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

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb updated this revision to Diff 16320.
rjvbb added a comment.


This version sets the Dock menu in the documentListMenu updated method (if not yet set), uses a focusInEvent() override to change the Dock menu when a different container is selected, and unsets the menu in the Container dtor. This mostly works; there are still times when the Dock menu remains empty when it shouldn't.

I haven't checked if these functions are always called from a single thread only; are they, or can race conditions occur?

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6531?vs=16252&id=16320

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

AFFECTED FILES
sublime/container.cpp
sublime/container.h

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger