<table><tr><td style="">rkflx 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><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? 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>
<h4 class="remarkup-header">Comments on the code</h4>
<ul class="remarkup-list">
<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>
<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>
<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>
<h4 class="remarkup-header">UI: Important issues</h4>
<p>(I consider these must-haves for the RC latests, but some might break string freeze)</p>
<ol class="remarkup-list">
<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>
<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>
</ol>
<ol class="remarkup-list" start="3">
<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>
<h4 class="remarkup-header">UI: Polishing</h4>
<p>(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.)</p>
<ol class="remarkup-list" start="4">
<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>
<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>
<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>
<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>
<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>
<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>
<li class="remarkup-list-item">Reduce default height of list if there will only ever be one or two items listed.</li>
<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>
<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>
<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>
<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>
<li class="remarkup-list-item">No icons for ".okular" files in tab bar.</li>
</ol>
<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>
<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>
<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>
<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>
<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>
<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>
<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></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D8642#inline-38525" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">index.docbook:992</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <listitem>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <para><action>Saves</action> the document <span class="bright">under a new name including all the changes (annotations, form contents, &etc;), provided the document backend supports saving changes. With the &PDF; backend it is possible to save the document with the changed values of the form fields.<!--Only useful for pdf? what happens with other formats/backends?--> It can be possible (provided that the data were not secured using DRM) to save annotations with &PDF; files</span>.</para>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <note>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <para>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> Note that, due to the way this is implemented, even if there are no changes to the file, the new file need not to be an exact bit-for-bit copy of the original file (⪚ can have a different SHA-1 hash, &etc;).
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> </para>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> </note>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <para><action>Saves</action> the document <span class="bright">including all the changes (annotations, form contents, &etc;), provided the document backend supports saving those changes, if the backend does not support saving the changes the user will be give the option to lose them or to save as &okular; archive</span>.</para>
</div><div style="padding: 0 8px; margin: 0 4px; "> </listitem>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Same changes as in other comment, please.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D8642#inline-38524" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">index.docbook:1007</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <listitem>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"> <para><action>Saves</action> a copy of the original document under a new name (completely bypassing the document backend). The saved document will be a bit-for-bit copy of the original</span>.</para>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"> <para><action>Saves</action> the document under a new name including all the changes (annotations, form contents, &etc;), provided the document backend supports saving changes, if the backend does not support saving the changes the user will be give the option to lose them or to save as &okular; archive</span>.</para>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <note>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><ul class="remarkup-list">
<li class="remarkup-list-item">"if the backend": start new sentence</li>
<li class="remarkup-list-item">"the user will": add comma before</li>
<li class="remarkup-list-item">"will be give" → "will be given"</li>
<li class="remarkup-list-item">"to lose them…" → "to either discard or to save them together with the document in an &okular; archive."</li>
</ul></div></div></div></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>