Shizcow / dmenu-rs

A pixel perfect port of dmenu, rewritten in Rust with extensive plugin support
GNU General Public License v3.0
202 stars 9 forks source link

Add a maxlength plugin #32

Closed asdf8dfafjk closed 4 years ago

asdf8dfafjk commented 4 years ago

A replacement for i3-input -l

Hi, I'm not sure how well this fits with your philosophy so please feel free to reject.

What this adds- a flag maxlength which lets you specify the maximum length of input string after which the prompt returns with the input string, without having to press enter. It's a replacement for i3-input -l. I should note that there is a bit of difference between your more performant reimplementation and the original dmenu. Difference:

This plugin outputs the input (but only when a maxlength is supplied).

Example usage: echo ''| dmenu --maxlength=5 followed by an input, auto-terminates upon length being exceeded.

Testing:

Code

Let me know if you want me to make improvements.

Shizcow commented 4 years ago

As for the different behavior, that's probably a bug, but irrelevant to this discussion. I'll take a look at that.

Edit: Yup, this was a bug. It's been fixed.

asdf8dfafjk commented 4 years ago

Hi, thank you for agreeing to merge it, and I read your review and all points are very pertinent. I will address soon.

asdf8dfafjk commented 4 years ago

Let me know if the changes are alright and I will squash my commits to make a single.

Shizcow commented 4 years ago

Looks great. I noticed the comment about pasting. In that event, should the input be truncated to fit maxlength? If so graphemes(true) returns an Iterator so you can .take(N).collect::<String>()or whatever the correct types are.

As for your issues with fcitx, I'll run it on my machine and give it some tests. I don't expect any issues here.

asdf8dfafjk commented 4 years ago

I resolved the pasting issue, tested non-paste functionality. I also merged my commits.

I think I accidentally raised PR against your master branch, hence one of your commits shows up in the PR. Let me know if you'd like me to start another PR. Feel free to do anything that makes it easiest for you to merge (I only care about functionality)

Shizcow commented 4 years ago

You can easily change the target of a PR without submitting. In this case, you can switch from master to develop. I'm looking to do another minor release or two before a major release, so I expect these changes to hit master relatively soon anyway

Shizcow commented 4 years ago

Sorry for delay in merging. I've done some additional testing and everything checks out.

asdf8dfafjk commented 4 years ago

Thank you. Don't worry about delay.