D6376: Fix double delete crash during shutdown
rjvbb added a comment. In https://phabricator.kde.org/D6376#119500, @cullmann wrote: > I don't speak about threading races. The whole class is not thread-safe, if threading occurs, all is lost. Really? Only a single thread at a time should trigger audio notifications, is that what you're saying? That would surprise me a little, looking at the code it seems the class exists so that applications can request to play a notification sound by creating an instance and then forget about it. The threading I was thinking of is simply when 2 threads queue an audio notification at the same time, not the cross-thread use of a NotifyByAudio instance. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D6376 To: cullmann, #frameworks Cc: aacid, mpyne, rjvbb
D6376: Fix double delete crash during shutdown
rjvbb added a comment. In https://phabricator.kde.org/D6376#119376, @cullmann wrote: > Next try ;=) > Question is, can really such a race happen, that finishNotification triggers duplicate insertion? Do you see any other explanation why a crash could occur under qDeleteAll, one that is due to a bug in KNotifications and not in Phonon itself? I'd say the source for a duplicate insertion (and thus the location of the race condition) would be a concurrent execution of `m_reusablePhonons.takeFirst()`, where more than 1 thread gets hold of the same item. > At least with a hash set, this won't lead to later issues. Why not use a barrier or lock to ensure that the `takeFirst()` requests are executed one by one? FWIW, a race condition wouldn't only lead to duplicate entries in the list (something I'd hope Qt knows how to handle) but also to more than 1 thread using the same Phonon object at the same time. Who's to tell what state that object will be in? REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D6376 To: cullmann, #frameworks Cc: mpyne, rjvbb
D6376: Fix double delete crash during shutdown
rjvbb added a comment. > Hmm, not really Not really what? REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D6376 To: cullmann, #frameworks Cc: rjvbb
D6376: Fix double delete crash during shutdown
rjvbb added a comment. A priori this should be fine, and it might even address the long standing bug by leaving more time for Phonon objects to "do their thing". It might be an idea though to include a `qCDebug()` probe that outputs the number of items left for auto-cleanup in `m_reusablePhonons`. But what's the official stance on deleting objects (widgets) that have a parent and are thus capable of auto-cleanup? AFAIK one can still delete them explicitly, and if they're reparented during that process they should no longer be included in the cleanup step performed by their original parent's dtor. IOW, if you're right, isn't there a bug to address in `Phonon::MediaObject`? There can also be another reason *in release builds* for double frees (which your change will probably catch, too): `NotifyByAudio::notify()` does Q_ASSERT(!m_notifications.value(m)); m_notifications.insert(m, notification); and `NotifyByAudio::finishNotification()` does m_notifications.remove(m); //... m_reusablePhonons.append(m); Release builds do not check if `m_notifications` already contains the object being added. It seems extremely unlikely that this would ever happen, but apparently the code author thought it could. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D6376 To: cullmann, #frameworks Cc: rjvbb
D5111: Provide demo/preview for checkable menu items
rjvbb retitled this revision from "Provide demo/preview for checkable menu items and colour scheme comparison" to "Provide demo/preview for checkable menu items". rjvbb edited the summary of this revision. rjvbb edited the test plan for this revision. rjvbb set the repository for this revision to R113 Oxygen Theme. REPOSITORY R113 Oxygen Theme REVISION DETAIL https://phabricator.kde.org/D5111 To: rjvbb, hpereiradacosta, jriddell, zhigalin, anthonyfieroni Cc: ltoscano, kde-mac, #frameworks
D5111: Provide demo/preview for checkable menu items and colour scheme comparison
rjvbb updated this revision to Diff 15812. rjvbb added a comment. Updated for 5.10.2+ CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5111?vs=12788&id=15812 REVISION DETAIL https://phabricator.kde.org/D5111 AFFECTED FILES kstyle/demo/main.cpp kstyle/demo/oxygendemodialog.cpp kstyle/demo/oxygendemodialog.h kstyle/demo/oxygenmdidemowidget.cpp To: rjvbb, hpereiradacosta, jriddell, zhigalin, anthonyfieroni Cc: ltoscano, kde-mac, #frameworks
D6309: KIconThemes: some additional details about themes & icons on Mac & MSWin
rjvbb added a comment. Ben Cooksley wrote on 20170623::09:17:33 re: "https://phabricator.kde.org/D6309: KIconThemes: some additional details about themes & icons on Mac & MSWin" > The system has now polled the repository and the hook has correspondingly been triggered. I see. Does that mean you have rolled something yourself and could technically make it trigger also on a more compact key that's more in line with the other keywords (and easier to remember, more resistant to typos)? REPOSITORY R302 KIconThemes REVISION DETAIL https://phabricator.kde.org/D6309 To: rjvbb, #frameworks Cc: bcooksley, tfry, kde-mac, #frameworks
D6309: KIconThemes: some additional details about themes & icons on Mac & MSWin
This revision was automatically updated to reflect the committed changes. Closed by commit R302:ac5cbf6b4aa9: More details about deploying icon themes on Mac & MSWin (authored by rjvbb). REPOSITORY R302 KIconThemes CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6309?vs=15694&id=15775 REVISION DETAIL https://phabricator.kde.org/D6309 AFFECTED FILES README.md To: rjvbb, #frameworks Cc: tfry, kde-mac, #frameworks
D6309: KIconThemes: some additional details about themes & icons on Mac & MSWin
rjvbb added a comment. Well, somehow this wasn't closed automatically: https://commits.kde.org/kiconthemes/ac5cbf6b4aa969542a780077213cb39a4c110fe6 > Arguably, this is not the place where I would have looked for an answer to that question, first, but since it does provide an answer, it should try to answer it good. Evidently, that was also my intention. Which makes me wonder: is it possible to deploy a stylesheet that tweaks a built-in style via an auto-loading plugin, and could such a plugin take care of setting an icon theme path? REPOSITORY R302 KIconThemes REVISION DETAIL https://phabricator.kde.org/D6309 To: rjvbb, #frameworks Cc: tfry, kde-mac, #frameworks
[Differential] [Commented On] D3830: Add a new FindGperf module
rjvbb added a comment. In https://phabricator.kde.org/D3830#72701, @pino wrote: > The full path of gperf is determined using `find_program`, which looks in `$PATH` -- you can always specify the variable with the full path to force a custom location. great, that's exactly what I thought but wanted to see confirmed! :) (your remark of "using the full path for the output location" confused me a bit, sorry about that) REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D3830 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: pino, #frameworks, #buildsystem, #windows, kde-mac Cc: kfunk, rjvbb, adridg
[Differential] [Commented On] D3830: Add a new FindGperf module
rjvbb added a comment. In https://phabricator.kde.org/D3830#72508, @pino wrote: > For the input file? Yes. I did mean the gperf executable...! REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D3830 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: pino, #frameworks, #buildsystem, #windows, kde-mac Cc: kfunk, rjvbb, adridg
[Differential] [Commented On] D3830: Add a new FindGperf module
rjvbb added a comment. You're still determining the location from the path with this new revision, right? REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D3830 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: pino, #frameworks, #buildsystem, #windows, kde-mac Cc: kfunk, rjvbb, adridg
[Differential] [Commented On] D3830: Add a new FindGperf module
rjvbb added a comment. In https://phabricator.kde.org/D3830#71722, @pino wrote: > Windows and Mac people: at least from a quick glance, GNU gperf should be already available on Windows and Mac I can only speak for 10.9 but indeed, gperf is available in /usr/bin . REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D3830 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: pino, #frameworks, #buildsystem, #windows, kde-mac Cc: rjvbb, adridg
