search

mattydebie / bitwarden-rofi

Wrapper for Bitwarden https://github.com/bitwarden/cli and Rofi
GNU General Public License v3.0
351 stars 57 forks source link

Add support for options? #4

Closed Mange closed 5 years ago

Mange commented 5 years ago

I've been working on a fork to add some things that I wanted.

  1. Support xsel instead of just xclip. (#3)
  2. Don't show password in the notification dialog.
  3. Allow the automatic clipboard clearing to wait longer.
  4. Store state file in a XDG-compatible location.

I've implemented step 2-4 via CLI options in my fork. If you merge #2 and #3, then I could open another PR with those changes applied too.

Let me know if you are interested! I'm not opening them right now because of the requirement on #2 and #3. If either of them are rejected, I could rebase my changes and try to open on top of your own master.

[EDIT]: Here's a link to show the commits that would be included in such a PR: https://github.com/Mange/bitwarden-rofi/compare/35d05edd8f25233e59a38b76ac2a3288bf17ee3a...options

mattydebie commented 5 years ago

First of all I'd like to thank you for taking the time to improve this script. Improving the quality of the script (#2) has been accepted and something that's indeed necessary to keep the code quality.

3 is a nice addition as well, I think the abstraction is fine.

I surely am interested in a PR for options, the changes you are showing could lead to a nice improvement. I was thinking about making this script configurable at first by using a simple INI file, but maybe creating runtime configuration is a better plan. Though I'll probably still implement the INI file for basic configuration like the --state-path.

Looking forward to your PR!

Mange commented 5 years ago

I'm glad to hear it! PR will be coming very soon!

mattydebie commented 5 years ago

Closing this issue as it is being handled in PR: #5