Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle paste to beginning of buffer correctly. #15939

Merged
merged 1 commit into from
Apr 25, 2022

Conversation

jfehrle
Copy link
Member

@jfehrle jfehrle commented Apr 22, 2022

Fixes #15882

The cause is similar to that of #15861. When the entire buffer is replaced with a paste, the buffer is emptied and all the marks disappear, therefore a different approach is needed.

Not very elegant; IMHO the marks are not a great fit for the test.

@Alitzer Would you test this, too? I tried the cases I could think of--not many.

@jfehrle jfehrle added kind: fix This fixes a bug or incorrect documentation. part: CoqIDE Issues and PRs related to CoqIDE or other IDE features of coq. labels Apr 22, 2022
@jfehrle jfehrle added this to the 8.15.2 milestone Apr 22, 2022
@jfehrle jfehrle requested a review from ppedrot April 22, 2022 04:37
@jfehrle jfehrle requested a review from a team as a code owner April 22, 2022 04:37
let () = List.iter (fun mark ->
try match mark with
| `Mark mark -> buffer#delete_mark (`MARK mark)
| `Begin -> let action = Coq.seq (coqops#backtrack_to_begin ())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you simply use handle_reset_initial there? I think it's precisely behaving like this piece of code.

Copy link
Member Author

@jfehrle jfehrle Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling handle_reset_initial gives the error "Already initialized" from Idetop.init. Calling Coq.reset_coqtop works, but it doesn't correctly clear breakpoints in the replaced part of the buffer. I'd rather not tie this code to breakpoints, at least for now.

@@ -118,7 +118,9 @@ let create_script coqtop source_buffer =

let misc () = CDebug.(get_flag misc)

type action_stack = Gtk.text_mark list option
type action_stack_entry = [ | `Mark of Gtk.text_mark | `Begin ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use algebraic variants, they have a lot of issues. Either use the option type or remove the backquotes from these constructors.

Copy link
Member Author

@jfehrle jfehrle Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. The OCaml doc doesn't explain algebraic variants very well. Or for that matter, type inference.

Copy link
Contributor

@Alizter Alizter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine, I've tried a few messy things stepping back and forth but I haven't been able to reproduce any phantom processed proofs, as I was able to do before.

@ppedrot ppedrot self-assigned this Apr 22, 2022
@jfehrle
Copy link
Member Author

jfehrle commented Apr 22, 2022

Updated.

Couldn't we avoid using marks entirely by instead finding the smallest changed offset in the buffer in delete_cb and add_cb and calling backtrack_to_iter? That code would be much simpler and wouldn't have had these two problems.

@ppedrot
Copy link
Member

ppedrot commented Apr 22, 2022

I tried using backtrack_to_iter recently to fix this set of issues but I didn't manage to make it work properly. Iters have a strong tendency to get invalidated under your feet.

@jfehrle
Copy link
Member Author

jfehrle commented Apr 22, 2022

In this case, compute the offset and create an iterator from it just before calling backtrack_to_iter.

@ppedrot
Copy link
Member

ppedrot commented Apr 25, 2022

@coqbot merge now

@coqbot-app coqbot-app bot merged commit f373deb into coq:master Apr 25, 2022
SkySkimmer added a commit to SkySkimmer/coq that referenced this pull request May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: fix This fixes a bug or incorrect documentation. part: CoqIDE Issues and PRs related to CoqIDE or other IDE features of coq.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CoqIDE processed state isn't updated upon pasting to the buffer
3 participants