Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support for simultaneous lean3/lean4 (in separate windows/tabs) #35
Support for simultaneous lean3/lean4 (in separate windows/tabs) #35
Changes from 10 commits
d7e0685
1de6b3b
cd18592
d6ea036
70dca5d
5976501
23522d4
f15f610
c2db67d
f9ac13a
edeaa70
81ab591
e6c4656
50953f9
7a90e25
b9e0576
d3ebca1
0a9a20f
bcef288
126690c
5d45699
adf970d
d039630
333c84a
12e14bd
8ade70b
5138c58
ef93fa1
5d1e7a9
27dda16
5e024be
ba3b226
fec293f
8e15a8f
6eecae6
04d4f60
2836a3e
61ff2b6
6fca73c
ff94cd4
28b62d8
3265d1b
0c29f2d
bbf811a
1250a0a
1689187
9f5b875
22b873b
14fbe51
7ec6249
0e5d621
37be218
40666a2
8885da7
e25e07e
ae3f0bc
c0b6c0e
01f7e35
7540117
beef5e4
24a4137
d48c9f8
e73cc6d
c63b1eb
f97b0a0
1033ed2
0e3e2f6
01d6f99
e3b1063
8604975
ac92804
6cb1154
58da940
3cdad44
eb4cd45
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should remove line 37 here suggesting installing lean.vim I suppose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for still working through things here, but one question -- can you elaborate on your thinking for having separate
_infoviews_open
here, which I think relates to functionality that persists infoview windows after they're semi-closed, right?What benefit is there in supporting that extra state / how do you see it being used?
I know there's a difference between
:close
and:quit
for normal windows, but ISTM that given that the infoview windows are non-modifiable and basically easily repopulatable from the server response that there isn't a huge functional difference, but let me know if I'm missing something.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question - it may be wise to refactor this to instead let
_infoviews[key]
have two fields,data: { win: ..., buf: ...}
andopen
, and remove_infoviews_open
- let me know if you'd like this in this PR.If we didn't have a state indicating closure, I'm not sure how we would distinguish between an infoview simply not having been created yet (in which case
_infoviews[key] = nil
, and we would want to open it) and having been explicitly closed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, after the refactor above, we may be able to indicate this by setting
_infoviews[key].data = nil
when closed and_infoviews[key] = nil
when erased - though doing this might be a little bit less readable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I see where you might have been confused here - I moved the infoview opening code to the
M.update()
method because I thought it was necessary for this, but looking back it should be fine to move it back toM.open()
, and add a config settinginfoview_open_default
to control whether new source windows automatically open it. In this case, I should be able to use whether or not_infoviews[key] = nil
as an indicator of whether it is open. Let me know if you'd like this in this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right something like that is I think exactly what's confusing me yeah. I think probably it's reasonable to do on top of what's here, yeah, since I'm still trying to wrap my head around things, so I suspect doing it that way will actually simplify things a bit and make them more similar to the original diff, so worth doing I suspect. Let me first just push a simple style commit but then think go for it! (EDIT: Done)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok actually here are my updated thoughts on this - it may be better just to do the initial refactor I mentioned above, because we're eventually going to have to add persistent state between open/close to infoviews anyways when we implement e.g. pinnable/pausable messages. At that point we would be forced to have a new state field indicating open/closed. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the refactor I've done of this anyways, if you're curious what it would have looked like: https://github.com/rish987/lean.nvim/tree/infoview_close_refactor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I follow what you mean when you're talking about persistent state yet -- I still don't understand why we don't always just want to completely get rid of an infoview when it's closed -- for pinnable messages it seems to me all you do is temporarily/permanently unhook calling
update
on a window, and then it will stay showing the contents it has. Maybe I'm missing something still though.But I want to get these set of incremental changes merged from this PR before doing more functionality, unless the implementation simplifies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it more similar to VSCode, where you can close the webview without having your set of pinned messages deleted.
This will be one of the aspects of this that seems "necessary" to make this a practical tool. For example, say you're editing Lean and LaTeX side-by side. The infoview is useful for getting an idea of how to interpret the Lean code, but once you understand it you'll probably want to get rid of the infoview for more screen space as you work on your LaTeX code. Now if there's a detail that you missed, you'll want to open the infoview again to see your pinned messages without them having been deleted.
Again, the code we have now should be good for a merge, but I'm pretty certain we'll want to implement persistence in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's take things step by step, if/when we get there we can make the implementation more complex, but for now I think we should stick to the simplest thing that solves the current desired functionality, which is:
don't error when closing the window manually(Closing the infoview buffer causes errors #20)properly look for leanpkg.toml relative to the file that's open(Properly detect lean version for files not relative to cwd #13)properly enforcemax_width
even when windows are closed/resizedall of which are in this PR (and which I'm trying to merge one by one essentially).
If after that we need a more complex implementation we can evolve into it.
EDIT: Other things that are here:
Current issues / items to check:
require'lean.infoview'.close()
re-opens the window immediately (on update). The reason is because all the open logic is in update, instead of being done onWinNew
or thereabouts I think.:close
in addition to:quit
toggle()
now toggle only the current lean window's infoview, or all?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this I believe while seeing tests fail for different reasons and causing later tests to fail. But it's possible it indeed isn't necessary. If it doesn't seem to be, feel free to kill it.