WIP-NotWorking: Try to fix flicker of screen while locked and detaching screens
Needs ReviewPublic

Authored by tcanabrava on Jan 29 2020, 12:00 PM.

Details

Summary

This patch tries - and fails - to fix the flicker of screens while
the kscreenlocker_greeter is active. The reasoning of the patch is
simple:

instead of deleting the overlays in the vector untill we have as
many overlays as screens, we check if a overlay belongs to a screen
that's currently plugged in, deleting only those that don't have
a screen anymore.

and then, instead of going thru all of the views and updating the
geometry and position, we only set that once for each overlay created.

This is not by all means something that I want to merge, as currently
is broken, but given the current implementation is also broken I think
it's safe to assume a 'v1' so we can discuss what's wrong on both
implementations.

To test the bug: plug two monitors, run the locker, detach and reattach
the monitors. on detaching (but not reattaching) the monitor will flicker
and display the contents of what's behind the locked screen.

Diff Detail

Repository
R133 KScreenLocker
Branch
wip_fix_screenlocker_detach_reattach
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22715
Build 22733: arc lint + arc unit
tcanabrava created this revision.Jan 29 2020, 12:00 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 29 2020, 12:00 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
tcanabrava requested review of this revision.Jan 29 2020, 12:00 PM

This is definitely an area for improvement.

This code was initially written before QGuiApplication::screenRemoved existed, which is why it's like this.
I think this solution is overly complex, you have the pointer to what changed in screenRemoved, we then discard that information and then go through loops to effectively retrieve that information we originally had.
With a refactor that could be cleaner.

As for your initial bug you're trying to fix:
You're still in a race to see if kscreenlocker can update before kwin renders something, this might make kscreenlocker faster, but it won't resolve it being a race.

This is definitely an area for improvement.

This code was initially written before QGuiApplication::screenRemoved existed, which is why it's like this.
I think this solution is overly complex, you have the pointer to what changed in screenRemoved, we then discard that information and then go through loops to effectively retrieve that information we originally had.
With a refactor that could be cleaner.

I'll try to refactor.

As for your initial bug you're trying to fix:
You're still in a race to see if kscreenlocker can update before kwin renders something, this might make kscreenlocker faster, but it won't resolve it being a race.

I'll poke vlad to see how to proceed here.

tcanabrava updated this revision to Diff 74565.Jan 29 2020, 1:30 PM
  • Simplify Architecture
zzag added a subscriber: zzag.Jan 29 2020, 2:06 PM

Sorry, I know little to nothing about internals of KScreenLocker.

davidedmundson requested changes to this revision.Feb 17 2020, 8:21 PM

There's no need for this property stuff

We have:

view->setScreen()

(or ideally passed in the constructor, it's a lot faster)

and

view->screen()

and that has everything we need.

greeter/greeterapp.cpp
405

Why are we looping through things again. It's all overly complex.

We just need

void screenAddedImpl(QScreen *)
{
  //everything here
}

then one place in the ctor,

for (screen, qApp->screens()) {
   screenAddedImpl(screen);
}
This revision now requires changes to proceed.Feb 17 2020, 8:21 PM
tcanabrava updated this revision to Diff 76053.Feb 20 2020, 3:06 PM
  • Simplify function / remove desktopResized