D16768: mage global menu screen aware

[Michail Vourlakos]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:3fa1cc6b7716: mage global menu screen aware (authored by 
mvourlakos).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16768?vs=45243=46031

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

AFFECTED FILES
  applets/appmenu/package/contents/ui/main.qml
  applets/appmenu/plugin/appmenumodel.cpp
  applets/appmenu/plugin/appmenumodel.h

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: mart, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D16768: mage global menu screen aware

[Michail Vourlakos]

mvourlakos added a subscriber: mart.
mvourlakos added a comment.


  @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...

REPOSITORY
  R120 Plasma Workspace

BRANCH
  appMenuScreen

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

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: mart, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D16768: mage global menu screen aware

[Michail Vourlakos]

mvourlakos updated this revision to Diff 45243.
mvourlakos added a comment.


  - improve check for window in current screen

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16768?vs=45193=45243

BRANCH
  appMenuScreen

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

AFFECTED FILES
  applets/appmenu/package/contents/ui/main.qml
  applets/appmenu/plugin/appmenumodel.cpp
  applets/appmenu/plugin/appmenumodel.h

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16768: mage global menu screen aware

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> mvourlakos wrote in appmenumodel.cpp:263
> you mean?
> 
>   setMenuHidden(info.isMinimized() || m_screenGeometry.isNull() || 
> !m_screenGeometry.contains(info.geometry().center()));

I mean

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

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

REPOSITORY
  R120 Plasma Workspace

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

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16768: mage global menu screen aware

[Michail Vourlakos]

mvourlakos updated this revision to Diff 45193.
mvourlakos added a comment.


  - remove confusing emit call

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16768?vs=45143=45193

BRANCH
  appMenuScreen

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

AFFECTED FILES
  applets/appmenu/package/contents/ui/main.qml
  applets/appmenu/plugin/appmenumodel.cpp
  applets/appmenu/plugin/appmenumodel.h

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16768: mage global menu screen aware

[Michail Vourlakos]

mvourlakos added inline comments.

INLINE COMMENTS

> davidedmundson wrote in appmenumodel.cpp:229
> why move this?

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);

> davidedmundson wrote in appmenumodel.cpp: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

you mean?

  setMenuHidden(info.isMinimized() || m_screenGeometry.isNull() || 
!m_screenGeometry.contains(info.geometry().center()));

REPOSITORY
  R120 Plasma Workspace

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

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16768: mage global menu screen aware

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> appmenumodel.cpp:85-86
> +connect(this, ::screenGeometryChanged, this, [this] {
> +emit onWindowChanged(m_currentWindowId);
> +});
> +

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

> appmenumodel.cpp:229
>  
> +m_currentWindowId = id;
> +

why move this?

> appmenumodel.cpp:263
> +KWindowInfo info(id, NET::WMState | NET::WMGeometry);
> +setMenuHidden(info.isMinimized() || 
> !m_screenGeometry.contains(info.geometry().center()));
> +}

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

REPOSITORY
  R120 Plasma Workspace

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

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16768: mage global menu screen aware

[Michail Vourlakos]

mvourlakos created this revision.
mvourlakos added reviewers: Plasma, broulik.
mvourlakos added a project: Plasma.
mvourlakos requested review of this revision.

REVISION SUMMARY
  --a new screenGeometry property is added in the
  AppMenuModel in order to be used for filtering
  windows based on their geometry.
  
  BUG: 384895
  Depends on D16715 

REPOSITORY
  R120 Plasma Workspace

BRANCH
  appMenuScreen

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

AFFECTED FILES
  applets/appmenu/package/contents/ui/main.qml
  applets/appmenu/plugin/appmenumodel.cpp
  applets/appmenu/plugin/appmenumodel.h

To: mvourlakos, #plasma, broulik
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart