Changes are no longer saved as part of the local configuration files, they can be either saved back to file, to an .okular file (if the file format doesn't support annotations) or not saved at all.
You are probably better having a look at each of the changes individually of
git log origin/master...origin/dont-use-docdata-for-annots-and-forms
| Lint Skipped |
| Unit Tests Skipped |
| core/document.cpp | ||
|---|---|---|
| 4388 | qCWarning(...) | |
| 4444 | Remove space before url | |
| 4494 | qCWarnign(...) | |
| 4729 | coding style : remove space after & | |
| core/document.h | ||
| 758 | 4.14? | |
| 771 | same | |
| 773 | coding style: Remove space before url | |
| 865 | kde 4.20 ? | |
| core/documentcommands.cpp | ||
| 732 | if button is null do you think that we need to add to QList ? | |
| part.cpp | ||
| 566 | You can still use | |
| 817 | You can use new connect api too | |
| 2476 | *copyJob = nullptr; to be sure that it's initialized | |
| ui/annotationmodel.cpp | ||
| 113 | qCWarning(...) | |
| ui/formwidgets.cpp | ||
| 89 | qCWarning(...) | |
Tried all steps in the TEST PLAN:
But the second button in the dialog "Close Document" is labeled "Discard" or "No", depending how I close:
Using window Close button first the second button is labeled "No"
Using File->Close (Ctrl+W) the second button is labeled "Discard"
Once I have used File->Close and selected the button "Cancel" in this dialog
the dialog opened with the window Close button has the second button labeled "Discard" now instead of "No"
Same issue with the label of the second button (Discard/No) in the dialog Close Document as in 1)
The dialog Warning with the option to save as document archive has a field with
one entry "User annotations", what does it mean?
What does the second option "Continue" in this dialog do, I see no difference to "Cancel"?
Open pdf file, add anotation, close app
You get dialog about losing changes, check that save, discard, cancel all do what they say
This works as described. One additional thought on that:
Some users may want to save the modified version to a new file instead of overwriting the existing one. While this is already possible via "File" -> "Save As" (or Ctrl+Shift+S), I think it might be helpful to add an additional button "Save as" in that dialog as well. This would also help avoid users accidently overwriting the original file. (Evince, for example only offers a "Save As" option and users switching between the two programs might not be aware of the difference at once.)
Alternatively (or in addition to that), clarifying the question in the dialog to something like "Do you want to save your changes TO THE DOCUMENT <FILENAME> or discard them?" (capital letters only here to indicate what has been added) might possibly be an option as well.
Another great feature \o/. Good news, I cannot reproduce or even describe the single crash I got and there are only small issues to be fixed before this can land (IMHO). The rest is optional polishing. (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.)
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?).
(I consider these must-haves for the RC latests, but some might break string freeze)
(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.)
"unsupported" warning dialog:
Okular document archive:
"password" warning dialog:
KMessageWidget storage migration warning:
| doc/index.docbook | ||
|---|---|---|
| 997 | Same changes as in other comment, please. | |
| 1007 |
| |
Hmh, cannot reproduce. Did you run make install? Special env? Exact steps to open the document?
The dialog Warning with the option to save as document archive has a field with
one entry "User annotations", what does it mean?
I agree this might lead to confusion. Not sure what different unsupported features may appear in the list and how this depends on the formats (only spotted annotations and forms in the code), but perhaps it is sufficient to just mention annotations and forms in general in the text and get rid of the cryptic list?
I'd prefer the second option, as upgrading to four buttons just for this special case is kind of ugly. If it works like an editor, it should be consistent in that regard, too.
Most of my final checks passed, there is one more serious problem though:
- 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).
Steps to reproduce:
Given no actual data was lost, I'd not consider this critical anymore. However, it is quite scary and should be fixed.
The same behaviour can be seen when Okular opens autotests/data/file1.pdf annotated on Android with "Adobe Acrobat" or "MuPDF".
Edit: To clarify, I assume this is fixable in the backend, no UI warning necessary as originally proposed.
| autotests/documenttest.cpp | ||
|---|---|---|
| 112 | delete added. | |
| core/document.cpp | ||
| 4388 | I don't agree, this is something that won't be triggered unless something really wrong happened, thus i don't want it hidden by a qCWarning | |
| 4494 | Same as above, don't agree. | |
| 4729 | no, that's the original okular style, as you can see on the diff it was exactly like this already. | |
| core/document.h | ||
| 744 | The branch was old ;) | |
| core/documentcommands.cpp | ||
| 732 | Yes and no, we need to detect error here, i'll work on an improvement. | |
| ui/annotationmodel.cpp | ||
| 113 | again i don't agree with the qcwarning here. | |
| ui/formwidgets.cpp | ||
| 89 | disagree again | |
I can't reproduce that :(
Can anyone reproduce it?
- Open png file, add anotation, close app... -> Works as expected Same issue with the label of the second button (Discard/No) in the dialog Close Document as in 1)
The dialog Warning with the option to save as document archive has a field with
one entry "User annotations", what does it mean?
What does the second option "Continue" in this dialog do, I see no difference to "Cancel"?
It says that you can "Continue" saving, but if you continue the "User annotations" will be lost. I guess the fact that you didn't understand it means it needs a rewording, anything you would suggest to make it more easy to understand?
- With old version of okular, add annotation to pdf file... -> Works as expected
- Open pdf file with forms... ->Works as expected
- Open png file add an annotation and save it as okular archive... ->Works as expected
I disagree, does libreoffice suggest "Save As" when you do open an existing file, do some changes and close the app? Do you know of any program that does offer "Save As" in that situation?
Alternatively (or in addition to that), clarifying the question in the dialog to something like "Do you want to save your changes TO THE DOCUMENT <FILENAME> or discard them?" (capital letters only here to indicate what has been added) might possibly be an option as well.
That makes sense and is again what libreoffice does for example, will add.
As mentioned, Evince does. Apart from that, I am currently not aware of another program that does similarly. I don't consider the "Save as" button necessary if the clarification as mentioned below is implemented.
Alternatively (or in addition to that), clarifying the question in the dialog to something like "Do you want to save your changes TO THE DOCUMENT <FILENAME> or discard them?" (capital letters only here to indicate what has been added) might possibly be an option as well.
That makes sense and is again what libreoffice does for example, will add.
Thanks! LibreOffice is actually from where I adapted the idea. :-)
Yes either manually or as part of the merge commit
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?).
Yes, merge is the correct way.
- Comments on the code
- parttest fails (missing potato.jpg)
Wops, added.
- see Laurent's comments
- more @since wrong (e.g. TODO, 0.21, …)
- Looked only briefly at the individual commits, it's just too much code to analyze in depth (i.e. "I trust you on the const_cast" and the tests you added :). In general the swapBackingFile mechanism seems to add a huge amount of complexity, but I guess if there was an easier path you would have followed it.
- Did you run this with ASAN and friends to check for any problems?
Yes
- There is some code duplication for non-trivial amounts of code (grep for loadSyncFile).
You mean in swapBackingFile and swapBackingFileArchive ? I'll try to merge them.
- UI: Important issues (I consider these must-haves for the RC latests, but some might break string freeze)
- Ctrl+S saves and is visible in Configure Shortcuts, but does not show up in the menu.
Works just fine here.
- 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).
- Only looking at the format warning dialog (screenshot below), what will the Continue 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 <format> file format does not support saving the following elements. Please use the Okular document archive format to preserve them." Remove the Continue button (in case of "saving+closing" instead of only "saving", text and behaviour could just be changed to Discard, but removal would be okay too). (See also 11. and 13.)
"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)
- 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.)
- Ctrl+Z 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".
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.
- Adding Save to the toolbar, the button is not disabled if no changes have been made (initially, but also for undo).
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.
- 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).
Should be easy to do hopefully, will add to the todo list.
- "Do you want to save your changes or discard them?": Would be nice to mention what changed exactly ("Huh, I did not change anything!") because for PDFs this might not be obvious, e.g. by using a similar dialog to the one with the list you get when Saving 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?".
Not sure this is doable, you really want a list of the 185 annotations you added to the document to be listed there?
- 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 Kate already has.
I've added the file url to the text.
"unsupported" warning dialog:
- Padding below text is much larger than below list. Reduce, maybe also top-align so the icon does not look so out of place.
Very minor, will only have a look if i really have time for it.
- Reduce default height of list if there will only ever be one or two items listed.
same
- It would be worth thinking about not cascading the warning dialogs ("changes" + "wrong format"). Instead, directly show the second dialog, but with a Discard button.
Probably makes sense, not sure how easy is to achieve codewise though, will have a look but can't promise anything.
Okular document archive:
- File → Open does not show "Okular document archives".
Wops, that bad, will fix
- Inconsistent wording for ".okular" format, e.g. "Okular archive" in Save As, but "Okular document archive" in warning dialog for Save 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).
Will try to unify.
- "filename.okular" in window title, but tab still says "filename.pdf" when using Save As → Okular archive.
Interesting, will have a look as to why that happens and try to fix it
- No icons for ".okular" files in tab bar.
That's because we don't have an icon for .okular files. Not a new issue, .okular files have existed since ages
"password" warning dialog:
- 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?"
Sounds good, changed.
- The hyphen in the title does not really work. I'd just use "Warning" with nothing in front like in the other dialog.
I disagree, i like the "Save - Warning" construct
- 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?"
- (Ideally, we would remember the password and the undo stack. I understand this is a technical limitation we currently have.)
Remembering a password is a no go (unless you're using kwallet), noone wants their password stored in cleantext in memory.
- Note for the other new dialogs which are similar (grep for i18n):
- Apply same changes.
- "Continue losing changes": Sounds weird, maybe use Continue in both cases or Discard? (I'd need to see the dialog in context.)
Text looks ok to me.
KMessageWidget storage migration warning:
- Assume the user closes the warning at first or by accident, but now wants it back. F5 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)
Will check
- 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."
"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.
I can't reproduce this, what do you mean by change file externally exactly? touch? edit it? remove it? copy something else over? ( i tried touch and my awesome pdf editing skills with vim)
To clarify, in the File menu, I only see Save As, but no entry for Save. Any special trick to make it show up? (I just used make install to ~/somedir and set the paths as outlined here.)
- Adding Save to the toolbar, the button is not disabled if no changes have been made (initially, but also for undo).
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.
I fully agree, but that's not what I meant. "Adding" was about "When I manually add the button to the toolbar", otherwise I would have written "Please add <…>, because <reasons>". You may have missed the main part of the sentence, which was about the (non)disabled state.
- "Do you want to save your changes or discard them?": Would be nice to mention what changed exactly ("Huh, I did not change anything!") because for PDFs this might not be obvious, e.g. by using a similar dialog to the one with the list you get when Saving 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?".
Not sure this is doable, you really want a list of the 185 annotations you added to the document to be listed there?
Good point. Just use my wording suggestion then (which does not imply having the list).
- 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."
"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.
Okay, my main motivation was more about rewording the last part which I stumbled upon. Could you change this to ", if you want to continue to edit the document."?
Cannot check right now, IIRC this was with both latexmk and cp somefile.pdf file_opened_in_okular.pdf (comment if you still cannot reproduce and I will check in the evening). That's obviously not part of a regular workflow, but could happen by accident in reality.
Ah, part.rc needs a version increase. Done in the branch.
- Adding Save to the toolbar, the button is not disabled if no changes have been made (initially, but also for undo).
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.
I fully agree, but that's not what I meant. "Adding" was about "When I manually add the button to the toolbar", otherwise I would have written "Please add <…>, because <reasons>". You may have missed the main part of the sentence, which was about the (non)disabled state.
Ah, you want save to be only enabled when the file has been modified? I don't see this being a common pattern for the Save action , e.g. kate doesn't do that, libreoffice doesn't do that.
- "Do you want to save your changes or discard them?": Would be nice to mention what changed exactly ("Huh, I did not change anything!") because for PDFs this might not be obvious, e.g. by using a similar dialog to the one with the list you get when Saving 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?".
Not sure this is doable, you really want a list of the 185 annotations you added to the document to be listed there?
Good point. Just use my wording suggestion then (which does not imply having the list).
Sincerely i don't see the need of saying "Annotations and/or forms", users are not that stupid. As mentioned in other comments i've already added the filename to the text.
- 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."
"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.
Okay, my main motivation was more about rewording the last part which I stumbled upon. Could you change this to ", if you want to continue to edit the document."?
Changed.
Cannot check right now, IIRC this was with both latexmk and cp somefile.pdf file_opened_in_okular.pdf (comment if you still cannot reproduce and I will check in the evening). That's obviously not part of a regular workflow, but could happen by accident in reality.
Yes, cp has this problem. This problem is on the other hand not new, if with the "old" version of okular you do the same, you will have exactly the same problem. I will try to fix it since you're right you can get data loss but it's nothing new and thus i don't think we should block the landing of this feature because of that.
Ah, you want save to be only enabled when the file has been modified? I don't see this being a common pattern for the Save action , e.g. kate doesn't do that, libreoffice doesn't do that.
Indeed, I missed this (still consider it a bug, though). LibreOffice used to do that, but they had to change that (and caused some uproar) when changing the button to a split button (i.e. with a menu below). In general disabling is a common pattern, I remember it from older versions of MS Office. But I agree, let's not change it for Okular. If there is support to change that globally, it should be done in a coordinated manner someday.
Yes, cp has this problem. This problem is on the other hand not new, if with the "old" version of okular you do the same, you will have exactly the same problem.
The old Okular only had Save As, and as this works fine with the newer Okular I assume it worked fine too with the older? In any case, I agree that's not a blocker.
Thanks for fixing a lot of the problems so far, let me know if/when I should verify the fixes or test again before this goes in (and by when you'd want me to do this).
"You are about to save changes, but the <format> file format does not support saving the following elements. Please use the Okular document archive format to preserve them . Click Continue to save but you will loose these elements."
No, it is broken too, Save As is not a fix for this, the problem is that you replace a file A with file B and the poppler code doesn't realize and when saving does an incremental update as if the file was still A but now is B and everything breaks.
Thanks for fixing a lot of the problems so far, let me know if/when I should verify the fixes or test again before this goes in (and by when you'd want me to do this).
On the branch (not put here in this fake diff) i've also fixed:
Still on the todo list:
The todo list is still a bit long but most of the changes would be hopefully not big (and imho it could really land as it is now) so if you could have a look as soon as possible (use the branch) would be great
This is now done.
- Fixing the code in documentcommands.cpp suggested by Laurent
This is now done.
- Bringing back the messagewidget on reload
There's an issue with the KMessageWidget API, calling setVisible after it has been closed doesn't really make it visible again, i'll try to fix have a look but at this point I don't think we should block on this.
- Fixing duplicated code in swapBackingFile[Archive]
This is now done
- Trying to have one warning dialog instead of two
Given how close we are to the deadline and how tricky is to get all the if/else right i propose we live this for later
- padding improvements on the unsupported warning dialog
This is a KMessageBox::warningYesNoCancelList nothing i can do to fix it in okular
- Poppler fix so that saving doesn't fail if the file is changed behind our feet
This is ortogonal to this request, i'll work on it but again i dont' think we should block on it
At this point it would be great if we could get it merged on thursday night.
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.
#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 ()Regarding the file format warning:
- 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.
| doc/index.docbook | ||
|---|---|---|
| 460 | "Okular Archive" → "Okular document archive" | |
| 997 | Still open. | |
| 1007 | Still open. | |
| part.cpp | ||
| 2485 | 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?" | |
| 2518 | "to save ignoring" → "to save the document and discard" | |
| 2519 | Comma before second "but", remove parentheses. | |
One more thing: Please also grep the docbook for &okular; archive, still some document missing.
I can't seem to reproduce this. Which file and which annotation are you using?
- parttest still fails (now requires to press button in dialog, then crashes with similar backtrace to 24.)
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)
- 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().
Are you sure we want the filename only? what about if you have two files with the same filename and different paths?
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.
Makes sense, the code is relatively complicated though, will see what i can get.
- 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.
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.
- (Note I assume here that all formats support saving either all or no changes, but not only some. Is this true?)
At this point yes
- See inline comments.
- 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.
I understand you disagree with the current wording, but honestly i like it more than your suggestion.
I can't seem to reproduce this. Which file and which annotation are you using?
IIRC, I did Open autotests/data/file1.pdf, used pen tool, Ctrl+S+Z+S.
Are you sure we want the filename only? what about if you have two files with the same filename and different paths?
Let's optimize for the common case here, i.e. not the above. As mentioned, the tab titles already show the filename only, we'd be consistent with other apps and switching to the tab to be closed when closing the window is good enough. So yes, I'd prefer that.
You mean PNG and not PDF, right?
I do, sorry for the confusion. Okular's file format support is just too complicated ;)
I started with a cursory glance at the changes, but the change set is huge, which makes it really hard to review, especially for people who have no extensive prior experience with okular. I think some of my comments could be "solved" by adding a comment here or there, as it seems like they are non-issues but rather arise from misunderstanding or lack of information from my side. Help future people looking at this code by writing some more comments. And consider splitting up such monster commits in the future into smaller patches, one building on the other - if possible. Or is this the case already? I.e. could I instead review a feature branch? I know that phabricator sucks in that regard, but at least it would simplify my life at reviewing, even if I'd read patches on the command line and then add comments here
| autotests/documenttest.cpp | ||
|---|---|---|
| 105 | can we test the actual migration, too? i.e. instead of pretending, actually do it? | |
| 112 | shouldn't this have an QEXECTED_FAIL, considering that in principle the annotations should have been migrated? | |
| autotests/parttest.cpp | ||
| 827 | this overrides the helper from above, is this desired? do you need a stack of helpers? or do you maybe miss a wait step before (signal spy?) to ensure the previous dialog has been shown and closed? | |
| 854 | dito, this again potentially overrides the close helper, no? | |
| core/document.cpp | ||
| 1162 | missing "if" | |
| 1170 | missing "in" | |
| core/generator.h | ||
| 296 | whitespace issue after & | |
| core/page.cpp | ||
| 502 | ws issues like above | |
| core/page.h | ||
| 256 | whitespace issue after & | |
https://phabricator.kde.org/source/okular/history/dont-use-docdata-for-annots-and-forms/
Or rather as written in the summary:
You are probably better having a look at each of the changes individually of git log origin/master...origin/dont-use-docdata-for-annots-and-forms
(seems Phab eats parts of the reply when using the mail interface…)
Ok, i can reproduce it now, the ASAN enabled build for some reason was not crashing nor giving me any warning. Will [try to] fix
| autotests/documenttest.cpp | ||
|---|---|---|
| 105 | Should be doable, i'll have a look. | |
| 112 | i don't understand why we would use QEXECTED_FAIL here, yes the annotations have been migrated so we compare docdata migration to false, at most we could use a qverify since one could argue qcompare with bools is weird, but why qexpected_fail? | |
| autotests/parttest.cpp | ||
| 827 | Yes it overrides the helper from above. Yes it is desired since the helper from above was already used in saveAs No i don't need a stack of helpers No i don't need a wait step since the helper destructor already ensures the previous dialog was shown and closed | |
| 854 | Same answer as above, this does exactly what it has to do. | |
| core/generator.h | ||
| 296 | You don't really want to look at all the whitespace issues the okular code has. | |
| core/page.cpp | ||
| 502 | No, this is actually the okular style (most of the time) | |
| core/page.h | ||
| 256 | don't complain about whitespace issue, there's too many styles to "fix" it. | |
| autotests/documenttest.cpp | ||
|---|---|---|
| 112 | phabricator UI issue: the comment is for the line above, i.e. there are now no more annotations. Before, there was one, which is why I wonder | |
| autotests/documenttest.cpp | ||
|---|---|---|
| 112 | Ok, this is because we "fake migrated" the file, so the xml describing the annotations is gone, but since we didn't actually save the file, the annotation doesn't exist anymore. | |
| autotests/documenttest.cpp | ||
|---|---|---|
| 105 | Done in the branch. | |
- 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.
Done, even though it adds some code "duplication" its easier to have it that way than to add lots of more if branches for smaller code duplication
- 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.
So this actually got fixed by the above change too, given that now you don't have Continue anymore on Close+Save this can't happen anymore 😸
Since it happens almost everyone is relatively happy i'll be commiting this to the Applications/17.12 branch in around 6 hours to make it in time for the freeze later tonight.