<table><tr><td style="">rkflx accepted this revision.<br />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>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 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);">#0 0x00007fffbf01c156 in Poppler::UnicodeParsedString (s1=0x27000000ad) at /home/user/poppler/qt5/src/poppler-private.cc:102
#1 0x00007fffbeff8414 in Poppler::Annotation::uniqueName (this=<optimized out>) at /home/user/poppler/qt5/src/poppler-annotation.cc:1399
#2 0x00007fffbf26d81d in PDFGenerator::save(QString const&, QFlags<Okular::SaveInterface::SaveOption>, QString*) ()
from /home/user/opt/lib64/plugins/okular/generators/okularGenerator_poppler.so
#3 0x00007fffd6efd570 in Okular::Document::saveChanges(QString const&, QString*) () from /home/user/okular/build/libOkular5Core.so.7
#4 0x00007fffd727fde0 in Okular::Part::saveAs(QUrl const&, QFlags<Okular::Part::SaveAsFlag>) () from ./okularpart.so
#5 0x00007fffd727f304 in Okular::Part::saveAs(QUrl const&) () from ./okularpart.so
#6 0x00007fffd727ec62 in Okular::Part::saveFile() () from ./okularpart.so
#7 0x00007fffd7275175 in Okular::Part::setupActions()::{lambda()#1}::operator()() const () from ./okularpart.so
#8 0x00007fffd7284e50 in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, Okular::Part::setupActions()::{lambda()#1}>::call({lambda()#1}&, void**) ()
from ./okularpart.so
#9 0x00007fffd7284e31 in void QtPrivate::Functor<Okular::Part::setupActions()::{lambda()#1}, 0>::call<QtPrivate::List<>, void>({lambda()#1}&, void*, {lambda()#1}&*) ()
from ./okularpart.so
#10 0x00007fffd7284dff in QtPrivate::QFunctorSlotObject<Okular::Part::setupActions()::{lambda()#1}, 0, QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) () from ./okularpart.so
#11 0x00007ffff31ee73c in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib64/libQt5Core.so.5
#12 0x00007ffff41bf982 in QAction::triggered(bool) () from /usr/lib64/libQt5Widgets.so.5
#13 0x00007ffff41c1e1c in QAction::activate(QAction::ActionEvent) () from /usr/lib64/libQt5Widgets.so.5
#14 0x00007ffff41c2625 in QAction::event(QEvent*) () from /usr/lib64/libQt5Widgets.so.5
#15 0x00007ffff41c5afc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/lib64/libQt5Widgets.so.5
#16 0x00007ffff41cceb4 in QApplication::notify(QObject*, QEvent*) () from /usr/lib64/libQt5Widgets.so.5
#17 0x00007ffff31c1128 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /usr/lib64/libQt5Core.so.5
#18 0x00007ffff3a3e272 in QShortcutMap::dispatchEvent(QKeyEvent*) () from /usr/lib64/libQt5Gui.so.5
#19 0x00007ffff3a3e33a in QShortcutMap::tryShortcut(QKeyEvent*) () from /usr/lib64/libQt5Gui.so.5
#20 0x00007ffff39f0897 in QWindowSystemInterface::handleShortcutEvent(QWindow*, unsigned long, int, QFlags<Qt::KeyboardModifier>, unsigned int, unsigned int, unsigned int, QString const&, bool, unsigned short) () from /usr/lib64/libQt5Gui.so.5
#21 0x00007ffff3a0e9e7 in QGuiApplicationPrivate::processKeyEvent(QWindowSystemInterfacePrivate::KeyEvent*) () from /usr/lib64/libQt5Gui.so.5
#22 0x00007ffff3a136e5 in QGuiApplicationPrivate::processWindowSystemEvent(QWindowSystemInterfacePrivate::WindowSystemEvent*) () from /usr/lib64/libQt5Gui.so.5
#23 0x00007ffff39ecf9b in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/libQt5Gui.so.5
#24 0x00007fffe91fa420 in ?? () from /usr/lib64/libQt5XcbQpa.so.5
#25 0x00007fffedd16f97 in g_main_context_dispatch () from /usr/lib64/libglib-2.0.so.0
#26 0x00007fffedd171d0 in ?? () from /usr/lib64/libglib-2.0.so.0
#27 0x00007fffedd1725c in g_main_context_iteration () from /usr/lib64/libglib-2.0.so.0
#28 0x00007ffff321723f in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/libQt5Core.so.5
#29 0x00007ffff31bf73a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/libQt5Core.so.5
#30 0x00007ffff31c7fc4 in QCoreApplication::exec() () from /usr/lib64/libQt5Core.so.5
#31 0x000000000040d926 in main ()</pre></div>
<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>
<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>
<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>
<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>
<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>
<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></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;">rkflx</span> wrote in <span style="color: #4b4d51; font-weight: bold;">index.docbook:992</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Same changes as in other comment, please.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Still open.</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;">rkflx</span> wrote in <span style="color: #4b4d51; font-weight: bold;">index.docbook:1007</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><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 style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Still open.</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-38942" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">index.docbook:460</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; "> <para>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span class="bright"> Since &kde; 4.2, &okular; has the "document archiving" feature. This is an &okular;-specific format for carrying the document plus various metadata related to it (currently only annotations). You can save a "document archive" from the open document by choosing <menuchoice><guimenu>File</guimenu><guisubmenu>Export As</guisubmenu><guimenuitem>Document Archive</guimenuitem></menuchoice></span>. To open an &okular; document archive, just open it with &okular; as it would be ⪚ a &PDF; document.
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="bright"> &okular; has the "document archiving" feature. This is an &okular;-specific format for carrying the document plus various metadata related to it (currently only annotations). You can save a "document archive" from the open document by choosing <menuchoice><guimenu>File</guimenu><guisubmenu>Save As</guisubmenu></menuchoice> and selecting <guilabel>Okular Archive</guilabel> in the <guilabel>Filter</guilabel> selector</span>. To open an &okular; document archive, just open it with &okular; as it would be ⪚ a &PDF; document.
</div><div style="padding: 0 8px; margin: 0 4px; "> </para>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">"Okular Archive" → "Okular document archive"</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-38943" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">part.cpp:2485</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; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span style="color: #aa4000">int</span> <span class="n">res</span> <span style="color: #aa2211">=</span> <span class="n">KMessageBox</span><span style="color: #aa2211">::</span><span class="n">warningYesNo</span><span class="p">(</span> <span class="n">widget</span><span class="p">(),</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">i18n</span><span class="p">(</span> <span style="color: #766510">"The current document format backend doesn't support internal reload on save so we will close and open the file again.<br />This means that the undo/redo stack will be lost.<br />Do you want to continue?"</span> <span class="p">),</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">i18n</span><span class="p">(</span> <span style="color: #766510">"Save - Warning"</span> <span class="p">)</span> <span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">As mentioned before, fix "we" and "stack" here too, also remove internal details (confusing users, but also often resulting in strange translations), e.g. replace with:</p>
<p style="padding: 0; margin: 8px;">"After saving, the current document format requires the file to be reloaded. Your undo/redo history will be lost.<br />Do you want to continue?"</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-38938" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">part.cpp:2518</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; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span class="n">warningMessage</span> <span style="color: #aa2211">=</span> <span class="n">m_document</span><span style="color: #aa2211">-></span><span class="n">canSwapBackingFile</span><span class="p">()</span> <span style="color: #aa2211">?</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">i18n</span><span class="p">(</span> <span style="color: #766510">"You are about to save changes, but the current file format does not support saving the following elements. Please use the <i>Okular document archive</i> format to preserve them. Click <i>Continue</i> to save ignoring these elements."</span> <span class="p">)</span> <span style="color: #aa2211">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">i18n</span><span class="p">(</span> <span style="color: #766510">"You are about to save changes, but the current file format does not support saving the following elements. Please use the <i>Okular document archive</i> format to preserve them. Click <i>Continue</i> to save but you will lose these elements (as well as the undo/redo history)."</span> <span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">"to save ignoring" → "to save the document and discard"</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-38937" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">part.cpp:2519</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; background: rgba(151, 234, 151, .6);"> <span class="n">i18n</span><span class="p">(</span> <span style="color: #766510">"You are about to save changes, but the current file format does not support saving the following elements. Please use the <i>Okular document archive</i> format to preserve them. Click <i>Continue</i> to save ignoring these elements."</span> <span class="p">)</span> <span style="color: #aa2211">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">i18n</span><span class="p">(</span> <span style="color: #766510">"You are about to save changes, but the current file format does not support saving the following elements. Please use the <i>Okular document archive</i> format to preserve them. Click <i>Continue</i> to save but you will lose these elements (as well as the undo/redo history)."</span> <span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span class="n">continueMessage</span> <span style="color: #aa2211">=</span> <span class="n">m_document</span><span style="color: #aa2211">-></span><span class="n">canSwapBackingFile</span><span class="p">()</span> <span style="color: #aa2211">?</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Comma before second "but", remove parentheses.</p></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, rkflx<br /><strong>Cc: </strong>rkflx, lueck, mlaurent, michaelweghorn, ngraham, Okular, aacid<br /></div>