Skip to content
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

feat: support preview for entries in path source menus #86

Merged
merged 1 commit into from
Mar 3, 2024

Conversation

willothy
Copy link
Collaborator

@willothy willothy commented Sep 10, 2023

Adds support for previewing files from the path source.

@willothy willothy changed the title [POC] feat(sources): preview entries in path source menus feat(sources): preview entries in path source menus Sep 10, 2023
@willothy willothy changed the title feat(sources): preview entries in path source menus feat: floating preview for entries in path source menus Sep 10, 2023
@willothy willothy force-pushed the feat-file-preview branch 2 times, most recently from ec3b03d to 8ee3dbd Compare September 10, 2023 11:29
@Bekaboo Bekaboo force-pushed the master branch 2 times, most recently from 86405e2 to d42a135 Compare October 27, 2023 06:22
@willothy
Copy link
Collaborator Author

willothy commented Nov 28, 2023

Considering an alternate approach here... what if the previews for files worked the same way as for TS/LSP sources, where the file is previewed in the active window instead of a popup? I think that would allow the preview to be more consistently visible, since having nested menus open significantly shrinks the space available for the preview popup.

Could also make this configurable, so files could be previewed in a popup or the active window. This could also be extended to allow other kinds of symbols to be previewed in popups.

@willothy willothy force-pushed the feat-file-preview branch 2 times, most recently from 98c590e to 4d3f30c Compare January 23, 2024 10:14
@willothy
Copy link
Collaborator Author

Not quite ready for merge but ready to be tested - now supports previewing in a floating window, or in the current window like document symbols are previewed.

@willothy willothy marked this pull request as ready for review January 23, 2024 10:15
@willothy willothy force-pushed the feat-file-preview branch 3 times, most recently from d175487 to 5c8cbe9 Compare January 24, 2024 18:05
@willothy
Copy link
Collaborator Author

Ok fixed the bugs, this should be pretty much ready as well.

@willothy
Copy link
Collaborator Author

I just need to document dropbar_menu_t:root_win()

I added it as a method instead of just doing a check in the path source because I think is useful to have and makes it more clear that prev_win can be either a menu or a regular buffer.

@willothy willothy changed the title feat: floating preview for entries in path source menus feat: support preview for entries in path source menus Jan 25, 2024
@willothy
Copy link
Collaborator Author

willothy commented Feb 5, 2024

@Bekaboo this has been working well in my config for a bit, I'll upload a new video here today.

Do we want to have the floating preview though? It's different from how all of the other previews work and I'm not sure I'm a fan of it - I kinda want to remove it and have this PR only support preview in the way we already do in order to keep things consistent between the sources. Thoughts?

@Bekaboo
Copy link
Owner

Bekaboo commented Feb 6, 2024

@willothy Thanks for your work!

Do we want to have the floating preview though? It's different from how all of the other previews work and I'm not sure I'm a fan of it - I kinda want to remove it and have this PR only support preview in the way we already do in order to keep things consistent between the sources. Thoughts?

I agree with you.

@willothy
Copy link
Collaborator Author

willothy commented Feb 6, 2024

Cool, sounds good! I'll clean this up and remove the floating preview.

@Bekaboo
Copy link
Owner

Bekaboo commented Feb 15, 2024

@willothy Is this ready to merge?

@willothy
Copy link
Collaborator Author

Uhh I forget, I'll look over it today and let you know. I think I just need to do a little bit of cleanup.

@willothy
Copy link
Collaborator Author

@Bekaboo ok, good to go! Sorry for the delay.

@Bekaboo
Copy link
Owner

Bekaboo commented Feb 29, 2024

Ty, looks great! @willothy I have been a bit busy recently but I'll review and merge this ASAP.

@willothy
Copy link
Collaborator Author

Sounds good! No rush :)

@Bekaboo Bekaboo force-pushed the feat-file-preview branch from 5cb334a to fa51f27 Compare March 2, 2024 10:00
@Bekaboo
Copy link
Owner

Bekaboo commented Mar 2, 2024

@willothy Why do we need a new field preview_win? Before this patch, we always preview symbols in the current window (the window that the dropbar is attached to) and the window id is stored in dropbar_symbol_t.win, i.e. it is the symbol instead of the menu that is responsible for previewing.

@willothy
Copy link
Collaborator Author

willothy commented Mar 2, 2024

Right, good catch! That's leftover from the popup preview, I should've removed it. I'll get rid of that now :)

@willothy willothy force-pushed the feat-file-preview branch from fa51f27 to 62ed697 Compare March 2, 2024 21:40
@willothy
Copy link
Collaborator Author

willothy commented Mar 2, 2024

@Bekaboo I've rebased onto master and removed the preview_win field. Should be all good now.

@Bekaboo Bekaboo force-pushed the feat-file-preview branch from 17d2fa8 to 281260f Compare March 3, 2024 03:56
fix(sources-path): indexing nil value when previewing dirs/links, etc.

feat(sources-path & configs): add `opts.sources.path.preview`
@Bekaboo Bekaboo force-pushed the feat-file-preview branch from b8e02b6 to 353e739 Compare March 3, 2024 13:25
@Bekaboo Bekaboo merged commit 0a557bd into Bekaboo:master Mar 3, 2024
5 checks passed
@Bekaboo
Copy link
Owner

Bekaboo commented Mar 3, 2024

@willothy Merged, thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants