rkflx accepted this revision. rkflx added a comment.
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. 24. Segfault: Add annotation, Save > Undo > Save. Introduced by https://phabricator.kde.org/R223:055f2db76d58b559b8a215cd5713cdbd7265fddd. Backtrace: #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 () 25. `parttest` still fails (now requires to press button in dialog, then crashes with similar backtrace to 24.) 26. 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 `.fileName()`. Regarding the file format warning: - I now understand why we need Continue for Save As. - 27. It is still very confusing when using Save, because for the user Continue and Cancel are exactly the same. Can we special-case with a flag and hide the last sentence and the button here? As Save is the more common operation, this would streamline the user experience massively and reduce frustration. Most users won't read the text and assume Save > Continue //will// actually save the changes. - 28. There is an inconsistency for Close: If a document `canSwapBackingFile` (e.g. for PDF), Continue will keep the document open, but if it cannot (e.g. for epub), Continue losing changes will close the document. I think Discard in the previous dialog is enough to //actually// close, so for Save (the only case triggered by closing) the Continue [l.c.] buttons can be removed, really. - (Note I assume here that all formats support saving either all or no changes, but not only some. Is this true?) 29. See inline comments. >> 18. Improve buttons to Continue and Cancel. > > Disagreed, Continue might make sense, but why "Cancel"? Cancel is not an answer to "Do you want to continue?" Got a nice idea for a very smooth reading dialog: Just remove the question, then you can change the buttons. INLINE COMMENTS > rkflx wrote in index.docbook:992 > Same changes as in other comment, please. Still open. > rkflx wrote in index.docbook:1007 > - "if the backend": start new sentence > - "the user will": add comma before > - "will be give" → "will be given" > - "to lose them…" → "to either discard or to save them together with the > document in an &okular; archive." Still open. > index.docbook:460 > <para> > - 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>. To > open an &okular; document archive, just open it with &okular; as it would be > ⪚ a &PDF; document. > + &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. To open an > &okular; document archive, just open it with &okular; as it would be ⪚ a > &PDF; document. > </para> "Okular Archive" → "Okular document archive" > part.cpp:2485 > + const int res = KMessageBox::warningYesNo( widget(), > + i18n( "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?" ), > + i18n( "Save - Warning" ) ); 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: "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?" > part.cpp:2518 > + const QString warningMessage = m_document->canSwapBackingFile() ? > + i18n( "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." ) : > + i18n( "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)." ); "to save ignoring" → "to save the document and discard" > part.cpp:2519 > + i18n( "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." ) : > + i18n( "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)." ); > + const QString continueMessage = m_document->canSwapBackingFile() > ? Comma before second "but", remove parentheses. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D8642 To: aacid, mlaurent, rkflx Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid
