Differential D16768

mage global menu screen aware
ClosedPublic
Actions

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

Details

Reviewers
broulik
davidedmundson
Group Reviewers
Plasma
Commits
R120:3fa1cc6b7716: mage global menu screen aware
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
Branch
appMenuScreen
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4804
Build 4822: arc lint + arc unit
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
mvourlakos added a subscriber: mart.Nov 18 2018, 7:04 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
Closed by commit R120:3fa1cc6b7716: mage global menu screen aware (authored by mvourlakos).  ·  Explain WhyNov 22 2018, 6:37 PM
This revision was automatically updated to reflect the committed changes.
Revision Contents
Changeset List
Diff 45243
View Options
applets/appmenu/package/contents/ui/main.qml
Loading...
View Options
applets/appmenu/plugin/appmenumodel.h
Loading...
View Options
applets/appmenu/plugin/appmenumodel.cpp
Loading...