In D23105#515574, @nicolasfella wrote:

I always had tearing when watching fullscreen videos on my Nvidia setup, but with this patch I haven't noticed any tearing so far!

• Perform compositing always from event loop
• Remove unneeded sync, retrace block plumbing
• Remove swap profiler code

I tested it now with Nvidia proprietary driver (v430, Neon unstable, GTX 950) and as expected the fallback timer is used since OML sync control and intel swap event are not available. This means there is no synchronization / delay minimization through delay timer in relation to the vblanks. There is of course the fallback timer providing a gap of ~16ms and the result looks still ok for me. Did not notice any tearing or freezes.

• No need for setting __GL_MaxFramesAllowed

Rebase on master.

• Simplify fallback timer

@alexeymin Can you test again with the update? I just accidentally had the fallback timer disabled before. Now works fine for me without swap event.

• Rebase on master
• Use fallback timer without swap event, move present

In D23105#513550, @alexeymin wrote:

With KWIN_USE_INTEL_SWAP_EVENT=1 all seems to work fine.

But with KWIN_USE_INTEL_SWAP_EVENT=0 or when unset, I see serious "lags" when dragging the window, it becomes very hard to drag window on screen, looks like some mouse movements are dropped(?) Resizing for example is still fine:

In D23105#514201, @fredrik wrote:

In D23105#512099, @romangg wrote:

In D23105#511957, @fredrik wrote:

NVIDIA doesn't support the OML extensions. They can't be implemented efficiently on their hardware IIRC.
[...]

That's good to know. Thanks! I believe we can let in some of these extensions again without increasing the complexity too much as long as the SGI ones is ignored and we have no manual control of vsync. The complexity in the old code came mostly from that.

That would leave the user without a way to override the driver's default vsync setting.

This means only change it in lines without [..].

In D23105#511957, @fredrik wrote:

NVIDIA doesn't support the OML extensions. They can't be implemented efficiently on their hardware IIRC.
[...]

In D23105#511884, @nicolasfella wrote:

What I really need additionally is testing it on an Nvidia setup.

I've briefly tested it on my NVidia GTX 550Ti with nvidia driver. Nothing appears to be horribly broken. Anything in particular I should look out for?

Some observations:

• Compositor swap events without timer
• Paint delay timer
• For analysis add ftrace marker from D23114

If at all this will be embedded into a push on wider scope in all of KDE or at least Plasma and Frameworks, see https://mail.kde.org/pipermail/plasma-devel/2019-August/101494.html.

Rebase on master

In D23105#510721, @alexeymin wrote:

I haven't yet tested it on AMD

I could test how it works on amdgpu (RX560), if you need help with that, when it's ready to test

Maybe add a comment why the y value needs to be adjusted in this one special case.

Your comments explain the new control flow just fine and why it is equivalent to the old one. You can add the const on push, no need to update the diff on Phab before.

• Restore some functions old positions to limit the diff size

• Remove unused variables

Thanks for the vid. Also I feel betrayed now by my system.

When you're at it could you take a look at why hardware keys are not working above 100%? Shouldn't be handled with a separate patch though.

Did you test the menu position in particular in edge cases?

Great, thanks. Please make sure arc won't fail on this commit. As a tip: when I'm unsure about it I use arc land with the --holdoption. This way I can inspect afterwards if the commit message and everything is alright before pushing normally with git the resulting commit to the remote.

Are you doing this deliberately? I said on tear down it makes sense to not process the calls to updateCompositeBlocking...

Replacing = 0 with {} in the header file is out of scope?

It makes sense on tear down to not process any updateCompositeBlocking calls any more since compositing won't be enabled any more.

In D23098#510410, @zzag wrote:

In D23098#510409, @romangg wrote:

Thanks, makes sense. Then just add a default implementation in base class doing nothing? Or will it then still try to access the overriding subclass method?

It doesn't matter whether default implementation is provided. All updateCompositingBlocked() method calls will be dispatched to Compositor, not X11Compositor or WaylandCompositor.

In D23098#510408, @zzag wrote:

In D23098#510403, @romangg wrote:

How about just telling me?

Any call made to a virtual method in constructor/destructor of the base class won't go to a derived class because that method may access either uninitialized or destroyed resources.

How about just telling me?

Why does it work with the other destructor then? Destructors are called one after the other, right?

In D23067#510334, @gladhorn wrote:

I hope this is now making the code slightly easier to read, without changing the functionallity, except for shuffling things around a tiny bit where we discussed. It also adds a few extra checks, to play safe with the smart pointers. These nullptr checks are extremely cheap.

In D23086#510331, @gladhorn wrote:

...
I though these were Qt touch points, interesting. In any case, I hope this is more complete than just casting in 16e904592ae4fe0a04ef61d42c9a114c62997c8e.
What is the best way to test these? I'll run my desktop with the change. I don't use the touch screen that often though.

In D23086#510314, @zzag wrote:

Someone "fixed" these warnings a while ago, but you reverted the fix.

I know. I reverted because it broke functionality. So there was no fix.

This makes sense because libinput provides (non-negative) int32 ids/slots.

To be honest this discussion about the necessity/preference of capitalizing a single letter doesn't lead to much and I don't want to continue it any more. I don't agree with the provided arguments and see no reason to deviate from the recommendation of Angular project and FWIW https://www.conventionalcommits.org. Either we take the current revision as it is or not at all.

Wow, didn't thought it would be that much abandoned code. I'm definitely now for removing all of it. Even if we find the time to bring tabbed windows back at some point in the future it will probably be easier to just start again from scratch. And with this commit we can then still consult the old code for inspiration.

In D22973#509896, @zzag wrote:

In D22973#509887, @romangg wrote:

I don't really follow this argument

The prefix is not part of a proper sentence. It has a bit special purpose, we just use a colon to separate it from the rest of subject line. Therefore, we can follow our own rules. Colon is just a stylistic choice I guess.

In D22973#509840, @zzag wrote:

The only comment I have is about capitalization. It's better to capitalize <subject> as the prefix serves a bit special purpose.

• Remove section about breaking changes

That's good. I hate this geom member variable without m_.

@yurchor: Thanks for the feedback. Corrected now most of them.

• Typos

Ok, I believe I got it now. The QWeakPointer constructer should get either passed a QSharedPointer or QWeakPointer as argument. But here we just pass the QObject EffectWindowImpl. Thanks for explaining.

In D23069#509733, @gladhorn wrote:

I just removed the obviously dead bits behind an if (false). If someone wants to bring it back, it's always in the git history. I don't care either way, but to me reducing lines of code is generally a good thing.

In D23070#509761, @gladhorn wrote:

See https://phabricator.kde.org/D23065

Using a QWeakPointer for QObjects works, but has been deprecated for around seven years. QPointer does the same cleanly (track life-time of QObjects). If you use .toStrongRef() on a QWeakPointer to a QObject without going through QSharedPointer you are in UB land.
In other words: this API is bad and will most likely be removed from Qt in the future.

Sorry, but although you said that the change would be straight-forward can you explain it to me some more. Weak pointers are normally used in conjunction with shared ones. Is there a shared one somewhere?

Sorry, was just to write a commet on the other patch about the names of the commits and missing (?) part 1. See https://phabricator.kde.org/D23070#509746

Where is part 1? Please don't name the commits "<many words> part x", "<many words> part y". Name explicitly each of them what the commit does. This one could be for example: "Remove thumbnails QWeakPointers".

This is about window tabs? There was a patch preparing enabling them again at the beginning of the year by @graesslin. Don't know what happened afterwards. There is also m_switchToTabMenu. Is this not also about the same functionality?