- save changes to the config files when the layout is saved
- :name() works even if netrootinfo isn't there
- as soon a rootinfo is set, connect all the desktops with name changes
Details
- Reviewers
hein davidedmundson graesslin - Group Reviewers
Plasma KWin - Maniphest Tasks
- T4457: [kwayland] Virtual Desktop protocol
- Commits
- R108:e983319d0d15: Fix changing the number of rows via the dbus protocol
- tested with the kcm to add, remove and rename desktops, all of that works
- setting the number of rows still only partly works: kwin notices it but
the pager doesn't notice, a plasma restart is needed
Diff Detail
- Repository
- R108 KWin
- Branch
- phab/vdconsistency
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 6225 Build 6243: arc lint + arc unit
dbusinterface.cpp | ||
---|---|---|
475 ↗ | (On Diff #47032) | VirtualDesktopManager::createVirtualDesktop calls save on line 464 |
virtualdesktops.cpp | ||
246 | we can get rid of this connect if we move the statement at ~468 to be outside if (m_rootInfo) | |
623 | so is m_rows the canonical source or this? | |
763 | I don't see why it would be an issue given we only load entries up to the value in "Number" anyway? If you do want to remove them, I'd rather something that uses the group as the data source. Then we don't need other changes and managing a second set of data. I think the current code breaks if you remove + add without calling save inbetween. Something along the lines of: for (int i = count() + 1; group.hasEntry("Id_" + i; i++) { group.deleteEntry("Id_" + i); } |
virtualdesktops.cpp | ||
---|---|---|
623 | hmm, thinking about it should probably be m_rows.. |
I have problems following what this change does. All of it has nothing to do with the title. Could you please split this into three commits and reviews for each of the addressed points.
Please also extend the existing virtual desktop manager test for the new changes you are doing.
virtualdesktops.cpp | ||
---|---|---|
482 | Unrelated empty line | |
708 | Unrelated empty line |
well, the thing the patch does is really one, making setting rows work when done via the dbus protocol, i don't think is really splittable
Please also extend the existing virtual desktop manager test for the new changes you are doing.
the last changes in the review were mostly to make existing tests work again tough
one test that may be useful to add is how rows() changes by changing the count
No, your change to the dbus interface is one line addition which does not even interact with the changes in virtual desktop manager. Your change might be driven by effects you see in the dbus protocol, but the change you put here has nothing to do with the dbus interface.
dbusinterface.cpp | ||
---|---|---|
448 ↗ | (On Diff #47900) | This save doesn't make sense. The option is not saved there, so that save does not work. I really don't understand what you want to achieve. |
unrelated addition of empty line
Hot take: KWin code is nigh-unreadable at times because it tends to be a dense blob devoid of sensible whitespace. Contending with that and then having improvements rejected in review makes KWin extra-despiriting to contribute to.
If you think that it needs more empty lines: do add them. But please do it in a separate commit. I find it extremely hard to read the review if all over the place there are additions of empty lines. I always wonder: what is it doing here. And it results in me focusing on stupid bullshit like the empty lines instead of being able to concentrate on the actual change.
Yes I'm aware I'm doing a hard review here. There is a very simple reason for this: the description does not match what the code does so I started to question everything what's done here.
empty lines reoved, added more range checks to m_rows to make sure is never 0
dbusinterface.cpp | ||
---|---|---|
448 ↗ | (On Diff #47900) | removed on last version |
virtualdesktops.cpp | ||
611–614 | uhm, where? (tough didn't work as without this saving desktop names gets corrupted and autotests fail without it | |
667 | almost, (the only place is not checking is when taking it from rootinfo) |
virtualdesktops.cpp | ||
---|---|---|
611–614 | From VirtualDesktopManager::save: if (s_loadingDesktopSettings) { return; } s_loadingDesktopSettings is set to true at start of VirtualDesktopManager::load and back to false at the end of the method. Given that this was identical to the newly introduced m_isLoading it got removed in 92b5ddf94c6b472d928c20e4d4bc5407b086e44b |
@mart this change broke a unit test: https://build.kde.org/job/Plasma/job/kwin/job/kf5-qt5%20SUSEQt5.11/288/testReport/projectroot/autotests/kwin_testScreenEdges/
Please run the test suite prior to push. It's quite sad to see a change both compile break and break unit tests. It's unfair against the developers who have to spend time to figure out which change broke it.