telegramdesktop / tdesktop

Telegram Desktop messaging app
https://desktop.telegram.org/
Other
26.19k stars 5.19k forks source link

Interacting with anything in the app makes KDE's loading animation play #24711

Closed thatAkiraFox closed 2 years ago

thatAkiraFox commented 2 years ago

Steps to reproduce

  1. Open Telegram v4.0 to 4.0.2
  2. Click on anything
  3. Watch the loading animation play

Expected behaviour

The loading animation shouldn't play, like before v4.0

Actual behaviour

https://user-images.githubusercontent.com/49398464/175771737-da075442-5ce5-45bf-99b4-0be3071cbbbd.mp4

Operating system

Arch Linux x86

Version of Telegram Desktop

From 4.0 to 4.0.2

Installation source

Static binary from official website

Logs

No response

MaxFleur commented 2 years ago

I can also reproduce this.

OS: Arch Linux Telegram Desktop: 4.0.2 KDE-Plasma: 5.25.1 KDE-Frameworks: 5.95.0 Qt: 5.15.5 Display Protocol: Wayland

lankylonky22 commented 2 years ago

note that the animation stops after 5s, as per the setting, u can disable it via this option. image so the system thinks every click is an app launching, normally when the app is launched successfully the animation stops, the longest time the system allows the animation to exist is 5s, sth got rekt in telegram src code.

thatAkiraFox commented 2 years ago

Yup I know and I've disabled that animation for now, but we shouldn't be forced to disable an useful animation just because of Telegram

lankylonky22 commented 2 years ago

i can also reproduce this:

Operating System: Arch Linux KDE Plasma Version: 5.25.1 KDE Frameworks Version: 5.95.0 Qt Version: 5.15.5 Kernel Version: 5.18.6-arch1-1 (64-bit) Graphics Platform: Wayland Processors: 8 × Intel® Core™ i5-9300H CPU @ 2.40GHz Memory: 15.5 GiB of RAM Graphics Processor: NVIDIA GeForce RTX 2060/PCIe/SSE2 Manufacturer: HP Product Name: OMEN by HP Laptop 15-dc1xxx

lankylonky22 commented 2 years ago

Yup I know and I've disabled that animation for now, but we shouldn't be forced to disable an useful animation just because of Telegram

ikr and telegram doesnt maximize at the first time i click the max button, maybe its due to wayland and nvidia proprietary driver, but i cant reproduce this issue with other winodws, so thats also an annoying glitch

MaxFleur commented 2 years ago

I did some more testing and the problem seems to appear only for Wayland. If I use Plasma with X11, the error does not appear. @joseph-20 Do you use Wayland? If so, could you try it also with X11?

MaxFleur commented 2 years ago

ikr and telegram doesnt maximize at the first time i click the max button, maybe its due to wayland and nvidia proprietary driver, but i cant reproduce this issue with other winodws, so thats also an annoying glitch

Nvidia should not be the problem, afaik, because I have the same problems with an AMD GPU.

thatAkiraFox commented 2 years ago

I did some more testing and the problem seems to appear only for Wayland. If I use Plasma with X11, the error does not appear. @joseph-20 Do you use Wayland? If so, could you try it also with X11?

@MaxFleur yeah setting env QT_QPA_PLATFORM=xcb in the .desktop file (and so forcing Qt in X mode) works around the issue, just like it "fixed" the "Use system window frame" option before it was officially fixed in 4.0.1, but 1. it's not a fix, just a workaround and 2. it always worked fine on Wayland, so the issue came up with 4.0 and needs to be fixed.

ilya-fedin commented 2 years ago

Well, Telegram got activation support on Wayland. As far as I understand, this is a KDE bug as this animation should play only on application start, not on every focus request.

thatAkiraFox commented 2 years ago

...this is a KDE bug as this animation should play only on application start, not on every focus request.

We're not talking about focus request here since the window always has focus in my video.

Also, it happens only with Telegram, so it's not a KDE issue.

ilya-fedin commented 2 years ago

We're not talking about focus request here since the window always has focus in my video.

Well, Telegram issues focus requests even when it has focus. It has a lot of QWidget::activateWindow calls through the codebase and abstractions.

Also, it happens only with Telegram, so it's not a KDE issue.

Well, that's a broken logic, most applications still have no activation support on Wayland, Telegram one of a few (if other exist at all yet).

thatAkiraFox commented 2 years ago

Well, that's a broken logic, most application still have no activation support on Wayland, Telegram one of a few (if other exist at all yet).

Ok I misunderstood there. What is the "activation support" you're talking about here?

ilya-fedin commented 2 years ago

What is the "activation support" you're talking about here?

bringing the window to the front

ilya-fedin commented 2 years ago

activation/focus are synonyms here

lankylonky22 commented 2 years ago

What is the "activation support" you're talking about here?

bringing the window to the front

why does telegram issue so many QWidget::activateWindow calls

ilya-fedin commented 2 years ago

why does telegram issue so many QWidget::activateWindow calls

IDK, probably to be sure it's in the front. The codebase is around 10 years old.

KotoWhiskas commented 2 years ago

We're not talking about focus request here since the window always has focus in my video.

Well, Telegram issues focus requests even when it has focus. It has a lot of QWidget::activateWindow calls through the codebase and abstractions.

Also, it happens only with Telegram, so it's not a KDE issue.

Well, that's a broken logic, most applications still have no activation support on Wayland, Telegram one of a few (if other exist at all yet).

I'm not sure about this. I wrote a simple test qt python program:

from PyQt6.QtWidgets import QApplication, QWidget
from PyQt6.QtCore import QTimer

app = QApplication([])
widget = QWidget()
widget.show()

timer = QTimer()
timer.timeout.connect(widget.activateWindow)
timer.start(1000)

app.exec()

And neither pyqt5 nor pyqt6 have this problem

ilya-fedin commented 2 years ago

And neither pyqt5 nor pyqt6 have this problem

  1. Both Qt 5 and Qt 6 don't support window activation on Wayland yet. Telegram has custom code for that to work.
  2. Your application needs an already activated window to move focus to another window on Wayland.
KotoWhiskas commented 2 years ago

And neither pyqt5 nor pyqt6 have this problem

1. Both Qt 5 and Qt 6 don't support window activation on Wayland yet

Qt6 does, actually. See https://codereview.qt-project.org/c/qt/qtwayland/+/321246

I also tried this in pyqt6:


from PyQt6.QtWidgets import QApplication, QWidget
from PyQt6.QtCore import QTimer, Qt

app = QApplication([])
widget = QWidget()
widget.show()

# Makes app icon in taskbar orange
timer = QTimer()
timer.timeout.connect(widget.activateWindow)
timer.timeout.connect(lambda: print("window activated"))
timer.start(1000)

# Does nothing
timer2 = QTimer()
timer2.timeout.connect(widget.raise_)
timer2.timeout.connect(lambda: print("widget raised"))
timer2.start(5000)

# Makes app icon in taskbar orange
timer3 = QTimer()
timer3.timeout.connect(lambda: widget.setWindowState(Qt.WindowState.WindowActive))
timer3.timeout.connect(lambda: print("active state set in widget"))
timer3.start(10000)

app.exec()

While in qt5 it says "qt.qpa.wayland: Wayland does not support QWindow::requestActivate() window activated", qt6 works, but instead of setting app in focus it just highlights the app in taskbar, but I think this is just kwin's implementation and decision to not allow apps to steal focus.

Telegram has custom code for that to work.

Didn't you say that telegram uses QWidget::activateWindow? Also, if it's custom telegram produces the bug and not default Qt API, I think that's telegram bug,

ilya-fedin commented 2 years ago

Qt6 does, actually. See https://codereview.qt-project.org/c/qt/qtwayland/+/321246

They added implementation, but it's not connected to the public APIs, so it's useless :(

Didn't you say that telegram uses QWidget::activateWindow?

Yeah, it does. Telegram has its own fork of Qt's xdg-shell plugin with actually working xdg-activation implementation, so it works without any changes in the rest of the code.

Also, if it's custom telegram produces the bug and not default Qt API, I think that's telegram bug

Well, no one said it's a Qt bug, so I don't understand this logic.

KotoWhiskas commented 2 years ago

They added implementation, but it's not connected to the public APIs, so it's useless :(

Hmm, I think default QWidget::activateWindow uses this implementation? Because I definitely see the difference between running this function in qt5 and in qt6. In qt6, when running in plasma wayland, it doesn't focus window but highlights the app icon. Again I think it's kwin not allowing it to steal focus although it could.

Yeah, it does. Telegram has its own fork of Qt's xdg-shell plugin with actually working xdg-activation implementation, so it works without any changes in the rest of the code. Well, no one said it's a Qt bug, so I don't understand this logic.

Couldn't that be a broken xdg-activation implementation in telegram and not plasma issue?

ilya-fedin commented 2 years ago

Again I think it's kwin not allowing it to steal focus although it could.

The protocol states a token should be passed and Qt's implementation doesn't set it, they created internal APIs for that, but they don't use them in any way. They just exist without any way for application developer to use them. So I think we can state this implementation is useless.

Couldn't that be a broken xdg-activation implementation in telegram and not plasma issue?

I don't imagine how it could be broken, there's no hint in the protocol to display bouncing icon, this is some bug in Plasma that displays application launching animation on every focus request on Wayland for some reason...

KotoWhiskas commented 2 years ago

they created internal APIs for that, but they don't use them in any way

But they do? I'm not 100% sure but looks like QWidget::activateWindow uses QWaylandWindow::requestActivateWindow because on qt5 in on wayland printed "Wayland doesn't not support..." when calling QWidget::activateWindow while in qt6 it prints nothing. Also, in this merge request I can see token mentions

Screenshot_20220629_051111

ilya-fedin commented 2 years ago

QWaylandWindow::requestActivateWindow

This is not enough. They should also use QWaylandWindow::requestXdgActivationToken, QWaylandWindow::setXdgActivationToken, QWaylandWindow::xdgActivationTokenCreated or the underlying shell integration APIs they're calling (I implemented with the underlying ones). xdg-activation without token handling is useless.

ilya-fedin commented 2 years ago

Also, in this merge request I can see token mentions

As I said, they added internal APIs, but they don't use them and don't forward to public APIs.

KotoWhiskas commented 2 years ago

QWaylandWindow::requestActivateWindow

This is not enough. They should also use QWaylandWindow::requestXdgActivationToken, QWaylandWindow::setXdgActivationToken, QWaylandWindow::xdgActivationTokenCreated or the underlying shell integration APIs they're calling (I implemented with the underlying ones). xdg-activation without token handling is useless.

Ah, I see. In QWaylandWindow::requestActivateWindow() they use QWaylandShellSurface::requestActivate() which just returns false 🤷‍♂

ilya-fedin commented 2 years ago

they use QWaylandShellSurface::requestActivate() which just returns false

It's the base class. Actual calls will be to one of QWaylandShellSurface implementations that are pluginable. On desktop, that's QWaylandXdgSurface usually. And that's what tdesktop replaces with its own fork.

KotoWhiskas commented 2 years ago

Btw, looks like your own xdg-activate implementation doesn't work either: when I open telegram, focus another window and then click tray icon - telegram doesn't steal focus, instead, it just closes. It's working as expected on xorg tho

ilya-fedin commented 2 years ago

I don't see a way to make it work with tray. Its implementation is completely in Qt, so we can't modify it. And tray protocol specification doesn't seem to have a way to get activation token.

hiornso commented 2 years ago

why does telegram issue so many QWidget::activateWindow calls

IDK, probably to be sure it's in the front. The codebase is around 10 years old.

Would it be possible to fix this? It would make this bug go away, but also there are other issues with it. For instance, on my system (also KDE Wayland) if I minimise an app in front of Telegam such that Telegram receives focus, it immediately grabs focus again and so cuts the minimisation animation (of the other app) short, making the transition seem rather jarring.

I wouldn't consider this to be a KDE bug, since switching focus to a window which is not minimised is intentionally instant, but in this instance Telegram is "not minimised" but the animation is still playing, so this switching focus breaks the animation.

ilya-fedin commented 2 years ago

Would it be possible to fix this?

I don't think so... This doesn't create problems on other platforms and Wayland userbase is really small for a refactoring that would mean rewriting good part of app here and there. Even entire Linux userbase would be too small for that probably.

KotoWhiskas commented 2 years ago

Looks like it was fixed in recent plasma update.

Btw, @ilya-fedin, will your qt merge request make telegram custom xdg token stuff implementation unnecessary?

thatAkiraFox commented 2 years ago

Confirm, fixed in Plasma 5.25.3.

lankylonky22 commented 2 years ago

Confirm, fixed in Plasma 5.25.3.

noice

ilya-fedin commented 2 years ago

Btw, @ilya-fedin, will your qt merge request make telegram custom xdg token stuff implementation unnecessary?

My stuff will be just equal to Qt's one in this regard. The xdg-shell fork is still required as it has other changes as e.g. support for custom decorations shadows.