Re: Review Request 129261: Hide the "Show Menu Bar" action if all the menubars are native

[Albert Astals Cid]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129261/
---

(Updated Dec. 30, 2016, 11:57 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit 2ae1ef1066c61f9552eea06248a4fda3dde071d5 by Albert Astals 
Cid to branch master.


Repository: kconfigwidgets


Description
---

Some applications have a "Show Menu Bar" action that is a bit silly on systems 
where the menubar is part of the shell (for example Unity 7).

This patch attempts to fix it by iterating all the main windows when they are 
shown and if all the menubars of all mainwindows are native, then hides the 
show menu bar action (basically erasing it from existence).

It's not the nicest of the codes and probably has some edge cases but works on 
the general case so i think it's worth the effort.


Diffs
-

  src/kstandardaction.cpp 89d011e 

Diff: https://git.reviewboard.kde.org/r/129261/diff/


Testing
---

Tried konsole, kate and dolphin under Unity 7 on Ubuntu 16.10

konsole and kate work fine (i.e. the action is gone from the menus and all is 
good)

dolphin is not 100% "perfectly behabed" (i.e. the "control" toolbar item is 
supposed to not be shown when menubars are shown and in this case it's shown) 
but it's not a regression and imho it's the dolphin code being a bit weird (i 
can propose a patch for it if this gets accepted)


Thanks,

Albert Astals Cid

Re: Review Request 129261: Hide the "Show Menu Bar" action if all the menubars are native

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129261/#review101667
---


Fix it, then Ship it!




Looks good now.

Indeed this is only an event filter on QMainWindows, not a qApp event filter, 
so it's fine.


src/kstandardaction.cpp (line 91)


trailing whitespace ;)


- David Faure


On Dec. 29, 2016, 11:01 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129261/
> ---
> 
> (Updated Dec. 29, 2016, 11:01 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kconfigwidgets
> 
> 
> Description
> ---
> 
> Some applications have a "Show Menu Bar" action that is a bit silly on 
> systems where the menubar is part of the shell (for example Unity 7).
> 
> This patch attempts to fix it by iterating all the main windows when they are 
> shown and if all the menubars of all mainwindows are native, then hides the 
> show menu bar action (basically erasing it from existence).
> 
> It's not the nicest of the codes and probably has some edge cases but works 
> on the general case so i think it's worth the effort.
> 
> 
> Diffs
> -
> 
>   src/kstandardaction.cpp 89d011e 
> 
> Diff: https://git.reviewboard.kde.org/r/129261/diff/
> 
> 
> Testing
> ---
> 
> Tried konsole, kate and dolphin under Unity 7 on Ubuntu 16.10
> 
> konsole and kate work fine (i.e. the action is gone from the menus and all is 
> good)
> 
> dolphin is not 100% "perfectly behabed" (i.e. the "control" toolbar item is 
> supposed to not be shown when menubars are shown and in this case it's shown) 
> but it's not a regression and imho it's the dolphin code being a bit weird (i 
> can propose a patch for it if this gets accepted)
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 129261: Hide the "Show Menu Bar" action if all the menubars are native

[Albert Astals Cid]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129261/
---

(Updated Dec. 29, 2016, 11:01 p.m.)


Review request for KDE Frameworks.


Repository: kconfigwidgets


Description
---

Some applications have a "Show Menu Bar" action that is a bit silly on systems 
where the menubar is part of the shell (for example Unity 7).

This patch attempts to fix it by iterating all the main windows when they are 
shown and if all the menubars of all mainwindows are native, then hides the 
show menu bar action (basically erasing it from existence).

It's not the nicest of the codes and probably has some edge cases but works on 
the general case so i think it's worth the effort.


Diffs (updated)
-

  src/kstandardaction.cpp 89d011e 

Diff: https://git.reviewboard.kde.org/r/129261/diff/


Testing
---

Tried konsole, kate and dolphin under Unity 7 on Ubuntu 16.10

konsole and kate work fine (i.e. the action is gone from the menus and all is 
good)

dolphin is not 100% "perfectly behabed" (i.e. the "control" toolbar item is 
supposed to not be shown when menubars are shown and in this case it's shown) 
but it's not a regression and imho it's the dolphin code being a bit weird (i 
can propose a patch for it if this gets accepted)


Thanks,

Albert Astals Cid

Re: Review Request 129261: Hide the "Show Menu Bar" action if all the menubars are native

[Albert Astals Cid]

> On Dec. 29, 2016, 9:24 a.m., David Faure wrote:
> > src/kstandardaction.cpp, line 199
> > 
> >
> > If an application creates many "show menu bar" actions, e.g. one per 
> > window in a many-windows process, then it will end up with many event 
> > filters (a somewhat costly construct).
> > 
> > I'm wondering if this shouldn't be a singleton, where you register all 
> > the show-menubar QActions, so that it can handle them all in one go. To 
> > handle removal, it would mean connecting to the destroyed() signal of the 
> > action in order to remove it from the list.
> > 
> > Hmm, on the other hand, storing wasChecked and wasVisible for every 
> > action becomes a bit cumbersome (QMap or dynamic properties).
> > 
> > So maybe ignore this. Just food for thought.

I honestly don't think in the grand-scheme of things, this event filter is a 
costly thing.


- Albert


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129261/#review101641
---


On Dec. 29, 2016, 11:01 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129261/
> ---
> 
> (Updated Dec. 29, 2016, 11:01 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kconfigwidgets
> 
> 
> Description
> ---
> 
> Some applications have a "Show Menu Bar" action that is a bit silly on 
> systems where the menubar is part of the shell (for example Unity 7).
> 
> This patch attempts to fix it by iterating all the main windows when they are 
> shown and if all the menubars of all mainwindows are native, then hides the 
> show menu bar action (basically erasing it from existence).
> 
> It's not the nicest of the codes and probably has some edge cases but works 
> on the general case so i think it's worth the effort.
> 
> 
> Diffs
> -
> 
>   src/kstandardaction.cpp 89d011e 
> 
> Diff: https://git.reviewboard.kde.org/r/129261/diff/
> 
> 
> Testing
> ---
> 
> Tried konsole, kate and dolphin under Unity 7 on Ubuntu 16.10
> 
> konsole and kate work fine (i.e. the action is gone from the menus and all is 
> good)
> 
> dolphin is not 100% "perfectly behabed" (i.e. the "control" toolbar item is 
> supposed to not be shown when menubars are shown and in this case it's shown) 
> but it's not a regression and imho it's the dolphin code being a bit weird (i 
> can propose a patch for it if this gets accepted)
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 129261: Hide the "Show Menu Bar" action if all the menubars are native

[Albert Astals Cid]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129261/
---

(Updated Dec. 28, 2016, 10:47 p.m.)


Review request for KDE Frameworks.


Repository: kconfigwidgets


Description
---

Some applications have a "Show Menu Bar" action that is a bit silly on systems 
where the menubar is part of the shell (for example Unity 7).

This patch attempts to fix it by iterating all the main windows when they are 
shown and if all the menubars of all mainwindows are native, then hides the 
show menu bar action (basically erasing it from existence).

It's not the nicest of the codes and probably has some edge cases but works on 
the general case so i think it's worth the effort.


Diffs (updated)
-

  src/kstandardaction.cpp 89d011e 

Diff: https://git.reviewboard.kde.org/r/129261/diff/


Testing
---

Tried konsole, kate and dolphin under Unity 7 on Ubuntu 16.10

konsole and kate work fine (i.e. the action is gone from the menus and all is 
good)

dolphin is not 100% "perfectly behabed" (i.e. the "control" toolbar item is 
supposed to not be shown when menubars are shown and in this case it's shown) 
but it's not a regression and imho it's the dolphin code being a bit weird (i 
can propose a patch for it if this gets accepted)


Thanks,

Albert Astals Cid

Re: Review Request 129261: Hide the "Show Menu Bar" action if all the menubars are native

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129261/#review100639
---


Fix it, then Ship it!





src/kstandardaction.cpp (line 201)


why not qobject_cast? (in all 3 places)


- David Faure


On Oct. 25, 2016, 10:15 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129261/
> ---
> 
> (Updated Oct. 25, 2016, 10:15 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kconfigwidgets
> 
> 
> Description
> ---
> 
> Some applications have a "Show Menu Bar" action that is a bit silly on 
> systems where the menubar is part of the shell (for example Unity 7).
> 
> This patch attempts to fix it by iterating all the main windows when they are 
> shown and if all the menubars of all mainwindows are native, then hides the 
> show menu bar action (basically erasing it from existence).
> 
> It's not the nicest of the codes and probably has some edge cases but works 
> on the general case so i think it's worth the effort.
> 
> 
> Diffs
> -
> 
>   src/kstandardaction.cpp 89d011e 
> 
> Diff: https://git.reviewboard.kde.org/r/129261/diff/
> 
> 
> Testing
> ---
> 
> Tried konsole, kate and dolphin under Unity 7 on Ubuntu 16.10
> 
> konsole and kate work fine (i.e. the action is gone from the menus and all is 
> good)
> 
> dolphin is not 100% "perfectly behabed" (i.e. the "control" toolbar item is 
> supposed to not be shown when menubars are shown and in this case it's shown) 
> but it's not a regression and imho it's the dolphin code being a bit weird (i 
> can propose a patch for it if this gets accepted)
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 129261: Hide the "Show Menu Bar" action if all the menubars are native

[Kai Uwe Broulik]

> On Okt. 27, 2016, 10:38 vorm., Kai Uwe Broulik wrote:
> > The problem with isNativeMenuBar() is that it returns true if the window 
> > *may* be a native menu bar, for example when the platform theme removes the 
> > AA_DontUseNativeMenuBar qApp flag. This does not mean that the platform 
> > theme actually created a native menu bar for the window - which ours does 
> > not if no global menu service is available.
> > 
> > Changing it from checking whether platformMenuBar() returns something fixes 
> > this for me and the entry behaves properly, being hidden when global menu 
> > is available, and being visible if not.

There's a Qt patch that fixes this 
https://code.qt.io/cgit/qt/qtbase.git/commit/?id=835d7cf54328bdd93d58bb64ed96a9c322580aea


- Kai Uwe


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129261/#review100331
---


On Okt. 25, 2016, 10:15 nachm., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129261/
> ---
> 
> (Updated Okt. 25, 2016, 10:15 nachm.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kconfigwidgets
> 
> 
> Description
> ---
> 
> Some applications have a "Show Menu Bar" action that is a bit silly on 
> systems where the menubar is part of the shell (for example Unity 7).
> 
> This patch attempts to fix it by iterating all the main windows when they are 
> shown and if all the menubars of all mainwindows are native, then hides the 
> show menu bar action (basically erasing it from existence).
> 
> It's not the nicest of the codes and probably has some edge cases but works 
> on the general case so i think it's worth the effort.
> 
> 
> Diffs
> -
> 
>   src/kstandardaction.cpp 89d011e 
> 
> Diff: https://git.reviewboard.kde.org/r/129261/diff/
> 
> 
> Testing
> ---
> 
> Tried konsole, kate and dolphin under Unity 7 on Ubuntu 16.10
> 
> konsole and kate work fine (i.e. the action is gone from the menus and all is 
> good)
> 
> dolphin is not 100% "perfectly behabed" (i.e. the "control" toolbar item is 
> supposed to not be shown when menubars are shown and in this case it's shown) 
> but it's not a regression and imho it's the dolphin code being a bit weird (i 
> can propose a patch for it if this gets accepted)
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 129261: Hide the "Show Menu Bar" action if all the menubars are native

[Kai Uwe Broulik]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129261/#review100331
---



The problem with isNativeMenuBar() is that it returns true if the window *may* 
be a native menu bar, for example when the platform theme removes the 
AA_DontUseNativeMenuBar qApp flag. This does not mean that the platform theme 
actually created a native menu bar for the window - which ours does not if no 
global menu service is available.

Changing it from checking whether platformMenuBar() returns something fixes 
this for me and the entry behaves properly, being hidden when global menu is 
available, and being visible if not.

- Kai Uwe Broulik


On Okt. 25, 2016, 10:15 nachm., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129261/
> ---
> 
> (Updated Okt. 25, 2016, 10:15 nachm.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kconfigwidgets
> 
> 
> Description
> ---
> 
> Some applications have a "Show Menu Bar" action that is a bit silly on 
> systems where the menubar is part of the shell (for example Unity 7).
> 
> This patch attempts to fix it by iterating all the main windows when they are 
> shown and if all the menubars of all mainwindows are native, then hides the 
> show menu bar action (basically erasing it from existence).
> 
> It's not the nicest of the codes and probably has some edge cases but works 
> on the general case so i think it's worth the effort.
> 
> 
> Diffs
> -
> 
>   src/kstandardaction.cpp 89d011e 
> 
> Diff: https://git.reviewboard.kde.org/r/129261/diff/
> 
> 
> Testing
> ---
> 
> Tried konsole, kate and dolphin under Unity 7 on Ubuntu 16.10
> 
> konsole and kate work fine (i.e. the action is gone from the menus and all is 
> good)
> 
> dolphin is not 100% "perfectly behabed" (i.e. the "control" toolbar item is 
> supposed to not be shown when menubars are shown and in this case it's shown) 
> but it's not a regression and imho it's the dolphin code being a bit weird (i 
> can propose a patch for it if this gets accepted)
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 129261: Hide the "Show Menu Bar" action if all the menubars are native

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129261/#review100279
---



+1 pretty cool!

- Aleix Pol Gonzalez


On Oct. 26, 2016, 12:15 a.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129261/
> ---
> 
> (Updated Oct. 26, 2016, 12:15 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kconfigwidgets
> 
> 
> Description
> ---
> 
> Some applications have a "Show Menu Bar" action that is a bit silly on 
> systems where the menubar is part of the shell (for example Unity 7).
> 
> This patch attempts to fix it by iterating all the main windows when they are 
> shown and if all the menubars of all mainwindows are native, then hides the 
> show menu bar action (basically erasing it from existence).
> 
> It's not the nicest of the codes and probably has some edge cases but works 
> on the general case so i think it's worth the effort.
> 
> 
> Diffs
> -
> 
>   src/kstandardaction.cpp 89d011e 
> 
> Diff: https://git.reviewboard.kde.org/r/129261/diff/
> 
> 
> Testing
> ---
> 
> Tried konsole, kate and dolphin under Unity 7 on Ubuntu 16.10
> 
> konsole and kate work fine (i.e. the action is gone from the menus and all is 
> good)
> 
> dolphin is not 100% "perfectly behabed" (i.e. the "control" toolbar item is 
> supposed to not be shown when menubars are shown and in this case it's shown) 
> but it's not a regression and imho it's the dolphin code being a bit weird (i 
> can propose a patch for it if this gets accepted)
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Review Request 129261: Hide the "Show Menu Bar" action if all the menubars are native

[Albert Astals Cid]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129261/
---

Review request for KDE Frameworks.


Repository: kconfigwidgets


Description
---

Some applications have a "Show Menu Bar" action that is a bit silly on systems 
where the menubar is part of the shell (for example Unity 7).

This patch attempts to fix it by iterating all the main windows when they are 
shown and if all the menubars of all mainwindows are native, then hides the 
show menu bar action (basically erasing it from existence).

It's not the nicest of the codes and probably has some edge cases but works on 
the general case so i think it's worth the effort.


Diffs
-

  src/kstandardaction.cpp 89d011e 

Diff: https://git.reviewboard.kde.org/r/129261/diff/


Testing
---

Tried konsole, kate and dolphin under Unity 7 on Ubuntu 16.10

konsole and kate work fine (i.e. the action is gone from the menus and all is 
good)

dolphin is not 100% "perfectly behabed" (i.e. the "control" toolbar item is 
supposed to not be shown when menubars are shown and in this case it's shown) 
but it's not a regression and imho it's the dolphin code being a bit weird (i 
can propose a patch for it if this gets accepted)


Thanks,

Albert Astals Cid