feat(mappings): show leaf node in preview window #518
Conversation
WalkthroughThe pull request introduces new mappings to the Changes
Sequence DiagramsequenceDiagram
participant User
participant FernPlugin
participant PreviewBuffer
User->>FernPlugin: Trigger preview mapping
FernPlugin->>PreviewBuffer: Open file in preview window
alt Optional directional preview
FernPlugin->>PreviewBuffer: Position preview window
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (4)
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 (
|
brandon1024
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
autoload/fern/mapping/open.vim (3)
22-23: Ensure consistent naming and add usage hints.
These lines add two new mappings for previewing nodes. It might be helpful to emphasize in accompanying documentation how these differ from other open actions. Consider providing a usage hint or including a short comment to describe their intended behavior.
97-101: Validate the scheme or show a warning.
Before checking that the scheme is 'file', consider logging or displaying a brief warning if it does not match. This helps users understand why the preview isn't available.
105-107: Consider an explicit check for preview-window availability.
When the user wants to jump to the preview window (wincmd P), ensure it is valid. In some edge cases, a preview window might be unavailable or used by another buffer, leading to confusion or unexpected results.doc/fern.txt (3)
1032-1036: Clarify behavior on non-leaf nodes.
These newly documented mappings focus on showing a leaf node in a preview window. If a user invokes them on a directory or other non-leaf node, clarify what happens (e.g., no action, error message, etc.). A short explanation can reduce confusion.
1045-1045: Confirm consistency with the new preview mappings.
Since<Plug>(fern-action-open)is labeled asopen:edit, confirm in the docs whether users can safely override it to useopen:preview. This ensures clarity for those who might want to unify their mappings.
1054-1054: Add short code snippet for user reference (optional).
If desired, provide a minimal usage example in the documentation, illustrating how to map user-defined keys to<Plug>(fern-action-open:preview)or<Plug>(fern-action-open:preview-jump). This helps users quickly adopt the feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
autoload/fern/mapping/open.vim(2 hunks)doc/fern.txt(3 hunks)
🔇 Additional comments (1)
autoload/fern/mapping/open.vim (1)
102-104: Handle directory paths gracefully.
For directories,peditmight fail or behave unexpectedly. Consider verifying ifl:pathpoints to a file and handle directory previews separately.
There was a problem hiding this comment.
- Please implement the mapping in https://github.com/lambdalisue/vim-fern/tree/main/autoload/fern/scheme/file/mapping if the mapping is schema specific
- Please follow the indentation rules around the code
- Please add more error handlings
brandon1024
force-pushed
the
node-preview-mapping
branch
from
8db35ef to
d677908
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
autoload/fern/scheme/file/mapping.vim (3)
16-17: LGTM! Consider documenting the recommended key mappings.The new preview mappings are well-implemented and follow the existing patterns. While it's good that they're not added to default mappings, consider documenting recommended key mappings in the plugin documentation to help users get started.
210-230: Consider enhancing the preview window management.The current implementation could be improved in several ways:
- Handle existing preview windows (close or reuse)
- Clean up preview windows when previewing new files
- Add window-local settings for preview window (e.g.,
nomodifiable)Here's a suggested enhancement:
function! s:map_preview(helper, mode) abort let node = a:helper.sync.get_cursor_node() if node is# v:null return s:Promise.reject('cursor node is not visible') endif if node.status isnot# a:helper.STATUS_NONE return s:Promise.reject('cannot preview a directory') endif + " Close existing preview window if any + pclose + try - execute "noautocmd bo pedit " .. node._path + execute "bo pedit " .. fnameescape(node._path) + + " Set preview window options + wincmd P + setlocal nomodifiable + setlocal nonumber + setlocal norelativenumber + wincmd p if a:mode ==# 'jump' wincmd P endif return s:Promise.resolve() catch return s:Promise.reject(v:exception) endtry endfunction
212-217: Improve error messages for better user experience.The current error messages could be more user-friendly and descriptive.
Apply this diff:
if node is# v:null - return s:Promise.reject('cursor node is not visible') + return s:Promise.reject('No file selected. Please move the cursor to a file.') endif if node.status isnot# a:helper.STATUS_NONE - return s:Promise.reject('cannot preview a directory') + return s:Promise.reject('Preview is only available for files, not directories.') endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
autoload/fern/scheme/file/mapping.vim(2 hunks)doc/fern.txt(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- doc/fern.txt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (macos-latest, nightly)
🔇 Additional comments (1)
autoload/fern/scheme/file/mapping.vim (1)
220-220: Consider the implications of usingnoautocmd.Using
noautocmdwithpeditskips buffer-local autocmds which might be important for proper buffer setup. Consider if this is really necessary, or if certain autocmds should be preserved.
Done :-)
My bad, I should have checked this. Thanks!
I wrapped the sensitive parts in a try/catch block, and I also added some checks on the cursor node to make sure it's actually valid (could be |
brandon1024
force-pushed
the
node-preview-mapping
branch
from
d677908 to
04d0158
Compare
|
This should be ready for a second review @lambdalisue :-) |
| return s:Promise.reject('cursor node is not visible') | ||
| endif | ||
| if node.status isnot# a:helper.STATUS_NONE | ||
| return s:Promise.reject('cannot preview a directory') |
There was a problem hiding this comment.
It's a bit annoying, so let's just return a void promise in this case so that user can use this mapping like
nnoremap j j<Plug>(fern-action-preview)
nnoremap k k<Plug>(fern-action-preview)There was a problem hiding this comment.
Oh, very clever idea. Will do!
| endif | ||
|
|
||
| try | ||
| execute "noautocmd bo pedit " .. node._path |
There was a problem hiding this comment.
Please make it (bo) configurable so that we can define the followings to control the preview window location.
fern-action-preview:left(vertical topleft)fern-action-preview:right(vertical botright)fern-action-preview:top(topleft)fern-action-preview:bottom(botright)fern-action-previewaliases tofern-action-preview:bottom(or one of the aboves)
Additionally, please use full name (like botright) instead of short name (like bo)
There was a problem hiding this comment.
Please use fnameescape to escape node._path like
execute printf('noautocmd %s pedit %s', a:prefix, fnameescape(node._path))There was a problem hiding this comment.
Good suggestions, will do :-)
| if a:mode ==# 'jump' | ||
| wincmd P | ||
| endif |
There was a problem hiding this comment.
I'm not 100% sure but I think we don't need this feature while we can write mapping like below to jump (please confirm if it's true.)
nnoremap P <Plug>(fern-action-preview)<C-w>pIt's nice if this tips is written in the help.
This revision introduces new plugin mappings for showing the current leaf node in a preview window, through the 'preview' and 'preview:*' actions available for the 'file://' scheme. Documentation has been updated accordingly.
brandon1024
force-pushed
the
node-preview-mapping
branch
from
04d0158 to
354126a
Compare
|
Thanks @lambdalisue :-) |
Overview
This revision introduces new plugin mappings for showing the current
leaf node in a preview window, through the 'preview' and
'preview:jump' actions available for the 'file://' scheme.
Documentation has been updated accordingly.
This was originally implemented in my personal dotfiles but I figured these features might be useful for the wider community.
Screenshots
Summary by CodeRabbit
fernfile management plugin.open:editaction.