-
Notifications
You must be signed in to change notification settings - Fork 265
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
Term: Line/Char gets out of sync with insert state #33
Comments
I've had issue with term but I can't remember that one. I'll keep that in mind and try to reproduce. |
ffa7e1a might have fixed it. I'll close for now and reopen if I continue seeing it. |
Actually the problem is in this function.
When the term buffer grows past the prompt (by one or two lines), this function always fails the >= so it will never call term-char-mode. |
I don't understand the issue, maybe you are expecting a different behaviour. I wrote this code so that going to insert mode when point is on the last line and after the prompt will switch to char mode. |
The assumption is shaky because the buffer can grow past the last prompt making the boolean condition always false. (i.e. your last-prompt check wlil always be bigger than (point)). You can get into this state with the cursor on the same line as the last prompt and never go back to char mode. |
The buffer is always "past the last prompt", so I don't understand what you mean. Can you give an example? |
Assume the buffer grows past the last prompt by (1 or 2 or 3 lines). The point check will always fail then. Good state: Bad state: Try pressing RET in normal state after typing 'ls' in insert state a few times as one way to repro. |
Sorry for the delay, I forgot about this. I cannot reproduce though. I still do not understand what you mean with "buffer grows past the last prompt". |
Did you take a look at the two pictures? Note the cursor position in both. In the 'bad' state, the cursor is wayy at the bottom of the buffer. That's because the buffer grew past the prompt. Here's a video. |
Thanks for the video, it shows a few things the pics did not show. I tried reproducing from a minimal config:
to no avail. It desperately works for me :) Try a minimal config. If you can reproduce, then the difference might lie in the shell / prompt.
I'm still confused by your terms so let's lay down some definitions to be sure we are on the same page.
To me, when you say "grow past the prompt", I understand that Maybe you meant that there are new lines inserted after the last prompt. On my end, if I insert N newlines with |
I think you are looking too closely at the 'repro' instead of the outcome. What's in the video is just one way to reproduce this bug. The bug itself is in the flawed check, (if the buffer grows past the prompt by more than a few lines, the check will always fail). Things like 'wrong shell', 'wrong setup' doesn't seem to be right when the check for switching to insert/normal state is what's flawed.
Again, the special qualifier is 'by a couple of lines'.
I would expect this to work. (i.e. I do not expect this to reproduce the bug). I think a good route going forward is just to disable this kind of 'smart' code (or at least provide the option to) as it's obvious it won't work for a variety of setups. |
Why would |
Err lets disect the function then...
(goto-char (point-max)) --> What happens if the buffer is extremely large, relative to the prompt (as shown in both screenshots and video?) (setq last-prompt (max (term-bol nil) (line-beginning-position)))) --> What will last-prompt be if point-max was large? |
It goes to point-max. Size does not matter here, does it? If there was an issue in the line's logic, you should be able to reproduce with just one extra line. I don't understand why "extreme" amounts are necessary to reproduce the issue.
It will be Since you cannot edit what's before the last line in char-mode, then if That "lag" in your terminal makes me realize there could be a race condition however: if the buffer keeps growing while the code is executed, then it will fail to switch to char-mode as it should.
and
are executed. Then indeed the condition will be wrong. Not sure how to solve that. |
Do you know why your terminal is so slow by the way? Some fancy prompt calculation? Could you try with /bin/sh just to confirm my hypothesis? |
Correction: the race condition is between
and
|
Yes bin/sh is fast enough so that the repro steps above do not work but again, that's just one way to repro it. Is race condition the correct categorization? After you've entered in that state, it will fail everytime.
That or zsh is slow or osx terminal is slow. |
Yeah? We need to understand precisely why if we want to fix this. If I get it properly, you are stuck in line-mode correct? Can you edebug what happens if you wait for output to stabilize, go to the end of the buffer and switch to insert mode (e.g. by pressing |
I'm not sure I grok this. Do you want to understand why we get to that state or do you want to understand why the check fails after we've reached the 'bad' state? The first is irrelevent. The second is obvious (in my opinion). I feel like my previous comments already have a good amount of information there.
Yes, as the video shows, going back to insert state will not go back to char-mode.
That's the problem. line-beginning-position will be bigger than (point).
GA switches it back to char-mode and resets the whole thing back to a good state. |
Doesn't it contradict your previous points, i.e. that you cannot go back to char-mode? I feel like you are expecting a different behaviour than me, hence the on-going misunderstanding since the beginning... But we are getting there ;) I think I see what's going on: when you are in line-mode, you type some command and press RET. At this point, you want to enter insert mode, but because of the aforementioned race-condition, you end up in line-mode at line N while the buffer last line is N+P. If you don't move to the last line, you won't be able to enter char-mode, and that's Note that this synchronization between line-mode/normal-state and char-mode/insert-state is an additional feature, you can still use C-c C-k and C-c C-l as in Emacs state. Nothing is really broken here. I'll think of a fix. |
No, it doesn't. Typing 'GA' to go back to char-mode is what I'd consider a 'workaround', but I could also just run the command to switch me back to char-mode (what I've been doing). It's like saying "well you can open another terminal and you'd be back in char-mode so there's no bug!" Sorry, just have to drive this point down. :)
Are you sure? I have a video that shows the bug in action.
Again, that's looking at what reproduces the problem and not the problem.
last line is not correct in this case. last line is more like 'If you don't move to the prompt line, you won't be able to enter char-mode', because as you see in the repro, you are on the last line when the bug repros. I think this reply might be muddy so I'll re-iterate. 'Last line' is what I'd consider the buffer's last line but I think your 'Last line' is where the last Prompt is.
I have a problem with this characterization because it is broken. This isn't an additional feature because it swallows up the term experience. Without this feature, I would never go to line-mode unless I manually do it. Since this feature is toggling me between the two, and because this bug exists, it enters line-mode unnecessarily (understatement). Without this feature, I would be in char-mode 99% of the time, so you can see my frustration :) with it now going to line-mode and bugging out in line-mode. Without a fix, it will probably be better just to not be too smart in this case and let the user manually select the mode as they did before and default to char-mode at the start. |
What I mean with "last line" is the buffer's last line. No misunderstanding here.
I think I know what's happening: the current line might end with
fixes that problem but it will fail in case of a race condition. Allow me to explain the intention of that "feature".
Only then we realize we want to add some flags to
Now we press RET and the following command actually gets passed to the underlying shell:
Do you see why? Because the movement keys in normal mode cannot send corresponding keys to the shell. That makes the whole experience very frustrating for Evil users: we basically cannot press "ESC". With my feature, it will make sure that going to insert state will enter char-mode and move the point back to where we left: not ideal, but at least the user will not type the wrong command. Note that I had been working on a huge pile of code to properly synchronize the normal-state/line-mode and the underlying shell by moving the cursor manually. |
Just to be clear: using |
Unless my last hypothesis is wrong, I think now I understand the whole extend of the issue: in some conditions (e.g. slow shells) the sync will hit a race condition. If we cannot work around this (for now I don't see how) then I propose to make this an option (disabled by default). |
I understand the feature. I am saying that it has a bug.
Honestly, it's just easier to get char-mode working. I'm fine with just using the emacs bindings here. (e.g. What you'd get in a normal terminal.)
Yes, I don't think we'll be able to get this feature stable enough. At this point I consider it fairly experimental.
It seems like we're on the same page. Lets make this optional and default it to false. I'm okay with defaulting this to enabled by default if it were working 100% but I don't think playing around with the Term Prompt is gonna yield 100% of the time. |
Good! Glad we finally sorted this out! :D
Can you be more specific? What features would you be looking for? At last: without that syncing, what does evil-term do? It has almost nothing Evil to it, or does it? :p |
a1ad79a made the sync optional. Feel free to close this if there is nothing more to change or discuss. |
I had term integration before but we dropped it in favor of your version. To be honest, since mine didn't have the buggy behavior, anything like that was fine. I think your change makes it like that now so that's good. Thanks!
It does what all the other integration files we have does, sets up bindings in a reasonable fashion. Thanks, I'll close this and revisit if I find something off. |
@Ambrevar I noticed in recent versions of emacs 26 that you can't move out of the prompt in char-mode. So I'd vote to move to making your stuff the default (even if it's a little buggy :)) Maybe we can revisit making the integration tighter at some point. |
What do you mean? You can't move "out of the prompt" in char-mode on Emacs 25 either. |
I will double check, but I believe that is not true. It was a recent change on emacs-26 branch that changed this behavior. |
Had a chance to look at Emacs 25. The behavior is different between the two platforms and it was a recent change on Emacs 26 that changed the behavior. The change was intended though (won't change back to the old Emacs 25 behavior), so I'll change the default back to t. Feel free to change it back to nil if you feel that's a better default @Ambrevar. I'll still leave this open since it'd be nice if we could make it 100% bulletproof. |
Here's the relevant NEWS entry.
|
I'll try it out as soon as possible. From what I read there, it sounds to me that Do you still experience the issue you mentioned? If so, we might have to do something more on our end. |
I'm keeping a watch on it. I haven't seen it recently so things are looking good. Once I have some time, I'll try to repro with the same steps as described above. |
@Ambrevar Do you have use cases for where you don't want to enter term-char-mode when you go into insert mode? I'm thinking we can support an option (maybe as a default) to do something like this instead.
I generally want to go back to the prompt if I go into insert state (as far as I can think of but I might be missing some important use cases) so this works fine for me. |
The use case would be as for Eshell: to allow direct editing of the output, which is a very nice feature in my opinion. On the one hand, it violates the integrity of the output, on the other hand, it saves some copy-pasting. We could definitely make an option between the current implementation and your suggestion. |
Hmnn OK, I added an option for it (as well as made it the default). If you think the original function should be the default, feel free to set it back. Either works for me. :) |
Not sure how to exactly reproduce but the behavior was a little confusing.
It seems line and char mode gets swapped in and out. There's a way to get it in a state where you're in insert state and line state at the same time.
The text was updated successfully, but these errors were encountered: