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

Feat: Add following mode #114

Merged
merged 14 commits into from
Dec 14, 2023
Merged

Conversation

TornaxO7
Copy link
Contributor

@TornaxO7 TornaxO7 commented Dec 13, 2023

Closes #113
Closes #81

I haven't added any docs yet because I'm unsure how I can test it and I'd like to get some suggestions from you first, like if it's fine how I changed the code :)

@AMythicDev
Copy link
Owner

AMythicDev commented Dec 13, 2023

The implementation seems to be ok however there are still a few things left:-

  • The f keybinding just doesn't work.
  • I think this feature shouldn't be the on by default. Most people want text to scroll down at their own will. It's the only reason why people use a pager. However we should give end-applications freedom to control this behavior which I see you have done.
  • In static output mode, I think its best if minus jumps directly to the last line which seems consistent with the behavior that we are currently trying to establish here.
  • Can we have a better key binding like C-f rather than just f.

I think this PR should also close #81.

@TornaxO7
Copy link
Contributor Author

TornaxO7 commented Dec 13, 2023

@arijit79 update :D (I tackled your points)

@AMythicDev
Copy link
Owner

You missed my point actually. I was saying that in static output, the pager should jump to the last line only if follow mode is enabled so that it matches with the functionality of dynamic mode. Also your code fails to compile. Also the ToggleSearch keybinding still does not work: its likely you didn't add it in the newer input system. I am making a commit to fix these issues.

@AMythicDev
Copy link
Owner

Now everything seems to be in place. Just few more things to get done. First if the user enables follow mode and new data is coming, minus takes them to the last line of output. But if no new input comes, then there it does not jump directly towards the last line.

@AMythicDev
Copy link
Owner

AMythicDev commented Dec 13, 2023

Thanks for 9e9b273. I didn't even notice that.

EDIT: BTW I am working on the issue that I talked in my earlier comment. I will post it later today.

@TornaxO7
Copy link
Contributor Author

EDIT: BTW I am working on the issue that I talked in my earlier comment. I will post it later today.

oh eh, I already pushed a change, which, if I understood you correctly, should address the issue. 😅

@AMythicDev
Copy link
Owner

No that wasn't exactly what I was talking about. I had already pushed a commit fixing exactly the same issue that you pushed. Don't worry I will fix the issue today.

@TornaxO7
Copy link
Contributor Author

No that wasn't exactly what I was talking about. I had already pushed a commit fixing exactly the same issue that you pushed. Don't worry I will fix the issue today.

oh, sorry... so should I let you do the changes for the time being?

@AMythicDev
Copy link
Owner

I have pushed the changes. You can try them to report any errors/bugs that you find.

@TornaxO7
Copy link
Contributor Author

I have pushed the changes. You can try them to report any errors/bugs that you find.

May I ask how I can test it out? Do I have to implement a custom binary which uses the lib?

@AMythicDev
Copy link
Owner

AMythicDev commented Dec 14, 2023

You can try it out in any of the examples.

You can run:-

cargo run --examples [example name] --features [features that you want]

For example:-

cargo run --example dyn_tokio --features=dynamic_output,search

@TornaxO7
Copy link
Contributor Author

I've got an idea: Would be neat, if the statusbar could show an indicator that it's currently in "following-mode".

@AMythicDev
Copy link
Owner

I was thinking about the same while writing the code and I have planned for it. We could use a traditional solution right now But it would get much easier to implement once I introducecommand queue feature which would allow us to chain another command after the current command is executed.
This would take me a day to get there.

Other then that I think this PR is ready so if you want this immediately on the main branch, I can merge this otherwise I will keep this until I get the status bar message thing going.

@TornaxO7
Copy link
Contributor Author

whoops.... I read your message too late ;-;
I can revert the changes if you want.

@TornaxO7
Copy link
Contributor Author

I was thinking about the same while writing the code and I have planned for it. We could use a traditional solution right now But it would get much easier to implement once I introducecommand queue feature which would allow us to chain another command after the current command is executed. This would take me a day to get there.

Sounds good!

Other then that I think this PR is ready so if you want this immediately on the main branch, I can merge this otherwise I will keep this until I get the status bar message thing going.

I don't need it now, so you can implement the command queue first

@AMythicDev
Copy link
Owner

It's good but I was thinking about using a message rather than adding an entire block to show that. If static text is required we could also use a single character for that. What do you say?

@TornaxO7
Copy link
Contributor Author

TornaxO7 commented Dec 14, 2023

It's good but I was thinking about using a message rather than adding an entire block to show that.

hm... may I ask what you mean with "message"?

If static text is required we could also use a single character for that. What do you say?

On the one hand I'd prefer to have this static text because it's self-explaining what it indicates. If there will be more entries, a single letter would be fine but on the other hand a [F] might be already self-explaining I think, so yeah, I think something like [F] is fine.

@AMythicDev
Copy link
Owner

A message in minus allows you to show a text message to the user at the prompt site. It would disappear when the user presses any other key/does a mouse action. We can display a message like Follow output enabled when it is enabled. If you want to know more about a message, try msg-tokio example to see a practical demo of it.

@TornaxO7
Copy link
Contributor Author

TornaxO7 commented Dec 14, 2023

A message in minus allows you to show a text message to the user at the prompt site. It would disappear when the user presses any other key/does a mouse action. We can display a message like Follow output enabled when it is enabled. If you want to know more about a message, try msg-tokio example to see a practical demo of it.

oh ok. So, I'd prefer a static text because assuming you're looking through some logs and you forgot that the follow mode is still enabled, you'll start scrolling up but after some time something new appears and you'll be thrown back to the bottom. I could imagine that this might be a bit annoying.

@AMythicDev
Copy link
Owner

Hmm. Fine then.

@TornaxO7
Copy link
Contributor Author

TornaxO7 commented Dec 14, 2023

but yeah, now I'm quite happy with the result 👍

thank you for being very responsive :)

@AMythicDev
Copy link
Owner

Let's merge this then. I will introduce minor improvements on this in future commits.

@AMythicDev AMythicDev merged commit e40068a into AMythicDev:main Dec 14, 2023
6 checks passed
@AMythicDev AMythicDev assigned AMythicDev and TornaxO7 and unassigned AMythicDev Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Follow-mode Starting at the end of input
2 participants