D5872: pidChanged also signals dataChanged in WindowModel
davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.
> it can still be set after being initially 0. In fact, it has to be.
Not at the time the client sees it.
see initialStateCallback which is important.
void PlasmaWindow::Private::initialStateCallback(void *data,
org_kde_plasma_window *window)
{
Q_UNUSED(window)
Private *p = cast(data);
if (!p->unmapped) {
emit p->wm->windowCreated(p->q);
}
}
We don't emit that we have a new window until we get this event. This can be
long after construction.
As long as the server sets the pid before this there's no need for clients to
have an unknown state, that is important and worth testing.
Whether there's a changed signal or not isn't something I particularly care
about, but it isn't needed.
Ping me on IRC if I haven't explained that well.
REPOSITORY
R127 KWayland
REVISION DETAIL
https://phabricator.kde.org/D5872
To: sebas, #plasma, hein, davidedmundson
Cc: bshah, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr,
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas,
apol, hein, lukas
D5867: Add or improve "Generated. Don't edit" messages and make consistent
apol accepted this revision. This revision is now accepted and ready to land. REPOSITORY R240 Extra CMake Modules BRANCH improvegeneratednote REVISION DETAIL https://phabricator.kde.org/D5867 To: kossebau, #frameworks, #build_system, apol
D5872: pidChanged also signals dataChanged in WindowModel
sebas added a comment. Possibly, in this case, I disagree. The pid is initially unknown set to in the client, and can change afterwards. In reality, the pid of the window doesn't change, but it can still be set after being initially 0. In fact, it has to be. In the end, I don't care much, but having it connected to dataChanged makes this whole thing way more consistent, and it makes sense semantically. IMO, not putting it behind dataChanged is semantic wanking making the interface harder to understand, not easier, and not better. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D5872 To: sebas, #plasma, hein, davidedmundson Cc: bshah, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
D5872: pidChanged also signals dataChanged in WindowModel
sebas requested review of this revision. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D5872 To: sebas, #plasma, hein, davidedmundson Cc: bshah, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
D5877: Use absolute path instead of "." as path to watch
elvisangelaccio edited the summary of this revision. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5877 To: elvisangelaccio, dfaure Cc: #frameworks
D5877: Use absolute path instead of "." as path to watch
elvisangelaccio edited the summary of this revision. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5877 To: elvisangelaccio, dfaure Cc: #frameworks
D5877: Use absolute path instead of "." as path to watch
elvisangelaccio edited the summary of this revision. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5877 To: elvisangelaccio, dfaure Cc: #frameworks
D5877: Use absolute path instead of "." as path to watch
elvisangelaccio created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY Either "." was explicitly added (in which case we use the absolute path of the cwd), or it's added as side effect of something else. For example in bug #374075 KIO adds ":/kio5/newfile-templates" as path to watch (this is probably another bug in itself) and this results in "." being passed as _path of addEntry(). If we are already watching "/home/user", this breaks the emission of the dirty() signal for every new children of "/home/user" (somehow, the relative path is used for them, e.g. "./foo.txt" instead of "/home/user/foo.txt"). In particular, in inotifyEventReceived() e->m_client is empty and so e->path is not added to e->m_pendingFileChanges. Replacing "." with the absolute path fixes this issue. BUG: 374075 FIXED-IN: 5.35 TEST PLAN From dolphin, Create New -> Text File in a folder which is also the current working directory of the dolphin process. REPOSITORY R244 KCoreAddons BRANCH master REVISION DETAIL https://phabricator.kde.org/D5877 AFFECTED FILES autotests/kdirwatch_unittest.cpp src/lib/io/kdirwatch.cpp To: elvisangelaccio, dfaure Cc: #frameworks
D5872: pidChanged also signals dataChanged in WindowModel
bshah added a comment. In https://phabricator.kde.org/D5872#109874, @sebas wrote: > What's d581? I think he meant https://phabricator.kde.org/D5851 REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D5872 To: sebas, #plasma, hein, davidedmundson Cc: bshah, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
D5872: pidChanged also signals dataChanged in WindowModel
sebas added a comment. What's d581? REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D5872 To: sebas, #plasma, hein, davidedmundson Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
D3883: Generate gperf output at build time
This revision was automatically updated to reflect the committed changes. Closed by commit R270:883e1ada57c4: Generate gperf output at build time (authored by pino). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D3883?vs=9532=14569#toc REPOSITORY R270 KCodecs CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D3883?vs=9532=14569 REVISION DETAIL https://phabricator.kde.org/D3883 AFFECTED FILES CMakeLists.txt src/CMakeLists.txt src/kcharsets.cpp src/kentities.h To: pino, #frameworks
D5871: Remove obviously wrongly-named symbolic links
This revision was automatically updated to reflect the committed changes. Closed by commit R267:1b641f89d752: Remove obviously wrongly-named symbolic links (authored by marten). REPOSITORY R267 Oxygen Icons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5871?vs=14560=14564 REVISION DETAIL https://phabricator.kde.org/D5871 AFFECTED FILES 128x128/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 16x16/mimetypes/text-x-generic.svapplication-x-awk.png 16x16/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 22x22/mimetypes/text-x-generic.svapplication-x-awk.png 22x22/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 256x256/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 32x32/mimetypes/text-x-generic.svapplication-x-awk.png 32x32/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 48x48/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 64x64/mimetypes/text-x-generic.svapplication-x-awk.png 64x64/mimetypes/text-x-generic.svapplicatiopn-x-awk.png To: marten, #plasma, davidedmundson Cc: plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
D5871: Remove obviously wrongly-named symbolic links
davidedmundson accepted this revision. This revision is now accepted and ready to land. REPOSITORY R267 Oxygen Icons REVISION DETAIL https://phabricator.kde.org/D5871 To: marten, #plasma, davidedmundson Cc: plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
D5872: pidChanged also signals dataChanged in WindowModel
davidedmundson requested changes to this revision. davidedmundson added a comment. This revision now requires changes to proceed. See d581 REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D5872 To: sebas, #plasma, hein, davidedmundson Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
D5872: pidChanged also signals dataChanged in WindowModel
sebas created this revision. Restricted Application added projects: Plasma on Wayland, Frameworks. Restricted Application added subscribers: Frameworks, plasma-devel. REVISION SUMMARY Eike removed this, but I'm not sure why. All other roles trigger datachanged, so I don't see why pid shouldn't. This patch fixes the tests, which I should not have broken in the first place. REPOSITORY R127 KWayland BRANCH master REVISION DETAIL https://phabricator.kde.org/D5872 AFFECTED FILES src/client/plasmawindowmodel.cpp To: sebas, #plasma, hein Cc: plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
D5871: Remove obviously wrongly-named symbolic links
marten created this revision. Restricted Application added projects: Plasma, Frameworks. Restricted Application added subscribers: Frameworks, plasma-devel. REVISION SUMMARY It's not clear where these entries came from, but they are clearly a mistake - either scripting or copy-and-paste. The intended 'text-x-generic' and 'application-x-awk' icons still exist. TEST PLAN Built and installed oxygen-icons5 with these changes, checked correct installation of icons. REPOSITORY R267 Oxygen Icons REVISION DETAIL https://phabricator.kde.org/D5871 AFFECTED FILES 128x128/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 16x16/mimetypes/text-x-generic.svapplication-x-awk.png 16x16/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 22x22/mimetypes/text-x-generic.svapplication-x-awk.png 22x22/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 256x256/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 32x32/mimetypes/text-x-generic.svapplication-x-awk.png 32x32/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 48x48/mimetypes/text-x-generic.svapplicatiopn-x-awk.png 64x64/mimetypes/text-x-generic.svapplication-x-awk.png 64x64/mimetypes/text-x-generic.svapplicatiopn-x-awk.png To: marten, #plasma Cc: plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
D3830: Add a new FindGperf module
This revision was automatically updated to reflect the committed changes. Closed by commit R240:c61aee80d647: Add a new FindGperf module (authored by pino). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D3830?vs=9502=14562#toc REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D3830?vs=9502=14562 REVISION DETAIL https://phabricator.kde.org/D3830 AFFECTED FILES find-modules/FindGperf.cmake To: pino, #windows, #frameworks, #build_system, kde-mac, adridg, rjvbb Cc: kfunk, rjvbb, adridg
D5866: ecm_qt_declare_logging_category(): more unique include guard for header
kossebau added a comment. A different reasoning for why not yet using `#pragma once`: this macro targets users of projects with at least cmake and Qt. Unless Qt itself does not use that pragma, let's not risk to screw over people who try to reuse ECM for some non-mainstream setup, unless we have no other choice. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D5866 To: kossebau, #frameworks, #build_system Cc: elvisangelaccio
D5866: ecm_qt_declare_logging_category(): more unique include guard for header
kossebau added a comment. Yes, `#pragma once` might be the nicer solution here. I stayed away from proposing it though, as for one it is not a real standard by specifications and also by KDE coding traditions. And I would not like to be the one adding (and thus being responsible) the guinea pig to find out if that pragma is good enough for the real world ;) Given the generated file is build-specific, one could write a configure test to check if the compiler supports it and then use that. Left for the next person though, the patch as-is solves the issues I hit already ;) REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D5866 To: kossebau, #frameworks, #build_system Cc: elvisangelaccio
D5867: Add or improve "Generated. Don't edit" messages and make consistent
kossebau created this revision. Restricted Application added projects: Frameworks, Build System. REPOSITORY R240 Extra CMake Modules BRANCH improvegeneratednote REVISION DETAIL https://phabricator.kde.org/D5867 AFFECTED FILES modules/ECMQmLoader.cpp.in modules/ECMQtDeclareLoggingCategory.cpp.in modules/ECMQtDeclareLoggingCategory.h.in modules/ECMVersionHeader.h.in To: kossebau, #frameworks, #build_system
D5866: ecm_qt_declare_logging_category(): more unique include guard for header
elvisangelaccio added a comment. What about `#pragma once`? Is there some real-world compiler that doesn't support it? REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D5866 To: kossebau, #frameworks, #build_system Cc: elvisangelaccio
D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function
rjvbb marked an inline comment as done.
rjvbb added a comment.
> I'll hope to find some time to test on Window, wouldn't want this to be
merged before that.
Should we read "not to be merged before *you* have tested it"? I have nothing
against that, esp. if you also find the exact value to use for the MSC_VERSION
test ;)
INLINE COMMENTS
> kfunk wrote in KDECompilerSettings.cmake:226
> `MSVC_VERSION GREATER 1900` is sufficient. No need for `${...}`
Will fix that before committing anything.
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
https://phabricator.kde.org/D5865
To: rjvbb, #frameworks, #build_system, cgilles
Cc: kfunk
D5866: ecm_qt_declare_logging_category(): more unique include guard for header
kossebau created this revision. Restricted Application added projects: Frameworks, Build System. REVISION SUMMARY The old guard was created just from the identifier + _H, which runs the chance to clash in projects which use an identifier matching the project name and which also have a class or central file header which is named by the project and then has an include guard matching the filename. Example: project ABC -> abc.h with ABC_H guard identifier ABC, header debug.h -> debug.h with ABC_H guard any.cpp including both abc.h and debug.h will see only one content Using both the header file name and identifier for the guard name and prefixing it additionally with a macro specific term should make the guard both follow the usual pattern for guards matching the file name and also add some namespacing to allow for similar named header files in bigger projects (e.g. "debug.h") which could be included in the same include tree. REPOSITORY R240 Extra CMake Modules BRANCH improveqtlogdeclareheaderguard REVISION DETAIL https://phabricator.kde.org/D5866 AFFECTED FILES modules/ECMQtDeclareLoggingCategory.cmake To: kossebau, #frameworks, #build_system
D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function
kfunk added a comment.
LGTM in general. I'll hope to find some time to test on Window, wouldn't want
this to be merged before that.
INLINE COMMENTS
> KDECompilerSettings.cmake:226
> +if (MSVC)
> +if (${MSVC_VERSION} GREATER 1900)
> +# /permissive- became the preferred switch to obtain
> standards-compliant
`MSVC_VERSION GREATER 1900` is sufficient. No need for `${...}`
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
https://phabricator.kde.org/D5865
To: rjvbb, #frameworks, #build_system, cgilles
Cc: kfunk
D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function
rjvbb created this revision. Restricted Application added projects: Frameworks, Build System. REVISION SUMMARY A recent commit disabled support for named (logical) operators (and, not, bitand, ) when using GCC, Clang or ICC. This patch adds a function to (re)enable them in a cross-platform fashion in projects where they are required, the same way C++ exceptions are handled. I have implemented the MSVC-specific part to the best of my knowledge but cannot test it and have not yet been able to figure out what the exact MSVC_VERSION value is to test against. "GREATER 1900" should be correct but may not be specific enough. TEST PLAN Tested on Mac and Linux (with digiKam 5.5.0). Someone should test the value of MSVC_VERSION with MSVC 2015 Update 3 - in case I cannot get that information via the CMake ML. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D5865 AFFECTED FILES kde-modules/KDECompilerSettings.cmake To: rjvbb, #frameworks, #build_system, cgilles
D5856: Use KDirWatch removeDir/addDir instead of stopDirScan/restartDirScan
dfaure added a comment. In fact, CopyJob has no way to know whether a view somewhere is watching that directory. When an app other than Dolphin uses CopyJob, there isn't going to be any watching, possibly. So I think we need to remove the qDebug in KDirWatch so that removeDir is silent if the dir wasn't watched [this is faster than checking before removing, from the outside]. At the same time, this shows that the CopyJob handling of m_parentDirs can be improved a little bit. Posting an update. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D5856 To: dfaure, aacid Cc: #frameworks
D5856: Use KDirWatch removeDir/addDir instead of stopDirScan/restartDirScan
dfaure updated this revision to Diff 14545. dfaure added a comment. avoid doing removeDir twice REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5856?vs=14515=14545 BRANCH master REVISION DETAIL https://phabricator.kde.org/D5856 AFFECTED FILES src/core/copyjob.cpp src/core/deletejob.cpp To: dfaure, aacid Cc: #frameworks
D5775: Don't include the pid in the dbus path when on flatpak
apol updated this revision to Diff 14541. apol added a comment. Fix regression REPOSITORY R271 KDBusAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5775?vs=14301=14541 BRANCH arcpatch-D5775 REVISION DETAIL https://phabricator.kde.org/D5775 AFFECTED FILES src/kdbusservice.cpp To: apol, #frameworks, jgrulich Cc: dfaure, davidedmundson, aacid
D5851: Fix the PID updated in PlasmaWindowedModel
davidedmundson abandoned this revision. davidedmundson added a comment. That would also be completely fine from my POV. I should have read the original review request, I was just fixing the tests based on what was currently here. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D5851 To: davidedmundson, #plasma Cc: graesslin, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
