D11584: Set a transient parent for SNI context menus
This revision was automatically updated to reflect the committed changes. Closed by commit R120:cf2d64fa9718: Set a transient parent for SNI context menus (authored by fvogt). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11584?vs=30279=30293 REVISION DETAIL https://phabricator.kde.org/D11584 AFFECTED FILES applets/systemtray/systemtray.cpp To: fvogt, #plasma, davidedmundson Cc: broulik, davidedmundson, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11584: Set a transient parent for SNI context menus
fvogt updated this revision to Diff 30279. fvogt added a comment. Split mouse.xy stuff into separate patch. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11584?vs=30251=30279 BRANCH snitest REVISION DETAIL https://phabricator.kde.org/D11584 AFFECTED FILES applets/systemtray/systemtray.cpp To: fvogt, #plasma Cc: broulik, davidedmundson, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11584: Set a transient parent for SNI context menus
fvogt added inline comments. INLINE COMMENTS > broulik wrote in StatusNotifierItem.qml:63 > I think this change is fine. Except that the `MouseArea` covers the entire > list item, so for hidden SNIs the app might get coordinates outside of its > icon. > > For context menu we ignore the coordinates anyway and place the menu relative > to the icon (so it never covers it). For `Activate` the app might want to > know where it was clicked. In any case this needs a bit of cleaning up, we > pass parameters around in places where they're ignored and so on. I think I'll do it differently. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11584 To: fvogt, #plasma Cc: broulik, davidedmundson, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11584: Set a transient parent for SNI context menus
broulik added inline comments.
INLINE COMMENTS
> StatusNotifierItem.qml:63
> onClicked: {
> -var pos = plasmoid.nativeInterface.popupPosition(taskIcon, 0, 0);
> +var pos = plasmoid.nativeInterface.popupPosition(taskIcon, mouse.x,
> mouse.y);
>
I think this change is fine. Except that the `MouseArea` covers the entire list
item, so for hidden SNIs the app might get coordinates outside of its icon.
For context menu we ignore the coordinates anyway and place the menu relative
to the icon (so it never covers it). For `Activate` the app might want to know
where it was clicked. In any case this needs a bit of cleaning up, we pass
parameters around in places where they're ignored and so on.
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D11584
To: fvogt, #plasma
Cc: broulik, davidedmundson, plasma-devel, ZrenBot, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
D11584: Set a transient parent for SNI context menus
fvogt added inline comments. INLINE COMMENTS > davidedmundson wrote in StatusNotifierItem.qml:63 > This change is unrelated, and not a change I would support. Why not? It is a fix for an oversight, which is currently a no-op due to another bug. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11584 To: fvogt, #plasma Cc: davidedmundson, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11584: Set a transient parent for SNI context menus
davidedmundson added inline comments.
INLINE COMMENTS
> StatusNotifierItem.qml:63
> onClicked: {
> -var pos = plasmoid.nativeInterface.popupPosition(taskIcon, 0, 0);
> +var pos = plasmoid.nativeInterface.popupPosition(taskIcon, mouse.x,
> mouse.y);
>
This change is unrelated, and not a change I would support.
> systemtray.cpp:297
> KAcceleratorManager::manage(menu);
> +menu->winId();
> +
> menu->windowHandle()->setTransientParent(statusNotifierIcon->window());
This part is all fine.
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D11584
To: fvogt, #plasma
Cc: davidedmundson, plasma-devel, ZrenBot, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
D11584: Set a transient parent for SNI context menus
fvogt updated this revision to Diff 30251. fvogt added a comment. Now for real. Please arc, cooperate. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11584?vs=30250=30251 BRANCH snitest REVISION DETAIL https://phabricator.kde.org/D11584 AFFECTED FILES applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml applets/systemtray/systemtray.cpp To: fvogt, #plasma Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11584: Set a transient parent for SNI context menus
fvogt updated this revision to Diff 30250. fvogt added a comment. Split into https://phabricator.kde.org/D11586 REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11584?vs=30248=30250 BRANCH snitest REVISION DETAIL https://phabricator.kde.org/D11584 AFFECTED FILES applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml applets/systemtray/systemtray.cpp libdbusmenuqt/dbusmenuimporter.cpp To: fvogt, #plasma Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11584: Set a transient parent for SNI context menus
fvogt updated this revision to Diff 30248. fvogt added a comment. libdbusmenu-qt: Remove nonexistant actions directly from the menu The getLayout response handler compares the new list of actions with the current actions in the menu and calls deleteLater on all actions which aren't part of the new list anymore. Then, it adds all actions from the new list which aren't part of the menu yet. As deleteLater only has an effect after the next event processing, the menu still contains them together with the added actions. This resulted in broken size calculations, as even for static menus the item count changed during aboutToShow. Note that this is not a proper solution for the resize issue, as the aboutToShow handler changes the menus content for a reason, the application is free to add/remove items at any point in time. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11584?vs=30245=30248 BRANCH snitest REVISION DETAIL https://phabricator.kde.org/D11584 AFFECTED FILES applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml applets/systemtray/systemtray.cpp libdbusmenuqt/dbusmenuimporter.cpp To: fvogt, #plasma Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11584: Set a transient parent for SNI context menus
fvogt added a comment. Urgh, arc. I'll split again. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11584 To: fvogt, #plasma Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11584: Set a transient parent for SNI context menus
fvogt added a comment. The blank line deletion should be ok, but someone should test on X11 as well. I don't expect any regressions there though. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11584 To: fvogt, #plasma Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11584: Set a transient parent for SNI context menus
fvogt created this revision. fvogt added a reviewer: Plasma. Restricted Application added a project: Plasma. fvogt requested review of this revision. REVISION SUMMARY Those had no transient parent set, so they got displayed somewhere, most of the time on the wrong screen. Also pass the actual click coordinates instead of (0,0), although those are ignored anyway. TEST PLAN Works fine for spotify and kteatime now, but certain applications which trigger a LayoutChanged signal on the opened event have a Y offset. REPOSITORY R120 Plasma Workspace BRANCH snitest REVISION DETAIL https://phabricator.kde.org/D11584 AFFECTED FILES applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml applets/systemtray/systemtray.cpp To: fvogt, #plasma Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
