Fix changing the number of rows via the dbus protocol
ClosedPublic

Authored by mart on Nov 30 2018, 4:48 PM.

Details

Summary
  • 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
Test Plan
  • 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 5777
Build 5795: arc lint + arc unit
mart created this revision.Nov 30 2018, 4:48 PM
Restricted Application added a project: KWin. · View Herald TranscriptNov 30 2018, 4:48 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
mart requested review of this revision.Nov 30 2018, 4:48 PM
mart updated this revision to Diff 46903.Dec 5 2018, 3:40 PM
  • fix saving and loading rows number
mart updated this revision to Diff 47029.Dec 7 2018, 1:25 PM
  • ensure consistency of rows
mart updated this revision to Diff 47032.EditedDec 7 2018, 1:52 PM
  • initialize auto created desktops with default name
ngraham added a subscriber: ngraham.
hein accepted this revision.Dec 19 2018, 8:16 PM
This revision is now accepted and ready to land.Dec 19 2018, 8:16 PM
davidedmundson requested changes to this revision.Dec 19 2018, 11:45 PM
davidedmundson added a subscriber: davidedmundson.
davidedmundson added inline comments.
dbusinterface.cpp
475

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)

629

so is m_rows the canonical source or this?

770

I don't see why it would be an issue given we only load entries up to the value in "Number" anyway?
It wasn't an issue before?

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);
}
This revision now requires changes to proceed.Dec 19 2018, 11:45 PM
mart updated this revision to Diff 47880.Dec 20 2018, 10:07 AM
  • adress comments
mart added inline comments.Dec 20 2018, 10:08 AM
virtualdesktops.cpp
629

hmm, thinking about it should probably be m_rows..
m_rows is the number of rows it's configured to have, grid().height() is the number of rows it actually has.
They are usually the same, except when it's loading, when not all desktops have been loaded yet

graesslin requested changes to this revision.Dec 20 2018, 5:09 PM
graesslin added a subscriber: graesslin.

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
487

Unrelated empty line

715

Unrelated empty line

This revision now requires changes to proceed.Dec 20 2018, 5:09 PM
mart added a comment.Dec 20 2018, 5:26 PM

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.

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

mart updated this revision to Diff 47900.Dec 20 2018, 5:42 PM
  • add a test more
In D17265#380040, @mart wrote:

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.

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

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.

mart updated this revision to Diff 47902.Dec 20 2018, 6:08 PM
  • remove unrelated change
mart added a comment.Dec 20 2018, 6:09 PM

here, no more changes in dbusprotocol.cpp

graesslin added inline comments.Dec 20 2018, 6:09 PM
dbusinterface.cpp
448

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.

graesslin added inline comments.Dec 20 2018, 6:19 PM
virtualdesktops.cpp
601

unrelated addition of empty line

617–620

I don't understand this change. The method save has a protection to not do anything when calling save from loading.

673

Can we be sure that this is never 0?

675

unrelated addition of empty line

hein added a comment.Dec 20 2018, 6:26 PM

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.

In D17265#380071, @hein wrote:

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.

mart retitled this revision from Ensure consistency when the layout is changed via the dbus protocol to Fix changing the number of rows via the dbus protocol.Dec 21 2018, 9:20 AM
mart updated this revision to Diff 47937.Dec 21 2018, 10:00 AM
  • remove whitespaces
mart updated this revision to Diff 47939.Dec 21 2018, 10:01 AM
  • remove more whitespace
mart updated this revision to Diff 47940.Dec 21 2018, 10:03 AM
  • remove more whitespace
mart added a comment.Dec 21 2018, 10:04 AM

empty lines reoved, added more range checks to m_rows to make sure is never 0

dbusinterface.cpp
448

removed on last version

virtualdesktops.cpp
617–620

uhm, where? (tough didn't work as without this saving desktop names gets corrupted and autotests fail without it

673

almost, (the only place is not checking is when taking it from rootinfo)
will add a qmax there too

graesslin added inline comments.Dec 23 2018, 9:44 AM
virtualdesktops.cpp
617–620

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 updated this revision to Diff 48864.Jan 7 2019, 2:24 PM
  • Merge branch 'master' into phab/vdconsistency
  • use only s_loadingDesktopSettings
zzag added a subscriber: zzag.Jan 7 2019, 6:23 PM
zzag added inline comments.
virtualdesktops.cpp
665–666

Wasn't it removed in another revision? Also, unrelated whitespace change.

770

Extra whitespace before group.

775

Extra empty line.

mart updated this revision to Diff 48908.Jan 7 2019, 6:32 PM
  • remove extra whitespaces
davidedmundson accepted this revision.Jan 8 2019, 12:48 PM
graesslin accepted this revision.Jan 8 2019, 5:29 PM
This revision is now accepted and ready to land.Jan 8 2019, 5:29 PM
mart added inline comments.Jan 9 2019, 9:08 AM
virtualdesktops.cpp
665–666

doesn't seem to compare it correctly against master?

770

indentation seems correct?

This revision was automatically updated to reflect the committed changes.

@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.

Unit test is now fixed.