romangg added a comment.
In D29024#669970 <https://phabricator.kde.org/D29024#669970>, @dvratil wrote: > If I may add my two cents here, Hi Daniel, sorry for the late reply. But I was busy as I had to kick things off now with the libkscreen fork. > I agree with David that introducing a plugin for a plugin is a bit over the top. Is it though? Just because it's normally not done must not mean it should never be done. The Wayland platform is pretty special and pretty new so why not try something unusual? That said if a one-level plugin-infrastructure is in a similar way possible and somebody has implemented it: sure, let's go with that then. > Whats the issue with contributing your backend into libkscreen upstream instead? I would have been fine with that. But I anticipated there will be opposition to that. And I believe this was a correct assessment if you read the comments in D29028 <https://phabricator.kde.org/D29028>: > I don't see why we should have that in KDE code. and > I don't really see why we'd want to support something that is not offering ABI stability and doesn't push Plasma in any direction. These comments show pretty clearly that there is political opposition to any integration effort with KWinFT. I don't want to comment on this any further but I think this is a really sad state of affairs and it shines a very unflattering light on the KDE Plasma project. Anyways, at least on a personal level I explain this with initial resentment and will stay open to reconciliation. > If there's any code that could be shared between KWayland backend and your new backend, you can create a small static library (basically reusing most of what you've already done). Otherwise you are basically forcing libkscreen to accommodate a single usecase for your fork, and the community has the right to push back on that as it makes things more complicated for them just to make live easier for you. It was not my intention to make things complicated for other people. And since I was basically the only person with larger contributions to libkscreen and KScreen in the last 1.5 years, I was its last maintainer and I definitely planned to do more work on it in the future the one person who would have needed to endure additional complexity in particular would have been me. ;) > Regarding the asynchronous plugin loading. You still need to block the constructor of the KWayland backend in order to figure out which plugin you want to load, because the KScreen backend loading code synchronously calls `isValid()` just after constructing the backend. But I think that's fine, otherwise the consumer gets incorrect or incomplete information about the current/initial setup. You think otherwise? > So it's nice you try loading your plugins asynchronously, but it doesn't solve much since all you do is blocking-waiting for a bunch o threads to finish, rather than just blocking-waiting for the calls to the registry to finish... The current backend loading code in libkscreen is of course simple and stupid because it was first designed only with XRandR and Fake backends in mind. Later on QScreen was added which also works synchronously. When Wayland support was introduced, Sebas rewrote lot of the code, but some of the assumptions were kept, like plugins initializing synchronously. It's not too hard to fix the backend loading code to not only make assumptions about backends based on Qt platform and to allow for true asynchronous initialization. I don't know if it's hard or not. When I look at the current backend loading code it looks pretty complicated because of the out-of-process loading so I didn't want to manipulate that directly. Since I have now forked libkscreen to Disman <https://gitlab.com/kwinft/disman> I might look into that through some bigger design changes. But I will likely rather try to slime the library down than packing stuff on top. I created some tasks for that. Your opinion as former KScreen maintainer and expert is of course very welcome: https://gitlab.com/kwinft/disman/-/issues I will close this review now. REPOSITORY R110 KScreen Library REVISION DETAIL https://phabricator.kde.org/D29024 To: romangg, #plasma Cc: dvratil, ngraham, davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
