Code for configure and info dialogs is largely based on the
old KCM's, with light style updates and reformatting, and a
different mechanism for setting the transient parent from
the Qt Quick frontend.
Details
Details
Diff Detail
Diff Detail
- Repository
- R108 KWin
- Branch
- master
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 6192 Build 6210: arc lint + arc unit
Comment Actions
Would it be feasible to create a model for virtual desktop switching animations instead of hard coding them?
Comment Actions
I'm not sure. Keep in mind this code is mostly out of the old KCM (the benefit: it's tested already), and I haven't checked if the Effects KCM is similarly hardcoded or not. Thoughts:
- The code currently uses hard-coded enumeration of built-in effects. Is there an API to get built-in effects by category?
- The code currently used hard-coded enumeration the fadedesktop effect. Is there an API to get installed effects by category?
If the enumeration has to be done hard-coded anyway, then a model wrapper would just be a fake improvement. Maybe the internal Effects APIs in KWin should support some of this management boilerplate to make a model easier.
Comment Actions
No, effects are not hard coded in the Desktop Effects KCM's model.
- The code currently uses hard-coded enumeration of built-in effects. Is there an API to get built-in effects by category?
Not that I'm aware of. You could get list of all builtin effects and then use std::copy_if, for example.
- The code currently used hard-coded enumeration the fadedesktop effect. Is there an API to get installed effects by category?
Well, you could use KPackage::PackageLoader::findPackages.
In general, it would be great if we could share the model between the Desktop Effects KCM and this KCM, imho.
Comment Actions
What I guess I'm asking is: Do we even have category metadata for the plugins? From the above I'm guessing the answer is yes.
In any case, I unfortunately don't have time to work on a model-based revision until 2019, so if this version is not accepted it might not get done for 5.15 as the VDG desired. If someone else wants to take this over please do. Sorry, just a time question.
Comment Actions
Yes, we have.
I unfortunately don't have time to work on a model-based revision until 2019, so if this version is not accepted it might not get done for 5.15 as the VDG desired.
Yeah, it would be great to go with the model-based approach. For what it's worth, in theory, distros can ship their own virtual desktop switching animations, so we probably should be generic.
Comment Actions
EDIT: Also, one could install a virtual desktop switching animation from store.kde.org. :-)
Comment Actions
I agree with you, although I'll add that not having the setting at all in the KCM is a regression, but not having the model is not as the old KCM didn't have that ability. Sometimes we reject patches without thinking about the end-user picture.
Comment Actions
Yes and no, the problem with temporary "workarounds" is that they often become permanent. So, I still suggest to go with the "right" approach even if it'll cause that regression. If other KWin folks think otherwise, let's then go with the hardcoded effects.
Comment Actions
UI looks good and works as expected. Thanks for the fast turnaround on this! I have a few code questions:
| kcmkwin/kwindesktop/package/contents/ui/main.qml | ||
|---|---|---|
| 229 | Is all this whitespace between every item really necessary? Or are you just following the existing style? | |
| kcmkwin/kwindesktop/virtualdesktops.cpp | ||
| 3 | These should probably be in chronological order | |
| 320 | Does this need a layout if it will only have one thing in it? Is there a reason we can't put the KCM into the QVBoxLayout that belongs to configDialog? | |
| kcmkwin/kwindesktop/virtualdesktops.h | ||
| 97 | Could we use an enum for this instead of an int? | |
Comment Actions
FWIW, I would be in favor of shipping the current, existing approach for 5.15 and investigating a model-based approach for 5.16 if we run out of time for 5.15.
I think there is a lot of value to re-using existing code that is tested and has been in production for ages rather than ship a UI regression just because the backend isn't perfect yet. Best not to let the perfect be the enemy of the good IMO.
| kcmkwin/kwindesktop/package/contents/ui/main.qml | ||
|---|---|---|
| 229 | It's necessary for code readability. I dislike dense code without whitespace, and will generally reject it in patches to codebases I maintain. If you don't put an empty line after a } I will generally dislike your code, it's like writing an essay without paragraph breaks. Readability is a prime concern in writing code. | |
| kcmkwin/kwindesktop/virtualdesktops.cpp | ||
| 320 | Dunno, don't care. It's existing code copied over, there's no need to change and possibly break it. | |
| kcmkwin/kwindesktop/virtualdesktops.h | ||
| 97 | Not really. Keep in mind that as per Vlad's comments, the whole thing is supposed to eventually be extensible, and there's no way to enumerate unknown effects ahead of time. | |
Comment Actions
Yes and no, the problem with temporary "workarounds" is that they often become permanent. So, I still suggest to go with the "right" approach even if it'll cause that regression.
This isn't a "workaround". The original code, apparently written by the then-maintainer of KWin, was a workaround, and I agree it's quite bad code. This is a refactoring commit to avoid a user-visible regression, designed to minimize risk of further regression in awareness of the specific point on the release timeline. Trying to force contributors to rewrite code to attain some unrelated ideal (and suggesting to rewrite a second module in the process) misappreciates the goal and situation, is unnecessarily punishing (pinning past mistakes of the project on the contributor who tries to get something else done) and not how you make contribution to KWin rewarding. IME, your chance of getting a follow-up commit cleaning up your own mess is a lot higher if contribution to the project in the first place is rewarding enough to create a stakeholder who cares.
I'm abandoning this revision as I'm no longer interested in contributing to the KWin project for the moment, after this and some other recent reviews I've observed. The maintainer/review community on it has priorities I don't agree with and shows too little regard for release schedules, user value or team health in my eyes.