Feature/add bookmarks menu #2554
Conversation
make registering a command bound to the class
|
Excited for this! This and a terminal selector are the things I use most that I've added myself, so glad to see it being added properly! If I can swap my terminal selector to be entirely in Oni (and not half in Oni half in my Vim config), I'll try adding that too. |
Codecov Report
@@ Coverage Diff @@
## master #2554 +/- ##
==========================================
+ Coverage 44.93% 45.31% +0.38%
==========================================
Files 352 353 +1
Lines 14336 14427 +91
Branches 1863 1885 +22
==========================================
+ Hits 6442 6538 +96
+ Misses 7669 7666 -3
+ Partials 225 223 -2
Continue to review full report at Codecov.
|
|
@CrossR would be good to get your feedback on the approach I took, I basically wanted the versatility to set add a bookmark dir and traverse it a little so that you could say set the |
|
One thing I would quite like (though I'm biased since its what I currently use), is some form of wildcard selection of folders. That way I can have a bookmark of Obviously with the 2 menu way, I can just set the Git folder as the bookmark and then select my project from the second menu, but having it work straight from the first menu would be nice. The other thing would be the second menu would be more useful (to me at least) if it listed all files rather than just the top level directory. My general workflow is swap project > quickopen for the file/files I need. Maybe have an option to auto open the QuickOpen menu and nothing (for people who want to make a new file etc) instead of the second bookmark menu? That then supports three workflows:
That said, there is also times I have wanted something like the second menu to focus the quick open in to a specific folder if I know I don't need anything from the other folders. Perhaps we could add a second command to open up the second menu directly for the current folder, such that if a user did want to restrict the current workspace, its easy to do so? |
|
@CrossR actually looking through the code base I might be duplicating functionality I just saw seems its already partially implemented not sure how to use/access it though? @TalAmuyal would you know what that section of the code base does and if we can use it to surface a bookmarks menu? |
|
Had a look and seems I've definitely done a bit of duplication 🤣 I'll switch to using our implementation in |
use native bookmarks implementation
|
@CrossR I've decided to scope this down quite a lot since I spent quite a lot of time duplicating functionality we already had (my fault for not checking) atm I've basically hooked back up the menu we had which offers the user choices to if the bookmark is a file it opens it if it is a dir it sets it to the active workspace. I tried getting the quickOpen to auto open if the user selects a bookmarked dir but for reasons I cant decipher that doesnt work at all, there must be some block Im not seeing but tbh I think re. time and attention spread across too many PRs Id rather just scope this down to reactivating/activating the menu we have and slightly tidying up its functionality. Re. globs I found a library that can read globs which might be useful for more than just this usecase its called |
|
Makes sense! I'll give it a try and we can add globbing later perhaps. Since I've asked for it, I'll have a look, and also compare it to other editors to see how they achieve it. Just about to checkout the ligatures stuff, then will re-review this. |
|
|
||
| private _getBookmarkPath = (bookmark: string) => { | ||
| const userBaseDir = this._oni.configuration.getValue<string>("oni.bookmarks.baseDirectory") | ||
| const baseDir = userBaseDir || homedir() |
There was a problem hiding this comment.
I think if there isn't "oni.bookmarks.baseDirectory" defined, then we should default to nothing?
Could cause confusion where I tried setting a bookmark of F:\User Files\Documents\Git\oni, which was resolved to C:\Users\ryan\F:\User Files\Documents\Git\uni_work.
Or, could leave this for now and in a follow up PR improve the logic here to be:
- If relative, use base path (either defined, or home dir)
- If absolute, use that always regardless of the value of
"oni.bookmarks.baseDirectory".
I don't mind doing that (along with the globbing), or its also a pretty simple change so we could leave it as a "first issue"?
If you'd rather do that, then I can accept the PR in its current form, since the rest of it works great!
There was a problem hiding this comment.
@CrossR tweaked this to use the absolute path if absolute and relative resolves from a base dir
This is largely inspired by perusing @CrossR's dotfiles a while ago and by the fact that FZF broke for me recently 😆, it uses the
oni.bookmarksto generate a menu with submenus the idea being, the first menu represents the contents ofoni.bookmarkse.g.The first menu will have two items, hitting enter on any will go into the directory and render its content of files and folders, this can be repeated over and over for folders, if a user selects a file it will open it.
There is also a command (
oni.bookmarks.setFolderOrOpenFile) that can be bound to set a folder to the active workspace so if a user e.g. was currently on.dotfiles/vimand hit the bound key it would make the folder the active workspace.