If/else to case in preview-tui #1009
Conversation
There was a problem hiding this comment.
@leovilok please review if you can
Edit: I'm mostly just looking for other opinions on whether this is actually an improvement over the old code, and comments on the specifics of the implementation. @jarun what do you think since you're the one who initially requested the review. I saw no other problems in preview-tui beside the abundant if/else clauses. To me it is more readable and potentially more performant though that would be hard to verify.
| @@ -355,7 +351,8 @@ generate_preview() { | |||
| image_preview() { | |||
| clear | |||
| if [ "$TERMINAL" = "kitty" ]; then | |||
| # Kitty terminal users can use the native image preview method. | |||
| # Kitty terminal users can use the native image preview method although one might consider | |||
| # installing ueberzug and moving the ueberzug clause to the top since it performs alot better. | |||
There was a problem hiding this comment.
Add note about kitty vs ueberzug performance consideration
| else | ||
| fifo_pager pager "$1" | ||
| fi | ||
| handle_mime "$1" "$mimetype" "$ext" "bin" |
There was a problem hiding this comment.
Pass "bin" as argument to handle_mime() to maintain the if [ "${encoding#*)}" = "binary" ] condition.
| else | ||
| fifo_pager pager "$1" | ||
| fi ;; | ||
| *) handle_ext "$1" "$3" "$4" ;; |
There was a problem hiding this comment.
If no mimetype matches, check extensions
| fifo_pager pager "$1" | ||
| fi ;; | ||
| pdf) generate_preview "$cols" "$lines" "$1" "pdf" ;; | ||
| *) if [ "$3" = "bin" ]; then |
There was a problem hiding this comment.
print_bin_info() or pager depending on whether or not we came from if [ "${encoding#*)}" = "binary" ]
|
Thanks for picking that up @luukvbaal. Seeing how a simple change like 1004 can be hectic if I'm the one doing it, I'm glad you stepped forward! I feel the same about |
|
Unless I am missing some magic Github feature that would automatically merge, those two |
|
Correct, I was working pre-#1004. It will merge fine since there are no merge conflicts, I'll rebase though. |
|
As you want, if you close it I'll add it here |
|
Thanks! |
|
Thank you! |
@jarun thoughs on this? |
|
After the next release. |
Replace
if/elseclauses withcaseblock in view of optimizations mentioned in #935 (comment). Should maintain exactly the same logic as previously.I'm not sure how I feel about maintaining
preview-tuiin the future though. If it were up to me I would just renamepreview-tui-exttopreview-tuiand let users edit that how they see fit instead of extending the more minimalpreview-tui. The extended script works fine without the optional dependencies installed.