updateXTime now forces a property change and we use Application::updateX11Time to then catch the time of the corresponding response event.
Details
Details
- Reviewers
davidedmundson zzag - Group Reviewers
KWin - Maniphest Tasks
- T4424: [kwin] Support updateXTime() on Xwayland
Works in a X session (Window resize and video works).
Works with XWayland windows.
Diff Detail
Diff Detail
- Repository
- R108 KWin
- Branch
- arcpatch-D28674
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 25231 Build 25249: arc lint + arc unit
| CMakeLists.txt | ||
|---|---|---|
| 513 | Rename the class to SyncAlarmNotifyX11Filter or something. | |
| main.cpp | ||
| 376–378 | It would be nice to have a comment that explains what is going on here. | |
| platform.cpp | ||
| 54–60 | Move/resize operations is something that Workspace needs to be concerned about. Please create the filter in Workspace::initWithX11(). | |
| 508–515 | While you're on this, could you please re-format this comment? /** * Heart touching doxygen comment. */ void Platform::updateXTime() { } | |
Comment Actions
Rename SyncFilter to SyncAlarmNotifyX11Filter, move its instance to Workspace, improve comments
| CMakeLists.txt | ||
|---|---|---|
| 513 | Please keep filenames in alphabetical order. Tip: open vim, select relevant lines in the visual mode, and run the sort command. | |
| platform.cpp | ||
| 38 ↗ | (On Diff #80095) | This include must come after #include "abstract_output.h" |
| platform.h | ||
| 44 ↗ | (On Diff #80095) | Unrelated change, please remove it. |
| workspace.cpp | ||
| 453–455 ↗ | (On Diff #80095) | This if must be in initWithX11. |
| workspace.h | ||
| 674 ↗ | (On Diff #80095) | I know that QScopedPointer sucks, but I suggest to use it anyway for the consistency sake. |
| workspace.cpp | ||
|---|---|---|
| 451–457 ↗ | (On Diff #80095) | You don't need a connect and m_syncFilter is shadowed. if (Xcb::Extensions::self()->isSyncAvailable()) { m_syncFilter.reset(new SyncAlarmNotifyX11Filter); } |
Comment Actions
While this works ok (resizing is smoother for X windows than before), there is more flickering with XWayland than with X.
I tried a few things but I am not sure where or why we drop frames, I am not familiar with X synchronization.
Any advice ?
Comment Actions
You see flickering because we update the window pixmap in X11Client::handleSync(). I need some time to think about how to fix it.
Comment Actions
Also currently my new implementation of updateXTime() has become asynchronous : it does not set immediately to the current timestamp, but will set it forcefully soon rather, when Application::updateX11Time gets called again.
Thanks for the pointer.
Comment Actions
resizing is smoother for X windows than before
@zzag is it worth merging this now and building upon it?
Comment Actions
Yeah... On Wayland, we cannot update kwin's timestamp as Qt's XCB plugin does, bad things may happen. On the other hand, we might face some problems if updateXTime() is updated asynchronously on X11, e.g. kwin refuses to activate some windows. We need to be very careful with such things.
| x11client.cpp | ||
|---|---|---|
| 2912 ↗ | (On Diff #80095) | Is it necessary to call this here ? |
| x11client.cpp | ||
|---|---|---|
| 2912 ↗ | (On Diff #80095) | Yes, because user can resize a window more than once during a vblank interval and it's not guaranteed that there won't be any pending sync requests before the next compositing cycle starts. |
Comment Actions
After some thinking, it's probably better not to merge this patch because updating current X11 time stamp asynchronously may break a lot of things on X11. On the other hand, it's highly undesired to make roundtrips to Xwayland.
I suggest to split this task into two sub-tasks:
(a) Implement updateXTime() on Wayland. After digging through Xwayland's internals, it appears like we could use the system monotonic time as Xwayland's current X11 time stamp. For what it's worth, Mutter does the same thing.
(b) Enable synchronized resizing for Xwayland clients. This will be a more subtle problem since Xwayland attaches a null buffer after the frame window has been resized. I don't know how to fix it yet.
I also would like to apologize for spotting issues with the patch too late in the code review process.