<table><tr><td style="">poboiko updated this revision to Diff 81212.<br />poboiko added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D29148">View Revision</a></tr></table><br /><div><div><p>Fixed a race condition that could happen due to focusOutEvent</p>

<p>When user changes the document via <tt style="background: #ebebeb; font-size: 13px;">KJotsTreeView</tt>, there are several places which react to this change:</p>

<ol class="remarkup-list">
<li class="remarkup-list-item">Slot <tt style="background: #ebebeb; font-size: 13px;">KJotsWidget::selectionChanged</tt>, which tries to save the "old" document</li>
<li class="remarkup-list-item">Slot <tt style="background: #ebebeb; font-size: 13px;">KJotsEdit::selectionChanged</tt>, which tries to <tt style="background: #ebebeb; font-size: 13px;">setReadOnly</tt> based on first element of selection</li>
<li class="remarkup-list-item">Slots <tt style="background: #ebebeb; font-size: 13px;">KJotsWidget::rowsInserted/rowsRemoved</tt>, which change the <tt style="background: #ebebeb; font-size: 13px;">KJotsEdit::document()</tt> to the new one</li>
<li class="remarkup-list-item"><tt style="background: #ebebeb; font-size: 13px;">KJotsEdit::focusOutEvent</tt> triggers, which also tries to save the "old" document.</li>
</ol>

<p>The <tt style="background: #ebebeb; font-size: 13px;">focusOutEvent</tt> could actually come AFTER the selection has changed, but BEFORE the <tt style="background: #ebebeb; font-size: 13px;">setDocument</tt> call,<br />
which leaves <tt style="background: #ebebeb; font-size: 13px;">KJotsEdit</tt> in an invalid state: the document() corresponds to one note, while <tt style="background: #ebebeb; font-size: 13px;">selection</tt> points to another. <br />
Because of that, <tt style="background: #ebebeb; font-size: 13px;">savePage</tt> actually overwrites "new" note with the contents of "old" one. I lost several notes because of it.</p>

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

<ul class="remarkup-list">
<li class="remarkup-list-item">Select couple of notes from a book using mouse, so <tt style="background: #ebebeb; font-size: 13px;">KJotsBrowser</tt> appears</li>
<li class="remarkup-list-item">Select a single note from a book using mouse</li>
<li class="remarkup-list-item">Quickly press <tt style="background: #ebebeb; font-size: 13px;">Ctrl+PageUp</tt> / <tt style="background: #ebebeb; font-size: 13px;">Ctrl+PageDown</tt> multiple times to switch between notes</li>
</ul>

<p>Instead, I propose the following:</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">Don't let <tt style="background: #ebebeb; font-size: 13px;">KJotsEdit</tt> interact with <tt style="background: #ebebeb; font-size: 13px;">selectionModel</tt>, instead just provide it with a single <tt style="background: #ebebeb; font-size: 13px;">QModelIndex</tt> pointing to a document</li>
</ul>

<p>(so that its state is always consistent)</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">This is done via <tt style="background: #ebebeb; font-size: 13px;">KJotsEdit::setModelIndex</tt> method, which does all the work in a single place:<ul class="remarkup-list">
<li class="remarkup-list-item">Saves the old document if necessary</li>
<li class="remarkup-list-item">Calls <tt style="background: #ebebeb; font-size: 13px;">KJotsEdit::setDocument</tt> if necessary</li>
<li class="remarkup-list-item">Also sets the <tt style="background: #ebebeb; font-size: 13px;">QTextCursor</tt> (if it was stored)</li>
<li class="remarkup-list-item">tries to <tt style="background: #ebebeb; font-size: 13px;">setReadOnly</tt> based on <tt style="background: #ebebeb; font-size: 13px;">NoteLockAttribute</tt></li>
</ul></li>
</ul></div></div><br /><div><strong>REPOSITORY</strong><div><div>R573 KJots</div></div></div><br /><div><strong>CHANGES SINCE LAST UPDATE</strong><div><a href="https://phabricator.kde.org/D29148?vs=81083&id=81212">https://phabricator.kde.org/D29148?vs=81083&id=81212</a></div></div><br /><div><strong>BRANCH</strong><div><div>focus (branched from master)</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D29148">https://phabricator.kde.org/D29148</a></div></div><br /><div><strong>AFFECTED FILES</strong><div><div>src/kjotsbrowser.cpp<br />
src/kjotsbrowser.h<br />
src/kjotsedit.cpp<br />
src/kjotsedit.h<br />
src/kjotswidget.cpp<br />
src/kjotswidget.h</div></div></div><br /><div><strong>To: </strong>poboiko, dvratil<br /><strong>Cc: </strong>kde-pim, fbampaloukas, dcaliste, dvasin, rodsevich, winterz, vkrause, mlaurent, knauss, dvratil<br /></div>