[KJotsWidget] Fix several KJotsEdit focus issues (race condition)
ClosedPublic

Authored by poboiko on Apr 24 2020, 10:50 AM.

Details

Summary

This patch also fixes a race condition that could happen due to the focusOutEvent. When user changes the document via KJotsTreeView, there are several places which react to this change:

  1. Slot KJotsWidget::selectionChanged, which tries to save the "old" document
  2. Slot KJotsEdit::selectionChanged, which tries to setReadOnly based on first element of selection
  3. Slots KJotsWidget::rowsInserted/rowsRemoved, which change the KJotsEdit::document() to the new one
  4. KJotsEdit::focusOutEvent triggers, which also tries to save the "old" document.

The focusOutEvent could actually come AFTER the selection has changed, but BEFORE the setDocument call,
which leaves KJotsEdit in an invalid state: the document() corresponds to one note, while selection points to another.
Because of that, savePage actually overwrites "new" note with the contents of "old" one. I lost several notes because of it.

This is hard to trigger manually, but it's sufficient to do a random combination of following things:

  • Select couple of notes from a book using mouse, so KJotsBrowser appears
  • Select a single note from a book using mouse
  • Quickly press Ctrl+PageUp / Ctrl+PageDown multiple times to switch between notes

Instead, I propose the following:

  • Don't let KJotsEdit interact with selectionModel, instead just provide it with a single QModelIndex pointing to a document (so that its state is always consistent)
  • This is done via KJotsEdit::setModelIndex method, which does all the work in a single place:
    • Saves the old document if necessary
    • Calls KJotsEdit::setDocument if necessary
    • Also sets the QTextCursor (if it was stored)
    • tries to setReadOnly based on NoteLockAttribute

This patch also fixes visual glitch:

  1. User tries to switch focus i.e. to KFontChooser to pick font he wants
  2. KJotsEdit catches focusOutEvent and does the autosave (savePage call)
  3. Autosave modifies the underlying data in the model via model()->setData()
  4. Selection model inside KJotsWidget catches dataChanged signal and calls renderSelection slot
  5. It switches focus back to KJotsEdit

so user has to press onto the KFontChooser second time (the same issue does not trigger again because autosave is skipped when document is not modified)
The fix is that KJotsEdit decides it should grab the focus only when the new document was opened explicitly.

Also, remove selectionModel from KJotsBrowser as it's not used there anyways.

Test Plan

The described issue does not trigger again

Diff Detail

Repository
R573 KJots
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
poboiko created this revision.Apr 24 2020, 10:50 AM
Restricted Application added a project: KDE PIM. · View Herald TranscriptApr 24 2020, 10:50 AM
poboiko requested review of this revision.Apr 24 2020, 10:50 AM
poboiko updated this revision to Diff 81212.Apr 25 2020, 10:44 PM

Fixed a race condition that could happen due to focusOutEvent

When user changes the document via KJotsTreeView, there are several places which react to this change:

  1. Slot KJotsWidget::selectionChanged, which tries to save the "old" document
  2. Slot KJotsEdit::selectionChanged, which tries to setReadOnly based on first element of selection
  3. Slots KJotsWidget::rowsInserted/rowsRemoved, which change the KJotsEdit::document() to the new one
  4. KJotsEdit::focusOutEvent triggers, which also tries to save the "old" document.

The focusOutEvent could actually come AFTER the selection has changed, but BEFORE the setDocument call,
which leaves KJotsEdit in an invalid state: the document() corresponds to one note, while selection points to another.
Because of that, savePage actually overwrites "new" note with the contents of "old" one. I lost several notes because of it.

This is hard to trigger manually, but it's sufficient to do a random combination of following things:

  • Select couple of notes from a book using mouse, so KJotsBrowser appears
  • Select a single note from a book using mouse
  • Quickly press Ctrl+PageUp / Ctrl+PageDown multiple times to switch between notes

Instead, I propose the following:

  • Don't let KJotsEdit interact with selectionModel, instead just provide it with a single QModelIndex pointing to a document

(so that its state is always consistent)

  • This is done via KJotsEdit::setModelIndex method, which does all the work in a single place:
    • Saves the old document if necessary
    • Calls KJotsEdit::setDocument if necessary
    • Also sets the QTextCursor (if it was stored)
    • tries to setReadOnly based on NoteLockAttribute
poboiko retitled this revision from [KJotsWidget] Don't switch focus to KJotsEdit on dataChanged to [KJotsWidget] Fix several KJotsEdit focus issues (race condition).Apr 25 2020, 10:49 PM
poboiko edited the summary of this revision. (Show Details)
poboiko updated this revision to Diff 81216.Apr 26 2020, 9:35 AM

Make KJotsEdit decide whether it wants focus or not
(based on whether it opened a new document, or reloaded something already opened)

poboiko edited the summary of this revision. (Show Details)Apr 26 2020, 9:37 AM
dvratil added inline comments.Apr 27 2020, 10:00 AM
src/kjotsedit.cpp
247

Could the m_index here be nullptr? Should there be a guard?

372–373

Could the m_index here be nullptr? Should there but a guard?

poboiko updated this revision to Diff 81323.Apr 27 2020, 10:22 AM

In theory, m_index can't be nullptr, because onLinkify can be triggered only from manage_link action,
which becomes available only when KJotsEdit gains focus; tooltipEvent can also only trigger when KJotsEdit
is available. And the only way KJotsEdit becomes available is through the setModelIndex call.

In practice, let's be safe and guard it anyways :)

dvratil accepted this revision.Apr 27 2020, 10:23 AM
This revision is now accepted and ready to land.Apr 27 2020, 10:23 AM
This revision was automatically updated to reflect the committed changes.