mage global menu screen aware
ClosedPublic

Authored by mvourlakos on Nov 8 2018, 10:42 PM.

Details

Summary

--a new screenGeometry property is added in the
AppMenuModel in order to be used for filtering
windows based on their geometry.

BUG: 384895, 395853

Test Plan

--validate that when moving a window between different screens the global menu applet is updated accordingly
--checkout that nothing has broke because of this new behavior

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mvourlakos requested review of this revision.Nov 8 2018, 10:42 PM
mvourlakos created this revision.
mvourlakos edited the test plan for this revision. (Show Details)Nov 8 2018, 10:43 PM
davidedmundson requested changes to this revision.Nov 9 2018, 6:16 PM
davidedmundson added a subscriber: davidedmundson.
davidedmundson added inline comments.
applets/appmenu/plugin/appmenumodel.cpp
85–86

this emit keyword is confusing, you're calling a slot.

229

why move this?

263

can we make this

m_screenGeometry.isNull || m_screenGeom.contains(...)

so that a user can not set a screen geometry to get windows on all screens

This revision now requires changes to proceed.Nov 9 2018, 6:16 PM
mvourlakos added inline comments.Nov 9 2018, 6:57 PM
applets/appmenu/plugin/appmenumodel.cpp
229

this patch depends on https://phabricator.kde.org/D16715 that has moved it...
the reason is that m_currentWindowId in current implementation is not
a current window id. it is used only for windows that delay their menu
creation. So I renamed it in patch D16715 to m_delayedMenuWindowId.

The m_currentWindowId needs to be set only for windows that create correctly their menus.
Maybe a better place is to place it with the setMenuHidden(false);

263

you mean?

setMenuHidden(info.isMinimized() || m_screenGeometry.isNull() || !m_screenGeometry.contains(info.geometry().center()));
mvourlakos updated this revision to Diff 45193.Nov 9 2018, 7:02 PM
  • remove confusing emit call
mvourlakos marked an inline comment as done.Nov 9 2018, 7:02 PM
davidedmundson added inline comments.Nov 10 2018, 4:09 PM
applets/appmenu/plugin/appmenumodel.cpp
263

I mean

const bool contained = m_screenGeometry.isNull() || m_screenGeometry.contains(info.geometry().center())

setMenuHidden(info.isMinimized() || !contained)

mvourlakos updated this revision to Diff 45243.Nov 10 2018, 4:41 PM
  • improve check for window in current screen
mvourlakos marked an inline comment as done.Nov 10 2018, 4:42 PM
davidedmundson accepted this revision.Nov 18 2018, 6:13 PM
This revision is now accepted and ready to land.Nov 18 2018, 6:13 PM

@davidedmundson I cant commit yet because @mart suggested to rename "menuHidden" to "visible" at https://phabricator.kde.org/D16715

what do you think is the best way to proceed?

I would prefer to accept first D16715 and after that I will rebase this one...

mvourlakos edited the summary of this revision. (Show Details)Nov 22 2018, 6:26 PM
This revision was automatically updated to reflect the committed changes.