How does this relate to https://bugs.kde.org/show_bug.cgi?id=382011 ?

Only used bugzilla's quicksearch which omits NEEDSINFO bugs, so I missed this (sorry). There doesn't seem do be any new information though. I'll add a link to this review.

When implementing the patch, I had to decide between the version in the texlive svn and several branches of the github repo. Both are roughly equivalent to the github state if you look at bugfixes and ignore new feature development after TeXLive 2017 (in terms of code, as the actual commit history is totally different). I like that development is opening up, but there is also confusion about branches (see issue #3) and synchronisation with the svn. In the end, distros are shipping from svn, so I used that.

As mentioned in the summary, we could think about our options for the longterm future. For 17.12 I would suggest to go with the pragmatic approach as outlined in this review request, except you are able to invest much time in a better solution right now. The work of going through the old patches and local commits without patches was needed anyway.

@mludwig As Kile is also providing synctex functionality, maybe you are interested in looking at this patch, too. In my rather simple tests Kile worked fine, but you might be more familiar how it is supposed to work.

Is it possible that a memory leak sneaked in in this code?

Without this code if i run

./autotests/editannotationcontentstest

i don't get any memory leak but with it i get

5: Direct leak of 131076 byte(s) in 4 object(s) allocated from:
5: #0 0x7fcf0e682ec0 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.3+0xc6ec0)
5: #1 0x7fcf0daeb869 in _synctex_malloc /home/kdeunstable/okular/core/synctex/synctex_parser_utils.c:73
5: #2 0x7fcf0dadb607 in synctex_reader_init_with_output_file /home/kdeunstable/okular/core/synctex/synctex_parser.c:759
5: #3 0x7fcf0dae6295 in synctex_scanner_new_with_output_file /home/kdeunstable/okular/core/synctex/synctex_parser.c:5837
5: #4 0x7fcf0d9ed585 in Okular::Document::openDocument(QString const&, QUrl const&, QMimeType const&, QString const&) /home/kdeunstable/okular/core/document.cpp:2413
5: #5 0x7fcf0dead04f in Okular::Part::doOpenFile(QMimeType const&, QString const&, bool*) /home/kdeunstable/okular/part.cpp:1295
5: #6 0x7fcf0deae866 in Okular::Part::openFile() /home/kdeunstable/okular/part.cpp:1417

In D7594#141938, @rkflx wrote:

When I run the command you specified, I only get QtTest output. Did you run cmake with -DECM_ENABLE_SANITIZERS=leak?

address instead of clang, but the output should be the same

Did you use clang?

No, gcc

(I'm asking, because the test is leaking for me with both valgrind and liblsan even without the synctex update. You said you didn't get any leaks before, so I'd be interested in the exact tools, options and suppressions you used.)

What i meant it's that it's a new leak

I also do not get the 5: in my output, so if this indicates a larger set of linting tests, I'd be happy if you could sketch out your setup so that I could improve my linting in the future.

Once this confusion is cleared up, I will try to look into the synctex leak (which seems real, thanks for finding) as soon as time permits.

I don't know where that 5: came from, i guess first it copy&pasted it somewhere esle?

This looks good to me in a "i've no idea what this does but tests still pass" way.

Ideally one would get this into KDE Applications 17.08.x but given it's a big ish change, my suggestion is get it into master for KDE Applications 17.12.0 and this way people that use unstable verisons have more time to try it.

What do you think?

Thanks, targeting master matches exactly with my own impact assessment and the FIXED-IN set in the summary.

I guess bugs in synctex for particular documents should go upstream, so if you are happy with my integration work, please "accept" it from the Okular side.

I'll probably wait with landing until next week in order to give the other reviewers and the bug reporters more time for comments, but so far there were none.

This broke the Windows build

I know, that's why I started setting up a windows machine already yesterday (it has still not finished installing and compiling, though). Basically we need -lshlwapi, but I'm not sure if that's enough, so I want to double check. Maybe someone else has a windows dev env ready and beats me to it?

In D7594#145608, @rkflx wrote:

With fcc0844, Build on CI succeeds again.

Thanks