-
Notifications
You must be signed in to change notification settings - Fork 57
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
[WIP] feat: help users use rclip with image viewers on Windows #141
base: main
Are you sure you want to change the base?
Conversation
solved,connected with xnview
Please review it |
@@ -216,7 +219,7 @@ def main(): | |||
args.exclude_dir, | |||
args.no_indexing, | |||
) | |||
|
|||
XNVIEW_PATH = "C:\\Program Files\\XnViewMP\\xnviewmp.exe" |
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.
- What if I have xnview installed to a different path?
- What if I don't have xnview installed?
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.
- Why xnview? What about other image viewers.
@@ -228,6 +231,13 @@ def main(): | |||
print(f'{r.score:.3f}\t"{r.filepath}"') | |||
if args.preview: | |||
preview(r.filepath, args.preview_height) | |||
if os.path.exists(XNVIEW_PATH): |
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.
Why does this always try to open the xnview?
- What if I am on Linux/macOS?
- What if I don't want to open xnview?
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.
Thank you for the initiative 🙌
But the design of this PR is bad:
- We probably shouldn't require the user to have xnview installed to use previews on Windows.
- We shouldn't hardcode the path to xnview.
- We shouldn't open result images in xnview by default.
A better version of this PR could be:
- no changes to rclip code
- a change to README.md showing how to use xnview together with rclip to view files
Another option may involve building a generic adapter/helper that may try calling an image viewer if the user passed a CLI arg to ask for this, but this is more involved, and I am not sure if we need to do it now. But happy to discuss different approach ideas.
Can you change this PR to:
- remove changes to the rclip code
- add an example on how to use rclip with xnview, similar to this one:
Lines 121 to 137 in 01801ea
<details> <summary>Using a different terminal or viewer</summary> If you are using any other terminal or want to view the results in your viewer of choice, you can pass the output of **rclip** to it. For example, on Linux, the command from below will open top-5 results for "kitty" in your default image viewer: ```bash rclip -f -t 5 kitty | xargs -d '\n' -n 1 xdg-open ``` The `-f` param or `--filepath-only` makes **rclip** print the file paths only, without scores or the header, which makes it ideal to use together with a custom viewer as in the example. I prefer to use **feh**'s thumbnail mode to preview multiple results: ```bash rclip -f -t 5 kitty | feh -f - -t ``` </details>
Thank you, and let me know if you have any questions 😄
Thank you for your review.I was inspired by the issue and suggestion on xnview and i misunderstood thinking that the pr can be made successful by just connecting with xnview .I will improve my pr and let you know |
solved,connected with xnview