<table><tr><td style="">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#166340" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">D8642#166340</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>Another great feature \o/. Good news, I cannot reproduce or even describe the single crash I got and <strong>there are only small issues to be fixed before this can land (IMHO). The rest is optional polishing.</strong> (Thanks for the extensive test plan, BTW, made it easier to review. Sorry it took a while to find enough time for reviewing. Still some things to check, will continue tomorrow.)</p>
<p>As you've commented on some bugs with the Phab URL, I guess you'll close them manually?</p></div>
</blockquote>
<p>Yes either manually or as part of the merge commit</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>Have found several more related bugzillas, will triage and maybe close some once the Diff is in (I assume you'll just merge the branch to preserve Fabio's copyright?).</p></blockquote>
<p>Yes, merge is the correct way.</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 phantom-item"><ol class="remarkup-list">
<li class="remarkup-list-item">Comments on the code</li>
</ol></li>
<li class="remarkup-list-item"><tt style="background: #ebebeb; font-size: 13px;">parttest</tt> fails (missing <tt style="background: #ebebeb; font-size: 13px;">potato.jpg</tt>)</li>
</ul></blockquote>
<p>Wops, added.</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">see Laurent's comments</li>
<li class="remarkup-list-item">more <tt style="background: #ebebeb; font-size: 13px;">@since</tt> wrong (e.g. <tt style="background: #ebebeb; font-size: 13px;">TODO</tt>, <tt style="background: #ebebeb; font-size: 13px;">0.21</tt>, …)</li>
<li class="remarkup-list-item">Looked only briefly at the individual commits, it's just too much code to analyze in depth (i.e. "I trust you on the <tt style="background: #ebebeb; font-size: 13px;">const_cast</tt>" and the tests you added :). In general the <tt style="background: #ebebeb; font-size: 13px;">swapBackingFile</tt> mechanism seems to add a huge amount of complexity, but I guess if there was an easier path you would have followed it.</li>
<li class="remarkup-list-item">Did you run this with ASAN and friends to check for any problems?</li>
</ul></blockquote>
<p>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;"><ul class="remarkup-list">
<li class="remarkup-list-item">There is some code duplication for non-trivial amounts of code (grep for <tt style="background: #ebebeb; font-size: 13px;">loadSyncFile</tt>).</li>
</ul></blockquote>
<p>You mean in swapBackingFile and swapBackingFileArchive ? I'll try to merge them.</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">
<li class="remarkup-list-item phantom-item"><ol class="remarkup-list">
<li class="remarkup-list-item">UI: Important issues (I consider these must-haves for the RC latests, but some might break string freeze)</li>
</ol></li>
<li class="remarkup-list-item"><kbd style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">Ctrl</kbd><span class="kbd-join" style="padding: 0 4px; color: #92969D;">+</span><kbd style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">S</kbd> saves and is visible in <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Configure Shortcuts</span></span></span>, but does not show up in the menu.</li>
</ol></blockquote>
<p>Works just fine here.</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="2">
<li class="remarkup-list-item">Got several "Lost annotation on document save, something went wrong" on the console. However, to prevent data loss this should show a warning in the UI and allow aborting (just show the warning dialog from below and amend the list appropriately). I'll try to add steps to reproduce as soon as I can (might be an annotation created elsewhere, i.e. already present in the document and thus should not be lost indeed).</li>
<li class="remarkup-list-item">Only looking at the format warning dialog (screenshot below), what will the <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> button do? It is confusing and potentially harmful. In addition, the wording does not maintain a reference to the original intention of saving. Suggestions for rewording: "You are about to save changes, but the <em><format></em> file format does not support saving the following elements. Please use the <em>Okular document archive</em> format to preserve them." Remove the <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> button (in case of "saving+closing" instead of only "saving", text and behaviour could just be changed to <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>, but removal would be okay too). (See also 11. and 13.)</li>
</ol></blockquote>
<p>"Continue" does what the dialog says, saves losing those changes that are on the list because they are not supported in the format you're saving. I can't remove the Continue button, otherwise you can't save the file (losing data if that is what you want, you had a big-ass dialog warning you about it)</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);"></pre></div>
<ol class="remarkup-list">
<li class="remarkup-list-item phantom-item"><ol class="remarkup-list">
<li class="remarkup-list-item">UI: Polishing (Concerning the overall user experience of the Diff and related functionality, not blocking this patch. Will file issues for those that won't/cannot be fixed in the near term.)</li>
</ol></li>
<li class="remarkup-list-item"><kbd style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">Ctrl</kbd><span class="kbd-join" style="padding: 0 4px; color: #92969D;">+</span><kbd style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">Z</kbd> to the point where the document state corresponds to the state on disk (e.g. last save point or complete undo for non-yet-saved files). The document is still marked as changed ("*" shown in title, warning dialog despite there being no changes). Instead, the state should return to "unchanged", i.e. same behaviour as in every "editor".</li>
</ol></blockquote>
<p>True, the modified status needs to be in sync with the undo/redo stack, at the moment it isn't, should be easy to fix.</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="5">
<li class="remarkup-list-item">Adding <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> to the toolbar, the button is not disabled if no changes have been made (initially, but also for undo).</li>
</ol></blockquote>
<p>I don't agree in having the Save button in the toolbar by default, the toolbar is pretty full as it is now, and while it's true that we can save, i think the majority of people using Okular just use the viewer part.</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="6">
<li class="remarkup-list-item">Show dirty state in tab title too, not only in window title (avoids surprises and clicking back and forth when there are multiple changed tabs).</li>
</ol></blockquote>
<p>Should be easy to do hopefully, will add to the todo list.</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="7">
<li class="remarkup-list-item">"Do you want to save your changes or discard them?": Would be nice to mention what changed exactly ("Huh, I did not change <em>anything</em>!") because for PDFs this might not be obvious, e.g. by using a similar dialog to the one with the list you get when <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Saving</span></span></span> a PNG, or just by changing the text. Also mention the filename. Suggestion (separate text for forms, if possible, to avoid the "and/or"): "Annotations and/or forms in the document "<filename>" have been modified. <linebreak>Do you want to save your changes or discard them?".</li>
</ol></blockquote>
<p>Not sure this is doable, you really want a list of the 185 annotations you added to the document to be listed there?</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="8">
<li class="remarkup-list-item">When closing multiple dirty tabs, the warning dialog brings the document in question to the front, which is nice. However, this might not be obvious for every user and for small window sizes especially. It should be sufficient to read the text in the dialog, instead you have to read the title of the (potentially darkened by KWin) window behind. Mention at least the filename in the dialog text, optionally use the same dialog with a checkable list of documents as <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Kate</span></span></span> already has.</li>
</ol></blockquote>
<p>I've added the file url to the text.</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>"unsupported" warning dialog:<br />
<a href="https://phabricator.kde.org/F5486097" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">F5486097: okular-unsupported-warning.png</a></p>
<ol class="remarkup-list" start="9">
<li class="remarkup-list-item">Padding below text is much larger than below list. Reduce, maybe also top-align so the icon does not look so out of place.</li>
</ol></blockquote>
<p>Very minor, will only have a look if i really have time for it.</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="10">
<li class="remarkup-list-item">Reduce default height of list if there will only ever be one or two items listed.</li>
</ol></blockquote>
<p>same</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="11">
<li class="remarkup-list-item">It would be worth thinking about not cascading the warning dialogs ("changes" + "wrong format"). Instead, directly show the second dialog, but with a <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> button.</li>
</ol></blockquote>
<p>Probably makes sense, not sure how easy is to achieve codewise though, will have a look but can't promise anything.</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>Okular document archive:</p>
<ol class="remarkup-list" start="12">
<li class="remarkup-list-item"><span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">File</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 ">Open</span></span></span> does not show "Okular document archives".</li>
</ol></blockquote>
<p>Wops, that bad, will fix</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="13">
<li class="remarkup-list-item">Inconsistent wording for ".okular" format, e.g. "Okular archive" in <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>, but "Okular document archive" in warning dialog 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> of e.g. PNG (check sources and docs for more). I'd probably prefer "Okular archive" because it is shorter (important for buttons) and "document" is not really meaningful (in general, but also for images).</li>
</ol></blockquote>
<p>Will try to unify.</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="14">
<li class="remarkup-list-item">"filename.okular" in window title, but tab still says "filename.pdf" when using <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 style="color: #92969D;"> → </span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Okular archive</span></span></span>.</li>
</ol></blockquote>
<p>Interesting, will have a look as to why that happens and try to fix it</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="15">
<li class="remarkup-list-item">No icons for ".okular" files in tab bar.</li>
</ol></blockquote>
<p>That's because we don't have an icon for .okular files. Not a new issue, .okular files have existed since ages</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>"password" warning dialog:<br />
<a href="https://phabricator.kde.org/F5486094" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">F5486094: okular-password-warning.png</a></p>
<ol class="remarkup-list" start="16">
<li class="remarkup-list-item">Grammar of "has password" is wrong, but I'd suggest some rewording anyway (e.g. to get rid of "we", "stack" and more): "The current document is protected with a password. In order to save, the file needs to be reloaded. You will be asked for the password again and your undo/redo history will be lost. Do you want to continue?"</li>
</ol></blockquote>
<p>Sounds good, changed.</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="17">
<li class="remarkup-list-item">The hyphen in the title does not really work. I'd just use "Warning" with nothing in front like in the other dialog.</li>
</ol></blockquote>
<p>I disagree, i like the "Save - Warning" construct</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="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 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="19">
<li class="remarkup-list-item">(Ideally, we would remember the password and the undo stack. I understand this is a technical limitation we currently have.)</li>
</ol></blockquote>
<p>Remembering a password is a no go (unless you're using kwallet), noone wants their password stored in cleantext in memory.</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="20">
<li class="remarkup-list-item">Note for the other new dialogs which are similar (grep for <tt style="background: #ebebeb; font-size: 13px;">i18n</tt>):<ul class="remarkup-list">
<li class="remarkup-list-item">Apply same changes.</li>
<li class="remarkup-list-item">"Continue losing changes": Sounds weird, maybe use <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> in both cases or <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>? (I'd need to see the dialog in context.)</li>
</ul></li>
</ol></blockquote>
<p>Text looks ok to me.</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><tt style="background: #ebebeb; font-size: 13px;">KMessageWidget</tt> storage migration warning:</p>
<ol class="remarkup-list" start="21">
<li class="remarkup-list-item">Assume the user closes the warning at first or by accident, but now wants it back. <kbd style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">F5</kbd> or reopening will not help, only restarting Okular brings it back. Is there any reason for this behaviour? If not, the warning should be shown everytime the document is (re)loaded. (← very minor issue, as the migration is probably not used very often)</li>
</ol></blockquote>
<p>Will check</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="22">
<li class="remarkup-list-item">Wording: Remove linebreak and replace "and continue editing the document" with "to a supported storage format, if you want to continue to edit the document."</li>
</ol></blockquote>
<p>"to a supported storage format" seems to technical, people would be scared because they don't know what that means, and in most of the cases it will hopefully it will be a pdf file and then the changes can just be saved fine wihtout the scary dialog about the okular archive format.</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<br /><strong>Cc: </strong>rkflx, lueck, mlaurent, michaelweghorn, ngraham, Okular, aacid<br /></div>