画像を取得して、sixelで表示できるようにする #84
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe recent changes enhance the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
画像がでか過ぎたりするとその一部しか表示されないので、補完ウィンドウみたいな感じで適切な大きさにするようにしたい |
|
gw31415/denops-sixel-view.vim#1 PR出してみた |
|
sixel用プラグインがちゃんと認識されてるかチェックを入れとく |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
doc/traqvim.jax (1)
208-237: Well-structured FAQ section.The new FAQ section provides clear instructions on using the sixel graphics protocol with the
denops-sixel-view.vimplugin. The example functions and key mapping are well-documented, making it easy for users to implement the image display feature. Ensure that the plugin's installation instructions are up-to-date and consider adding a brief explanation of sixel for users unfamiliar with the format.
| export const downloadFile = async ( | ||
| fileId: string, | ||
| ): Promise<Uint8Array> => { | ||
| try { | ||
| const fileRes = await api.fetchWithToken( | ||
| "GET", | ||
| "/files/" + fileId, | ||
| {}, | ||
| ); | ||
| const blob = await fileRes.blob(); | ||
| return new Uint8Array(await blob.arrayBuffer()); | ||
| } catch (e) { | ||
| console.error(e); | ||
| } | ||
| return new Uint8Array(); | ||
| }; |
There was a problem hiding this comment.
Ensure comprehensive error handling in downloadFile.
The downloadFile function correctly uses try-catch for error handling. However, consider providing more context in the error message or logging additional details to aid in debugging. Additionally, ensure that the API endpoint and response handling are robust and handle potential edge cases, such as network failures or unexpected response formats.
- console.error(e);
+ console.error('Failed to download file:', e);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const downloadFile = async ( | |
| fileId: string, | |
| ): Promise<Uint8Array> => { | |
| try { | |
| const fileRes = await api.fetchWithToken( | |
| "GET", | |
| "/files/" + fileId, | |
| {}, | |
| ); | |
| const blob = await fileRes.blob(); | |
| return new Uint8Array(await blob.arrayBuffer()); | |
| } catch (e) { | |
| console.error(e); | |
| } | |
| return new Uint8Array(); | |
| }; | |
| export const downloadFile = async ( | |
| fileId: string, | |
| ): Promise<Uint8Array> => { | |
| try { | |
| const fileRes = await api.fetchWithToken( | |
| "GET", | |
| "/files/" + fileId, | |
| {}, | |
| ); | |
| const blob = await fileRes.blob(); | |
| return new Uint8Array(await blob.arrayBuffer()); | |
| } catch (e) { | |
| console.error('Failed to download file:', e); | |
| } | |
| return new Uint8Array(); | |
| }; |
denops/traqvim/main.ts
Outdated
| async getFile(fileId: unknown): Promise<unknown> { | ||
| ensureString(fileId); | ||
| const file: Uint8Array = await downloadFile(fileId); | ||
| // denops-sixel-view.vimのsixel_viewがUint8Arrayを受け取れる必要がある | ||
| const sixel = denops.dispatch("sixel_view", "img2sixel", file); | ||
| ensureObject<{ data: string; height: number; width: number }>(sixel); | ||
| return sixel; |
There was a problem hiding this comment.
Validate sixel_view dispatch result thoroughly.
The getFile method ensures the fileId is a string and handles the Uint8Array correctly. However, the validation of the sixel object could be more robust. Consider checking if the sixel object is not only an object but also contains the expected properties with valid values to prevent runtime errors.
- ensureObject<{ data: string; height: number; width: number }>(sixel);
+ const { data, height, width } = ensureObject<{ data: string; height: number; width: number }>(sixel);
+ if (typeof data !== 'string' || typeof height !== 'number' || typeof width !== 'number') {
+ throw new Error('Invalid sixel object structure');
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getFile(fileId: unknown): Promise<unknown> { | |
| ensureString(fileId); | |
| const file: Uint8Array = await downloadFile(fileId); | |
| // denops-sixel-view.vimのsixel_viewがUint8Arrayを受け取れる必要がある | |
| const sixel = denops.dispatch("sixel_view", "img2sixel", file); | |
| ensureObject<{ data: string; height: number; width: number }>(sixel); | |
| return sixel; | |
| async getFile(fileId: unknown): Promise<unknown> { | |
| ensureString(fileId); | |
| const file: Uint8Array = await downloadFile(fileId); | |
| // denops-sixel-view.vimのsixel_viewがUint8Arrayを受け取れる必要がある | |
| const sixel = denops.dispatch("sixel_view", "img2sixel", file); | |
| const { data, height, width } = ensureObject<{ data: string; height: number; width: number }>(sixel); | |
| if (typeof data !== 'string' || typeof height !== 'number' || typeof width !== 'number') { | |
| throw new Error('Invalid sixel object structure'); | |
| } | |
| return sixel; |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- doc/traqvim.jax (1 hunks)
Additional comments not posted (1)
doc/traqvim.jax (1)
207-246: Review of New FAQ Section and Associated Functions and Key Mappings
FAQ Section:
- The FAQ section provides a clear guide on how to display images using the sixel format. This aligns well with the PR objectives and linked issues.
- The instructions are concise and direct users to install the necessary plugin and set up functions.
- The FAQ section is well-written and serves its purpose effectively.
Function
s:clear:
- This function is designed to clear the displayed image, similar to clearing the screen in a terminal.
- The implementation directly calls a function from the
sixel_viewplugin, which is appropriate.- The function is correctly implemented and documented.
Function
s:preview_img:
- This function handles the downloading and conversion of files to sixel format for display.
- It uses the
denops#requestfunction to fetch the file, which is a good use of asynchronous capabilities in Vim plugins.- The function is well-implemented and serves its intended purpose.
Function
s:preview_cursor_img:
- This function is triggered by cursor movement and is designed to preview an image based on the URL at the cursor position.
- It extracts the file ID from the URL and calls
s:preview_img.- The use of an autocommand to clear the image on cursor movement or buffer leave is a thoughtful addition.
- The function is innovative and enhances user interaction with images.
Key Mapping:
- The key mapping for
Kis set to trigger thes:preview_cursor_imgfunction.- This choice of key is intuitive for Vim users, as
Kis often associated with looking up documentation or related information.- The key mapping is appropriately set and documented.
Overall, the changes are well-aligned with the objectives of the PR and enhance the documentation significantly. The implementation of functions and key mappings is robust, ensuring a good user experience.
kamecha
force-pushed
the
feature/view-file-sixel
branch
from
26db8b3
to
4467409
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- denops/traqvim/main.ts (2 hunks)
- denops/traqvim/model.ts (1 hunks)
- doc/traqvim.jax (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- doc/traqvim.jax
Additional comments not posted (1)
denops/traqvim/model.ts (1)
345-360: Enhance error handling indownloadFile.The basic functionality of the
downloadFilefunction is implemented correctly. However, the error handling could be more informative to aid in debugging. Consider adding more context to the error message and ensuring that the API endpoint and response handling are robust enough to handle potential edge cases, such as network failures or unexpected response formats.Apply this diff to enhance the error message:
- console.error(e); + console.error('Failed to download file:', e);Likely invalid or redundant comment.
| async getFile(fileId: unknown): Promise<unknown> { | ||
| assert(fileId, is.String); | ||
| const file: Uint8Array = await downloadFile(fileId); | ||
| // denops-sixel-view.vimのsixel_viewがUint8Arrayを受け取れる必要がある | ||
| const sixel = await denops.dispatch("sixel_view", "img2sixel", file); | ||
| assert( | ||
| sixel, | ||
| is.ObjectOf({ | ||
| data: is.String, | ||
| height: is.Number, | ||
| width: is.Number, | ||
| }), | ||
| ); | ||
| return sixel; | ||
| }, |
There was a problem hiding this comment.
Enhance validation of sixel object in getFile.
The getFile method correctly handles file fetching and initial processing. However, the validation of the sixel object could be more robust to prevent runtime errors. Consider checking if the sixel object not only exists but also contains the expected properties with valid values.
Apply this diff to enhance the validation:
- ensureObject<{ data: string; height: number; width: number }>(sixel);
+ const { data, height, width } = ensureObject<{ data: string; height: number; width: number }>(sixel);
+ if (typeof data !== 'string' || typeof height !== 'number' || typeof width !== 'number') {
+ throw new Error('Invalid sixel object structure');
+ }Committable suggestion was skipped due to low confidence.
kamecha
force-pushed
the
feature/view-file-sixel
branch
from
4467409
to
23c7e16
Compare
kamecha
force-pushed
the
feature/view-file-sixel
branch
from
23c7e16
to
1fcd5bc
Compare
|
windows terminalもpreviewだとsixel対応したし、mergeしてドッグフーディングとやらをやってきたい |
https://github.com/gw31415/denops-sixel-view.vim
↑を使ってsixelを表示するようにしてみた
本家の方がUint8Arrayを受け付けるようにしたり、色々手を加える必要があるため一旦保留
Close #7
Summary by CodeRabbit
New Features
Bug Fixes