https://bugs.kde.org/show_bug.cgi?id=446581
--- Comment #8 from nyanpasu64 <nyanpas...@tuta.io> --- ## Popping up menus upon copying URLs At https://invent.kde.org/plasma/plasma-workspace/-/blob/master/klipper/klipper.cpp#L825-833, you call URLGrabber::checkNewData() (which pops up a menu if Klipper's Action Menu is enabled and you're not copying from Firefox), *then* skip assigning lastURLGrabberText if the text matches. I believe the intent was to skip calling URLGrabber::checkNewData() if the text matches, but that would mean copying the same text twice doesn't popup a menu the second time. Should this be changed to the design intent (which changes clipboard behavior), or should we remove m_lastURLGrabberTextSelection and m_lastURLGrabberTextClipboard altogether? ## Fixing the bug through asynchronous work On the topic of this bug, QMimeData::text() is blocking and there's no continuation-based alternative (even though the underlying xcb is asynchronous). - The hard solution is to get text through raw xcb (and I'm not familiar with X so I can't do this without a lot of new learning). - The easiest solution I can think of, which minimizes learning unknown APIs and code changes, is to move fetching clipboard contents (which can block) to either a new thread or a job in QThreadPool::globalInstance(). - This job will call HistoryItem::create(clipData), and sends the data back to a new Klipper::onClipData(HistoryItemPtr) method called in the main thread, which inserts the resulting HistoryItemPtr into history (hopefully in the same order as you copied text). - How do we send a HistoryItemPtr back to Klipper on the main thread? - Add our own message queue which workers push onto, and having the Qt event loop poll it and invoke Klipper (probably difficult to achieve). - The worker job holds a QPointer<Klipper> and sends a custom QEvent subclass there. Overload QObject::event() in Klipper to dispatch on our new event ID. (Creates a use-after-free if the main thread deletes Klipper after it checks it's alive but before it sends a message.) - The worker job holds a QPointer<Klipper> and calls QMetaObject::invokeMethod(Klipper*, &Klipper::onClipData, HistoryItemPtr) (which internally sends a QMetaCallEvent, but Qt handles invoking the right method for us). (Creates a use-after-free if the main thread deletes Klipper after it checks it's alive but before it sends a message.) - Give the worker job a signal clipDataReady(HistoryItemPtr), and connect it to Klipper::onClipData(HistoryItemPtr) with a QueuedConnection. (I don't like dynamically adding connections, but it's immune to use-after-free I hope. But QRunnable can't have signals unless you inherit from QObject *and* QRunnable! https://forum.qt.io/post/357008) - Let the worker job update a QFutureInterface (stable but undocumented internal Qt type, similar to the documented QPromise in Qt 6), wrap a QFutureWatcher around the QFuture, and connect to Klipper::onClipData(HistoryItemPtr). At this point it's so complicated I'd rather inherit from QObject and QRunnable. - Should we wait for all worker threads to finish before destroying Klipper? Probably doesn't matter, it's fine to destroy the signal connections with Klipper, and let the background threads work until the process exits. (Does QApplication::exec() wait for all QThreadPool tasks to finish? I don't know. I sure hope that we don't get stuck background threads preventing plasmashell termination.) The least bad solution I've come up with is a worker object inheriting from QObject and QRunnable, emitting a clipDataReady signal before exiting. Is there a better solution? I hope so. ## Reentrancy Also we will need to make sure that splitting functionality across asynchronous callbacks does not interact incorrectly with Klipper::m_locklevel (apparently designed to prevent reentrant calls?). I'm not sure all the ways Klipper::m_locklevel is manipulated, but the check in Klipper::applyClipChanges() (https://invent.kde.org/plasma/plasma-workspace/-/blob/master/klipper/klipper.cpp#L657) seems to be completely redundant. Klipper::newClipData() (https://invent.kde.org/plasma/plasma-workspace/-/blob/master/klipper/klipper.cpp#L681) returns early if m_locklevel is nonzero, before calling -> Klipper::checkClipData -> Klipper::applyClipChanges, with no increments to m_locklevel along the way. The latter two methods have no other callers or references, but are bizarrely protected methods despite Klipper having no subclasses in all of plasma-workspace (and I hope it has no external subclasses in other Qt projects, https://lxr.kde.org/search?%21v=kf5-qt5&_filestring=&_string=Klipper&_casesensitive=1 didn't return anything). Because there is no other path to call Klipper::applyClipChanges() except through Klipper::newClipData() (which returns if m_locklevel != 0), then m_locklevel must be 0 in Klipper::applyClipChanges(). But I'd make this into an assertion to verify my reasoning, rather than remove this check entirely. So in any case I don't *think* Klipper::m_locklevel matters. -- You are receiving this mail because: You are watching all bug changes.
