aacid closed this revision.
aacid added a reviewer: mlaurent.
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8642
To: aacid, rkflx, mlaurent
Cc: mwolff, rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid
aacid removed a reviewer: mlaurent.
This revision is now accepted and ready to land.
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8642
To: aacid, rkflx
Cc: mwolff, rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid
aacid added a comment.
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.
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8642
To: aacid, mla
aacid marked an inline comment as done.
aacid added a comment.
> - 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
operati
aacid marked 2 inline comments as done.
aacid added inline comments.
INLINE COMMENTS
> aacid wrote in documenttest.cpp:105
> Should be doable, i'll have a look.
Done in the branch.
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8642
To: aacid, mlaurent, rkflx
Cc: mwol
aacid added a comment.
In https://phabricator.kde.org/D8642#167919, @aacid wrote:
> In https://phabricator.kde.org/D8642#167903, @rkflx wrote:
>
> > > I can't seem to reproduce this. Which file and which annotation are you
using?
> >
> > IIRC, I did Open `autotests/data/file1.pdf`
aacid marked 5 inline comments as done.
aacid added inline comments.
INLINE COMMENTS
> mwolff wrote in 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
Ok, this is because we "f
mwolff added inline comments.
INLINE COMMENTS
> aacid wrote in documenttest.cpp: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
aacid marked 7 inline comments as done.
aacid added inline comments.
INLINE COMMENTS
> mwolff wrote in documenttest.cpp:105
> can we test the actual migration, too? i.e. instead of pretending, actually
> do it?
Should be doable, i'll have a look.
> mwolff wrote in documenttest.cpp:112
> should
aacid added a comment.
In https://phabricator.kde.org/D8642#167903, @rkflx wrote:
> > 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].
Ok, i can reproduce i
rkflx added a comment.
https://phabricator.kde.org/source/okular/history/dont-use-docdata-for-annots-and-forms/
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8642
To: aacid, mlaurent, rkflx
Cc: mwolff, rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aacid
mwolff added a comment.
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,
rkflx added a comment.
> 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, [S] + [Z] + [S].
> Are you sure we want the filename only? what about if you have two files
with the same filename and
aacid marked 8 inline comments as done.
aacid added a comment.
In https://phabricator.kde.org/D8642#167826, @rkflx wrote:
> 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
aacid added a comment.
In https://phabricator.kde.org/D8642#167837, @rkflx wrote:
> One more thing: Please also grep the docbook for `&okular; archive`, still
some `document` missing.
Fixed too
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8642
To: aac
rkflx added a comment.
One more thing: Please also grep the docbook for `&okular; archive`, still
some `document` missing.
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8642
To: aacid, mlaurent, rkflx
Cc: rkflx, lueck, mlaurent, michaelweghorn, ngraham, #okular, aa
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 c
aacid updated this revision to Diff 22346.
REPOSITORY
R223 Okular
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D8642?vs=21843&id=22346
REVISION DETAIL
https://phabricator.kde.org/D8642
AFFECTED FILES
autotests/data/file1-docdata.xml
autotests/data/potato.jpg
autotests/docum
aacid added a comment.
In https://phabricator.kde.org/D8642#167371, @aacid wrote:
> Still on the todo list:
>
> - syncing the modified bit to the undo/redo stack so if you add an
annotation and then undo it, it's not marked as modified
This is now done.
> - Fixing the cod
aacid added a comment.
In https://phabricator.kde.org/D8642#167301, @lueck wrote:
> In https://phabricator.kde.org/D8642#167017, @aacid wrote:
>
> > In https://phabricator.kde.org/D8642#164520, @lueck wrote:
> >
> > > In https://phabricator.kde.org/D8642#164036, @aacid wrote:
> >
aacid added a comment.
In https://phabricator.kde.org/D8642#167178, @rkflx wrote:
> > 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
lueck added a comment.
In https://phabricator.kde.org/D8642#167017, @aacid wrote:
> In https://phabricator.kde.org/D8642#164520, @lueck wrote:
>
> > In https://phabricator.kde.org/D8642#164036, @aacid wrote:
> >
> > > Note KDE Applications 17.12 is Nov 16, it would be great if we c
lueck added a comment.
In https://phabricator.kde.org/D8642#167017, @aacid wrote:
> In https://phabricator.kde.org/D8642#164520, @lueck wrote:
>
> > In https://phabricator.kde.org/D8642#164036, @aacid wrote:
> >
> > > Note KDE Applications 17.12 is Nov 16, it would be great if we c
rkflx added a comment.
> 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 t
aacid added a comment.
In https://phabricator.kde.org/D8642#167049, @rkflx wrote:
> In https://phabricator.kde.org/D8642#167025, @aacid wrote:
>
> > > 1. [Ctrl] + [S] saves and is visible in Configure Shortcuts, but does
not show up in the menu.
> >
> > Works just fine here.
>
aacid added a comment.
In https://phabricator.kde.org/D8642#166482, @rkflx wrote:
> > 2. 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 dialo
rkflx added a comment.
In https://phabricator.kde.org/D8642#167025, @aacid wrote:
> > 1. [Ctrl] + [S] saves and is visible in Configure Shortcuts, but does not
show up in the menu.
>
> Works just fine here.
To clarify, in the File menu, I only see Save As, but no entry for Sav
aacid added a comment.
In https://phabricator.kde.org/D8642#166462, @rkflx wrote:
> Most of my final checks passed, there is one more serious problem though:
>
> 23. Data loss on external change despite trying to save: Open, add
annotation, change file externally, wait for warning, Sa
aacid added a comment.
In https://phabricator.kde.org/D8642#166340, @rkflx wrote:
> 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.*
michaelweghorn added a comment.
In https://phabricator.kde.org/D8642#167020, @aacid wrote:
> In https://phabricator.kde.org/D8642#165611, @michaelweghorn wrote:
>
> > > Open pdf file, add anotation, close app
> > > You get dialog about losing changes, check that save, discard, cance
aacid added a comment.
In https://phabricator.kde.org/D8642#165611, @michaelweghorn wrote:
> > 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
aacid marked 3 inline comments as done.
aacid added a comment.
In https://phabricator.kde.org/D8642#164520, @lueck wrote:
> In https://phabricator.kde.org/D8642#164036, @aacid wrote:
>
> > Note KDE Applications 17.12 is Nov 16, it would be great if we could get
this in, so please try
aacid marked 12 inline comments as done.
aacid added inline comments.
INLINE COMMENTS
> mlaurent wrote in documenttest.cpp:112
> closeDocument delete m_document with a deleteLater or we need to delete it
> before finish method ?
delete added.
> mlaurent wrote in document.cpp:4388
> qCWarning(.
rkflx added a comment.
> 2. 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 s
rkflx added a comment.
Most of my final checks passed, there is one more serious problem though:
23. Data loss on external change despite trying to save: Open, add
annotation, change file externally, wait for warning, Save. Instead of
overwriting, there is an error in the UI ("Could not
rkflx added a comment.
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 rev
rkflx added a comment.
In https://phabricator.kde.org/D8642#164520, @lueck wrote:
> 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
michaelweghorn added a comment.
> 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 fi
lueck added a comment.
In https://phabricator.kde.org/D8642#164036, @aacid wrote:
> Note KDE Applications 17.12 is Nov 16, it would be great if we could get
this in, so please try to review ASAP :)
Tried all steps in the TEST PLAN:
1. Open pdf file, add anotation, close app..
mlaurent requested changes to this revision.
mlaurent added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> documenttest.cpp:112
> +QCOMPARE( m_document->isDocdataMigrationNeeded(), false );
> +m_document->closeDocument();
> +}
closeDocument delete m_doc
aacid added a comment.
Note KDE Applications 17.12 is Nov 16, it would be great if we could get this
in, so please try to review ASAP :)
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D8642
To: aacid
Cc: #okular, aacid
aacid created this revision.
aacid added a project: Okular.
Restricted Application added a subscriber: Okular.
REVISION SUMMARY
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 an
42 matches
Mail list logo
