KeYProject / key

KeY Theorem Prover for Deductive Java Verification

https://key-project.org

Other

44 stars 25 forks source link

Proof tree filters not applied correctly #3367

Closed WolframPfeifer closed 10 months ago

WolframPfeifer commented 11 months ago

Description

The filters in the proof tree ("Hide Intermediate Proofsteps" etc.) are not applied correctly, at least in the case of a freshly started proof.

Reproducible

always

Steps to reproduce

Start any proof and apply some rules or macros to it.
Select "Hide Intermediate Proofsteps" in the proof tree settings (gear wheel).

I would expect that the intermediate proof steps are filtered out. However, the proof tree stays as it is (no filters applied). Furthermore, if the menu is opened again, the checkbox is not ticked.

Additional information

By bisecting, I found the first bad commit: 3ab8750d0e35f3a5d694d4c0e8bf196a7f1b2607.
The problem seems to have something to do with the focus: At least in my testing, it only happened in freshly started proofs. If multiple proofs are present, after switching between them, the filters work fine.

WolframPfeifer commented 11 months ago

@mi-ki I think this is the issue you mentioned to me yesterday, right?

WolframPfeifer commented 11 months ago

@unp1 The commit that introduced this problem is 3ab8750d0e35f3a5d694d4c0e8bf196a7f1b2607. It seems that you tried to do some refactoring here ...

unp1 commented 11 months ago

Will look into it (but first next week)

unp1 commented 11 months ago

See PR #3368.

The root cause was not the commit itself, but it unearthed an inconsistency between two places where node selections are stored.

unp1 commented 11 months ago

We might think about removing the additional selection storage from the model and instead have a wrapper record like

ProofTreeViewState(GUIProofTreeModel model, TreePath selectionPath, Filter[] activeFilters)

and then storing this record in the hashmap when switching proofs.

This change would avoid accidental usage of the selection stored in the model and it would allow us to have proof local filter settings. But that would change current behavior and we would need to be fine with it. The current PR just restores previous behavior.

unp1 commented 11 months ago

See PR #3369 for an implementation of the suggestion. It builds on top of PR #3368

mi-ki commented 11 months ago

@mi-ki I think this is the issue you mentioned to me yesterday, right?

@WolframPfeifer Yes, you are right, thanks for looking into it!

unp1 commented 11 months ago

Brief report on status of PR #3368 and #3369.

Both PRs contain the same refactorings as well as the (unrelated) bugfixes

for updating the sequentview when pretty printing options are modified
for a NullPointerException that occurred when closing the Proof Obligation Browser via Cancel when no proof (at all) was loaded in the GUI.
reduce updates of sequent view when changing view settings: changing a view setting propagated a PropertyChangeEvent causing unrelated properties to trigger their own redraw update just in case

3369 changes proof tree filters to proof local filters and #3368 does not

Both are finished from my side (of course, feedback from code reviews will be incorporated). The semantics to be used is up for discussion, but as @WolframPfeifer prefers and has actual use cases for #3368, we might want to stay with that semantics and not too optimize for some imaginery unknown user :-)