Review Request 129892: Do nothing in ShowMenubarActionFilter::updateAction if there are no menubars

[Anthony Fieroni]

> On Ян. 27, 2017, 7:01 преди обяд, Anthony Fieroni wrote:
> > So, wait a minute i just quote you
> > *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).* from https://git.reviewboard.kde.org/r/129261/
> > *This has nothing to do with whether you are using Unity or Plasma* from https://git.reviewboard.kde.org/r/129885/
> > I want an explanation, i never use Unity, i will never use it, but i want know in Unity - Kate, Dolphin etc. on showEvent allMenuBarsNative == true, wasNative == true ?
> > If so, i'm -1 for this approach
> 
> Albert Astals Cid wrote:
>     You don't seem to understand the code, what makes you think you're able to give a -1 when you don't understand the code?
>     
>     You want an explanation? What makes you think you should be given one? You're not the maintainer of the code, you have not contributed at all to kconfigwidgets and on top of that you've been nothing other than agressive both in this and in the other review request.
>     
>     Still i'll try to explain it to you, but don't think your attitude is helping, it's not.
>     
>     > but i want know in Unity - Kate, Dolphin etc. on showEvent allMenuBarsNative == true, wasNative == true ?
>     
>     This depends if it's the first show or not. On the first show obviously wasNative will be false since it is how it is initialized in the constructor. So on the first show it'll be allMenuBarsNative == true, wasNative == false and on the second show, yes it will be allMenuBarsNative == true, wasNative == true.
>     
>     I sincerely think the code is quite easy to understand, what you don't understand?
> 
> Anthony Fieroni wrote:
>     So when i know allMenuBarsNative == true, wasNative == true, why you think that i'm not uderstainding? I'm against to store / restore menubar chacked and visabiliity.
> 
> Anthony Fieroni wrote:
>     >This depends if it's the first show or not. On the first show obviously wasNative will be false since it is how it is initialized in the constructor. So on the first show it'll be allMenuBarsNative == true, wasNative == false and on the second show, yes it will be allMenuBarsNative == true, wasNative == true.
>     
>     Albert, i know that, that's why i made that patch https://git.reviewboard.kde.org/r/129885/
> 
> Albert Astals Cid wrote:
>     Do you know what a native menubar is? Can you explain why you are against storing the state?

I read this http://doc.qt.io/qt-5/qmenubar.html#nativeMenuBar-prop, personally i never use nor OSX nor Unity. I'm against because this is a bad practive, yeah i see new patch will work (logically, because i'm not test it) but i don't see any reason to do this when we can do it without. Now we insert new code rather than minimize it, my approach when i write code are:
1. simple logic
2. less code
3. less temporary state changes
4. less variables
For me new code doesn't meet these conditions, i'm not a person who decide who will be pushed or not.


- Anthony


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


On Ян. 27, 2017, 12:19 преди обяд, Albert Astals Cid wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129892/
> -----------------------------------------------------------
> 
> (Updated Ян. 27, 2017, 12:19 преди обяд)
> 
> 
> Review request for KDE Frameworks, Anthony Fieroni and David Faure.
> 
> 
> Repository: kconfigwidgets
> 
> 
> Description
> -------
> 
> It makes no sense to treat no menubars as all menubars native so just bail early, the app is probably still in construction if there's no menubars but it actually has a showmenubaraction.
> 
> 
> Diffs
> -----
> 
>   src/kstandardaction.cpp 6e5bba7 
> 
> Diff: https://git.reviewboard.kde.org/r/129892/diff/
> 
> 
> Testing
> -------
> 
> Dolphin behaves better regarding the show menubar action.
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170128/8e1eb6a9/attachment.html>