<table><tr><td style="">aacid marked 8 inline comments as done.<br />aacid 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/D8642" rel="noreferrer">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D8642#167826" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">D8642#167826</a>, <a href="https://phabricator.kde.org/p/rkflx/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@rkflx</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>Latest version is much improved, but still not there yet (see below). I'm giving this my approval nevertheless, because I'm confident that you'll be able to do the string changes now and fix the remaining problems in time for the RC (you could pretend I found the crasher only when testing the Beta ;). Please merge at your earliest convenience, I'll try to schedule another test run before the RC.</p>
<ol class="remarkup-list" start="24">
<li class="remarkup-list-item">Segfault: Add annotation, <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Save</span></span><span style="color: #92969D;"> → </span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Undo</span></span><span style="color: #92969D;"> → </span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Save</span></span></span>. Introduced by <a href="https://phabricator.kde.org/R223:055f2db76d58b559b8a215cd5713cdbd7265fddd" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">055f2db76d58</a>. Backtrace:</li>
</ol></div>
</blockquote>
<p>I can't seem to reproduce this. Which file and which annotation are you using?</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ol class="remarkup-list" start="25">
<li class="remarkup-list-item"><tt style="background: #ebebeb; font-size: 13px;">parttest</tt> still fails (now requires to press button in dialog, then crashes with similar backtrace to 24.)</li>
</ol></blockquote>
<p>Fixed the button press thing, was a regression of my last change to sync the modified status, good thing we have autottests :) (still don't get a crash)</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ol class="remarkup-list" start="26">
<li class="remarkup-list-item">Document is now named in warning dialog, but name includes full path. Instead, it should only show the file name as in KWrite, LibreOffice and the tab/window title. I guess it's just missing a <tt style="background: #ebebeb; font-size: 13px;">.fileName()</tt>.</li>
</ol></blockquote>
<p>Are you sure we want the filename only? what about if you have two files with the same filename and different paths?</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>Regarding the file format warning:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">I now understand why we need <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Continue</span></span></span> for <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Save As</span></span></span>.</li>
<li class="remarkup-list-item">27. It is still very confusing when using <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Save</span></span></span>, because for the user <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Continue</span></span></span> and <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Cancel</span></span></span> are exactly the same. Can we special-case with a flag and hide the last sentence and the button here? As <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Save</span></span></span> is the more common operation, this would streamline the user experience massively and reduce frustration. Most users won't read the text and assume <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Save</span></span><span style="color: #92969D;"> → </span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Continue</span></span></span> <em>will</em> actually save the changes.</li>
</ul></blockquote>
<p>Makes sense, the code is relatively complicated though, will see what i can get.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item">28. There is an inconsistency for <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Close</span></span></span>: If a document <tt style="background: #ebebeb; font-size: 13px;">canSwapBackingFile</tt> (e.g. for PDF), <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Continue</span></span></span> will keep the document open, but if it cannot (e.g. for epub), <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Continue losing changes</span></span></span> will close the document. I think <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Discard</span></span></span> in the previous dialog is enough to <em>actually</em> close, so for <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Save</span></span></span> (the only case triggered by closing) the <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Continue [l.c.]</span></span></span> buttons can be removed, really.</li>
</ul></blockquote>
<p>You mean PNG and not PDF, right? you should never get the file format warning for PDF. Otherwise makes sense too i guess, i'll see what i can do.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item">(Note I assume here that all formats support saving either all or no changes, but not only some. Is this true?)</li>
</ul></blockquote>
<p>At this point yes</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;">
<ol class="remarkup-list" start="29">
<li class="remarkup-list-item">See inline comments.</li>
</ol>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ol class="remarkup-list" start="18">
<li class="remarkup-list-item">Improve buttons to <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Continue</span></span></span> and <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Cancel</span></span></span>.</li>
</ol></blockquote>
<p>Disagreed, Continue might make sense, but why "Cancel"? Cancel is not an answer to "Do you want to continue?"</p></blockquote>
<p>Got a nice idea for a very smooth reading dialog: Just remove the question, then you can change the buttons.</p></blockquote>
<p>I understand you disagree with the current wording, but honestly i like it more than your suggestion.</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R223 Okular</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D8642" rel="noreferrer">https://phabricator.kde.org/D8642</a></div></div><br /><div><strong>To: </strong>aacid, mlaurent, rkflx<br /><strong>Cc: </strong>rkflx, lueck, mlaurent, michaelweghorn, ngraham, Okular, aacid<br /></div>