Fix duplicated entries in Google Calendar and Tasks resource settings dialog
ClosedPublic

Authored by poboiko on Jan 7 2018, 7:28 PM.

Details

Summary

The problem is following:
when I open settings dialog for my Google Calendar resource (in i.e. KOrganizer), all the items in "Calendars" and "Tasklists" lists gets duplicated. I wasn't able to find such a bug in BugZilla, though.

On first launch of settings dialog, GoogleSettingsDialog::currentAccountChanged signal gets called twice in a row:

  1. first time due to currentIndexChanged signal from combobox (common/googlesettingsdialog.cpp:72)
  2. second time, manually, in GoogleSettingsDialog::reloadAccounts() (common/googlesettingsdialog.cpp:107). Somehow, the disconnect-workaround there doesn't work (BTW, the code around there looks weird. Should there be "connect" on line 149?)

As a result, SettingsDialog::slotReloadCalendars() and SettingsDialog::slotReloadTaskLists() are called twice, running job and clearing corresponding lists twice as well. Then, in SettingsDialog::slot***Retrieved(), which gets called twice again after job is done, the data gets added twice. The contents of lists gets duplicated. Simple as that.

The problem is thus that list gets cleared when job is set up, and not before populating the lists.
I propose the most trivial (and harmless) solution to this problem: just clear corresponding lists once again, when the job is done, and right before putting new data there.

Test Plan

Opened the settings dialog for my google account. Data is no longer duplicated. I'm happy.

Diff Detail

Repository
R44 KDE PIM Runtime
Lint
Lint Skipped
Unit
Unit Tests Skipped
poboiko requested review of this revision.Jan 7 2018, 7:28 PM
poboiko created this revision.

The workaround in GoogleSettingsDialog::reloadAccounts() (with disconnecting/connecting) doesn't work because signal gets connected as

  • connect(m_accComboBox, QOverload<const QString &>::of(&KComboBox::currentIndexChanged), this, &GoogleSettingsDialog::currentAccountChanged);

and gets disconnected as

  • disconnect(m_accComboBox, SIGNAL(currentIndexChanged(QString)), this, SIGNAL(currentAccountChanged(QString)));

Somehow it doesn't match (?), and does not disconnect. If I switch to new signal/slot API in disconnect, the GoogleSettingsDialog::currentAccountChanged signal gets emitted only once. Probably, this is a better solution.

"Somehow it doesn't match (?), and does not disconnect. If I switch to new signal/slot API in disconnect, the GoogleSettingsDialog::currentAccountChanged signal gets emitted only once. Probably, this is a better solution." yep it's better to with disconnect to new connect api too for sure

My suggestion is to implement both fixes.
On one hand, wrong connect/disconnect behaviour is not OK and should be fixed.
On the other hand, we're dealing here with some sort of race condition, which in principle can be triggered i.e. if one has slow internet connection and somehow clicks "reload" twice (?), changes active account (?) before previous data arrived, or something like that. I'm not quite sure it is impossible; and additional clear() call looks rather harmless.

What do you think?

Yup, a combination of both the fixes would be good.

poboiko updated this revision to Diff 25019.Jan 9 2018, 1:44 PM
dvratil accepted this revision.Jan 10 2018, 11:08 AM

Thank you!

This revision is now accepted and ready to land.Jan 10 2018, 11:08 AM
This revision was automatically updated to reflect the committed changes.

a) is the fix relevant also for Applications/17.12?
b) please try to use arc land next time. This way the commit message has no context at all and having to check the review even for basic information about the change is not nice.

a) is the fix relevant also for Applications/17.12?

I guess so. I have experienced this for some time, it doesn't look like a recent regression.

b) please try to use arc land next time. This way the commit message has no context at all and having to check the review even for basic information about the change is not nice.

Sorry! Will provide more details in commit message next time...
Haven't used the Arcanist before, I'm just more familiar with good old git diff & web interface. I've just found this guide; I assume this is the right one.

a) is the fix relevant also for Applications/17.12?

I guess so. I have experienced this for some time, it doesn't look like a recent regression.

Oki, so let's see what @dvratil says, if it's worth to cherry-pick it.

b) please try to use arc land next time. This way the commit message has no context at all and having to check the review even for basic information about the change is not nice.

Sorry! Will provide more details in commit message next time...
Haven't used the Arcanist before, I'm just more familiar with good old git diff & web interface. I've just found this guide; I assume this is the right one.

Yes, that's the page. There is also a git-phab command from a 3rd-party, but I think that no one from KDE so far tested it.

a) is the fix relevant also for Applications/17.12?

I guess so. I have experienced this for some time, it doesn't look like a recent regression.

Oki, so let's see what @dvratil says, if it's worth to cherry-pick it.

Indeed, it has been broken since forever, so it can go to stable as well. I took care of it.