#194 improve marker cleanup

nittka commented 4 years ago

Remove markers in included files as well, not only in the main file to be compiled.

If you merge PR #192 first, I'd rebase this commit.

nittka commented 4 years ago

I have rebased the branch. Ready for review.

(I have worked too much with gerrit lately - one PR=one commit, so it did not even occur to me, that I could have simply done the two fixes with two commits in one PR.)

nittka commented 4 years ago

I am quite confident about this change and it is not a fundamentally new feature. However, I'd still appreciate your feedback before merging.

nittka commented 4 years ago

Thanks for your feedback! This is indeed a "border-case" currently not covered. However, the cause is not the marker removal itself, but the calculation of included files.

The calculation is based on the index information of resolved references. But an include alone does not yet create such a reference. Only if you refer to a definition from an included file, such a reference is created. You can see the difference if you add musicRef=\music to the main ly-file.

Should I try to fix this "academic" case for this PR or should it be a separate issue.

nittka commented 4 years ago

It turns out, the problem is even more border-line. Even if you add any real reference (like someRef=\trill) to the main file, it works. The scope provider is responsible for doing the the transitive include calculation. Without a reference it is not invoked (and it looks like the file has no includes at all).

Unfortunately it does not suffice to invoke the scope provider during validation - the linking and creation of the resource descriptions is already done at that point. Hopefully, I find a simple way to ensure the scope provider is at least invoked once...

thSoft commented 4 years ago

I see. If you find a simple fix, let's include also that one, otherwise I'd propose to track the scope calculation as a separate issue, rename #194 and I'll approve this PR.

nittka commented 4 years ago

I had to do another bigger refactoring. The initial change was too simplistic. Removing the markers from all included files when saving one changed file is wrong: if you have several included files, each with a lilypond compile error, markers should be removed only from the changed file. Otherwise too much information is lost.

The PR now contains the following improvements:

recognize an included file, even if there are no Xtext references (the problem you indicated in your comment)
remove a lilypond marker also in changed included files (the actual bug reported)
update the decorators in the score library as well (before you had to do a manual refresh on the project or file in order for problem markers to disappear)
do not ask the user if compilation is to be continued (open dirty files) if lilypond compilation is not active during build

nittka commented 3 years ago

If you are currently working on Elysium, could you have another look at the changes?

thSoft commented 3 years ago

Sorry for the delay. I'm updating my development environment and going to review this in one week if everything goes well.

thSoft commented 3 years ago

Unfortunately I stumbled into https://github.com/thSoft/elysium/issues/197. I ask for your patience again, this might take one more week.

thSoft commented 3 years ago

I'd like to try this out. Please rebase this branch to current master and provide manual test instructions to verify whether this works correctly. Thank you in advance!

nittka commented 3 years ago

OK to summarize: Compilation is executed only on the main ly-File. If the compiler finds problems in an included file, markers are created there.

Previously, these markers were not removed if the problem was fixed and the main file was acutally recompiled. This problem is fixed by the PR.
Also you indicated a problem with the initial attempt in https://github.com/thSoft/elysium/pull/195#pullrequestreview-442711338 This problem has been addressed (check with the test project you provided). Typically Xtext creates crossreferences if they are needed (there is an actual reference), an include/import alone does not suffice. This PR ensures, that includes alone suffice to establish the reference. I.e. if x.ly includes y.ily without actually referencing a variable defined there, error markers in y.ily will be cleared if the problem was fixed and x.ly is recompiled.
If compilation on save is enabled, you are asked whether you want to save an open dirty file before compilation starts. Previously, you were aske also if compilation on save was not enabled. This was annoying, because saving or not saving the file does not make a difference, if the compiler will not run.

nittka commented 3 years ago

I have updated the PR accordingly

thSoft / elysium

#194 improve marker cleanup #195