From kwin Wed Aug 10 13:30:19 2016 From: =?utf-8?q?Thomas_L=C3=BCbking?= Date: Wed, 10 Aug 2016 13:30:19 +0000 To: kwin Subject: Re: Review Request 128636: [libkwin] Stop raising the desktop Message-Id: <20160810133019.9365.99672 () mimi ! kde ! org> X-MARC-Message: https://marc.info/?l=kwin&m=147083583929287 --===============2792479945744725742== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit > On Aug. 10, 2016, 9:45 vorm., Martin Gräßlin wrote: > > KWin is the wrong place to fix this bug. It needs to be fixed in plasmashell as Thomas already said. > > Anthony Fieroni wrote: > Martin do you want this patch as you TODO does? > // TODO: does it make sense to cache the returned window type for SUPPORTED_MANAGED_WINDOW_TYPES_MASK? > > Martin Gräßlin wrote: > looking at the code: this method is fast - also the invoked one in NETWinInfo I know to be fast. Given that the answer to the question I asked myself in that TODO is: "no, it doesn't make sense to cache the returned window type". And the question was only concerning performance as it got added in an optimization change. > > Overall I don't like the patch due to the usage of mutuable. That just means that something is wrong there. > > Anthony Fieroni wrote: > It's standart to me, i can correct it if you want or to discard it. > It's standard to me Should not be. Setting a member mutable allows to alter it even if accessing the class "const", eg. from const member functions. It does _not_ fit regular data members. - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128636/#review98274 ----------------------------------------------------------- On Aug. 10, 2016, 3:41 vorm., Anthony Fieroni wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128636/ > ----------------------------------------------------------- > > (Updated Aug. 10, 2016, 3:41 vorm.) > > > Review request for kwin, David Edmundson and Martin Gräßlin. > > > Bugs: 350826 and 365014 > https://bugs.kde.org/show_bug.cgi?id=350826 > https://bugs.kde.org/show_bug.cgi?id=365014 > > > Repository: kwin > > > Description > ------- > > Desktop must not be raised, this cannot be a proper patch, but it's needed more investigate to track this down. > > > Diffs > ----- > > client.h 08e25ab > client.cpp 2e69666 > unmanaged.h 31aea04 > unmanaged.cpp 1a76175 > > Diff: https://git.reviewboard.kde.org/r/128636/diff/ > > > Testing > ------- > > 1. open 1 or more non maximized windows > 2. make left clicks periodically between window and the desktop > 3. desktop is raised over opened windows *sometimes* > > > Thanks, > > Anthony Fieroni > > --===============2792479945744725742== MIME-Version: 1.0 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128636/

On August 10th, 2016, 9:45 vorm. UTC, Martin Gräßlin wrote:

KWin is the wrong place to fix this bug. It needs to be fixed in plasmashell as Thomas already said.

On August 10th, 2016, 11:23 vorm. UTC, Anthony Fieroni wrote:

Martin do you want this patch as you TODO does?
// TODO: does it make sense to cache the returned window type for SUPPORTED_MANAGED_WINDOW_TYPES_MASK?

On August 10th, 2016, 12:33 nachm. UTC, Martin Gräßlin wrote:

looking at the code: this method is fast - also the invoked one in NETWinInfo I know to be fast. Given that the answer to the question I asked myself in that TODO is: "no, it doesn't make sense to cache the returned window type". And the question was only concerning performance as it got added in an optimization change.


Overall I don't like the patch due to the usage of mutuable. That just means that something is wrong there.

On August 10th, 2016, 1:17 nachm. UTC, Anthony Fieroni wrote:

It's standart to me, i can correct it if you want or to discard it.


It's standard to me




Should not be.
Setting a member mutable allows to alter it even if accessing the class "const", eg. from const member functions.
It does not fit regular data members.

- Thomas


On August 10th, 2016, 3:41 vorm. UTC, Anthony Fieroni wrote:

Review request for kwin, David Edmundson and Martin Gräßlin.
By Anthony Fieroni.

Updated Aug. 10, 2016, 3:41 vorm.

Bugs: 350826, 365014
Repository: kwin

Description 

Desktop must not be raised, this cannot be a proper patch, but it's needed more investigate to track this down.

Testing 

    

  1. open 1 or more non maximized windows
    2. 

  2. make left clicks periodically between window and the desktop
    3. 

  3. desktop is raised over opened windows sometimes
    4.

Diffs

  • client.h (08e25ab)
  • client.cpp (2e69666)
  • unmanaged.h (31aea04)
  • unmanaged.cpp (1a76175)

View Diff
--===============2792479945744725742==--