feat: support workspaces config option #155

weskoerber · 2023-07-21T19:14:31Z

This PR adds the following functionality:

"Workspaces" feature:

A list of paths to directories
Ability to switch between workspaces with :ObsidianWorkspace
- This updates paths for :ObsidianQuickSwitch, :ObsidianToday, etc...
If detect_cwd is true, attempt to resolve cwd in workspaces
- If cwd does not exist in workspaces, open cwd as fallback vault
If detect_cwd is false (default), use first workspace as working vault

`:ObsidianWorkspace`:

When called with no arguments, print the current workspace and its path
When called with one argument, try to switch to that workspace

Note: dir is maintained for backward compatibility. If dir is specified, it's used as the default workspace (as long as detect_cwd is false [it is by default]).

To upgrade, move dir from the old config into the workspaces list. For example, if this is your current config:

obsidian.setup({
  dir = '~/Documents/notes',
})

Use this config:

obsidian.setup({
  workspaces = {
    {
      name = 'notes',
      path = '~/Documents/notes',
    }
  },
})

closes: #128

see also: #60, #119

epwalsh

FYI you don't need to update doc/obsidian.txt. That file is generated automatically from the README. So just make those same changes in the README. I recently added a CONTRIBUTING guide that goes over that and some other tips for getting CI to pass.

weskoerber · 2023-07-21T20:38:52Z

I added the workspaces field and kept the dir field.

The idea is that the workspaces is assigned a value by the user when calling obsidian.setup. For example:

require('obsidian').setup({
  workspaces = { 
    '~/vault-1',
    '~/vault-2',
  },
}

Then, we loop through the workspaces table and compare each entry to the current working directory. The first matching value is assigned to dir.

One problem with this implementation is that the behavior is not consistent depending on whether the user passes in dir or not:

If dir is not assigned a value by the user and workspaces contains the current working directory, dir will be assigned the value of the current working directory from workspaces
If dir is assigned a value by the user and workspaces contains the current working directory, the value assigned to dir by the user will be reassigned (by client.ConfigOpts.normalize()) the value of the current working directory from workspaces
If dir is assigned a value by the user and workspaces does not contain the current working directory, dir keeps its assignment
If dir is not assigned a value by the user, and workspaces does not contain the current working directory, dir defaults to the current working directory in obsidian.new()

A potential solution to this inconsistency is to ignore what the user assigns to dir.

However, the last bullet point has me questioning this feature altogether. If dir defaults to the working directory, what's the point of workspaces? Wouldn't falling back to the working directory effectively make any directory a valid workspace?

weskoerber · 2023-07-21T20:39:14Z

FYI you don't need to update doc/obsidian.txt. That file is generated automatically from the README. So just make those same changes in the README. I recently added a CONTRIBUTING guide that goes over that and some other tips for getting CI to pass.

Ahh, I should have read that first...

epwalsh · 2023-07-21T20:52:30Z

Hey @weskoerber, I think dir and workspaces should be mutually exclusive.

If dir is not assigned a value by the user and workspaces contains the current working directory, dir will be assigned the value of the current working directory from workspaces.

In this case I dir should be assigned the corresponding path from workspaces, not necessary the current working directory, which could be a subdirectory of the path from workspaces.

weskoerber · 2023-07-21T22:43:06Z

In this case I dir should be assigned the corresponding path from workspaces, not necessary the current working directory, which could be a subdirectory of the path from workspaces.

Using the following config...

obsidian.setup({
  workspaces = {
    '~/notes',
  },
})

...and assuming the working directory is ~/notes/personal, dir should be assigned ~/notes instead of ~/notes/personal?

What if workspaces is empty, or workspaces does not contain an acceptable value for the current working directory?

Hey @weskoerber, I think dir and workspaces should be mutually exclusive.

My initial idea is that dir and workspaces was not mutually exclusive; if vim.fn.getcwd() matched a value in workspaces, then we'd assign the value to dir, using it as our current vault. Basically, workspaces is a list of vaults, and only one can be "active" at a time (this is pretty much what's described in #119).

Is that different than your expectation? How do you expect the workspaces feature to work?

weskoerber · 2023-07-21T23:22:14Z

I went back to #128 and looked at neorg workspaces. They say workspaces are "basically isolated directories that you can jump between".

My initial hack I wrote up in #128 was just to support opening a directory (e.g. cd ~/notes/personal && nvim) and using that as a vault, essentially setting the dir config option dynamically.

However, I think @sadtab's idea of workspaces were more similar to neorg's workspaces, and my little hack misses that mark.

If we can jump between workspaces (e.g. :ObsidianSwitchWorkspace work), then what's the purpose of dir? (Note: Jumping between workspaces is kinda similar to #60)

epwalsh · 2023-07-24T16:44:14Z

Ok so I think there's really two different features here:

Supporting more than one "static" vault directory.
Dynamically changing the vault dir to cwd.

In my mind workspaces should address (1). I can think of a couple other ways to support (2). For instance:

Allow wildcards / patterns for dir or/and workspaces, and whenever the cwd matches one of those patterns we update the vault dir to cwd.
Add a separate configuration option that activates "dynamic" vault dirs.

sadtab · 2023-08-01T16:39:23Z

I think workspace, dir and vault should be all the same. Let me elaborate the example of neorg. In neorg we have the concept of workspaces, each workspace is just a path to a folder. One of them is always set as default, whether the user sets it explicitly or it will just pick the first one, the point is there is always a Current Workspoce defined.
Any command of Neorg (like new note, search notes etc) is base on the CURRENT workspace, there is even a command to print the current workspace. If the user changes the workspace (:Neorg workspace myAwesomeWorkspace for example) every command (like new note, search notes etc) will be still applied to the current workspace, except the current workspace is changed.

Based on the current implementation of obsidian.nvim, I think the least intrusive approach can be the same. Let the user define a list of vaults and pick one as default, there is always one vault set as the current vault.

Don't bother about the cwd or try to detect the cwd. Because it's complex, bug-prone, and also inconvenient in the very common case where you are working on your source codes and you want just to take a note quickly. The user knows its default vault, if they want to choose another vault, it changes the vault and then takes the note. Usually, a custom keymap will do it conveniently.

PS: I really appreciate what you are doing, my knowledge in lua is very minimal, otherwise I would love to contribute, thanks a lot, truly appreciated

weskoerber · 2023-08-02T20:52:16Z

Thanks for the feedback guys!

@epwalsh:

Ok so I think there's really two different features here:

Supporting more than one "static" vault directory.

Dynamically changing the vault dir to cwd.

I agree, although I think your first point can be split into 2 parts

1a. #128 - support for multiple vaults
1b. #60 - change vault on the fly
2. #119 - use working directory as vault

However, I don't think I understand your second point completely (also #119), because I don't know that it's necessary with 1a and 1b. But I can see the value in changing the vault dir to cwd if the vault dir isn't set with 1a and 1b. Lemme know what you think.

@sadtab:

I think workspace, dir and vault should be all the same.

This was my initial assumption as well. I thought that workspaces would essentially be a list of paths to obsidian vaults, and dir would be the currently selected workspace.

One of them is always set as default, whether the user sets it explicitly or it will just pick the first one

IMHO, a more useful implementation would determine if the current directory exists in the workspaces list. If it does, we use it. However, I don't really know the best action to take if it doesn't exist. Maybe then we can fall back to a default workspace, pick the first one like you said, or just use cwd.

Don't bother about the cwd or try to detect the cwd. Because it's complex, bug-prone, and also inconvenient in the very common case where you are working on your source codes and you want just to take a note quickly.

The user also knows the cwd when neovim is opened, so one could assume that if neovim is opened in a directory that exists in the workspaces list, the user intended to open that vault, rather than the default vault.

Let me know what y'all think!

weskoerber · 2023-08-03T01:54:02Z

I misunderstood #119. The author of that issue said:

set the current working directory to the vault's directory when the plugin is loaded in a subdirectory of the vault

Which is different than your second point.

epwalsh · 2023-08-03T15:06:41Z

Thanks for the continued effort on this @weskoerber :) I'm very busy with my real job this week so I might not be able to give another review until next week.

weskoerber · 2023-08-03T18:16:05Z

No worries, I'm in no rush! I got busy a couple weeks ago too, I know how it goes...

okuuva · 2023-08-06T13:21:03Z

Really looking forward to this! I'm just in the process of setting up Obsidian and the ability to separate work and personal notes is a must have for me..

okuuva · 2023-08-06T13:41:30Z

If default_workspace is set in the config, open that workspace

If default_workspace is not set in the config, and workspaces list contains the cwd, open workspace that corresponds to cwd

If default_workspace is not set, and workspaces list does not contain the cwd, the cwd is used as a fallback

This whole default_workspace business seems overly complicated to me. I just don't see what a separate setting brings to the table over selecting the first workspace from the list as a default. I would assume it's a more useful default over cwd anyway: that way you would have access to your default vault regardless where you happened to open nvim. To me, having to set another config option to achieve that behavior seems like a hassle and having it to default to cwd seems confusing. But maybe that's just me.

weskoerber · 2023-08-08T00:24:30Z

that way you would have access to your default vault regardless where you happened to open nvim

Yep, that's exactly the intended behavior. It's actually not new functionality, just renamed from dir because I thought the name was a bit more clear, although this breaking change is totally unnecessary.

I just don't see what a separate setting brings to the table over selecting the first workspace from the list as a default

The intention of default_workspace was to allow the user to specify a specific vault to open at startup, even if the user opened neovim in a directory that exists in workspaces. If default_workspace wasn't set and we instead used the first item in workspaces, but neovim was opened in a directory that exists in workspaces, then that workspace (essentially cwd) would be opened.

having to set another config option to achieve that behavior seems like a hassle and having it to default to cwd seems confusing

Admittedly, default_workspace is dual-purpose: decide whether we should detect cwd, and set a default vault. Maybe a better approach could add a setting like detect_cwd that controls whether we detect cwd or not:

if true, we detect if cwd is in workspaces and use it as our working vault, otherwise we fall back to the first workspace in workspaces
if false, we fall back to the first workspace in workspaces

okuuva · 2023-08-08T06:54:18Z

having to set another config option to achieve that behavior seems like a hassle and having it to default to cwd seems confusing

Admittedly, default_workspace is dual-purpose: decide whether we should detect cwd, and set a default vault. Maybe a better approach could add a setting like detect_cwd that controls whether we detect cwd or not:
* if true, we detect if `cwd` is in `workspaces` and use it as our working vault, otherwise we fall back to the first workspace in `workspaces`

* if false, we fall back to the first workspace in `workspaces`

Yeah, the dual-purpose was what made it a bit confusing. A boolean for tracking/detecting/following cwd and the logic you described sounds like a much more intuitive approach.

weskoerber · 2023-08-09T00:35:14Z

Lua hash maps are unordered so the order of workspaces may change. This means that the first item in workspaces may differ between consecutive invocations of obsidian.setup(). In order to implement this, I refactored workspaces; instead of using the name of the workspace as the key, I moved the name into the nested table. workspaces is now a "array" (a table indexed with integers, rather than strings). The order of workspaces will now be maintained.

This makes the definition of workspaces a little more verbose, but it makes the implementation cleaner IMO:

obsidian.setup({
  workspaces = {
    {
      name = 'personal',
      path = '~/notes/personal',
    },
    {
      name = 'work',
      path = '~/notes/work',
    }
  },
  detect_cwd = false, -- default
})

okuuva · 2023-08-09T06:31:34Z

Lua hash maps are unordered so the order of workspaces may change. This means that the first item in workspaces may differ between consecutive invocations of obsidian.setup(). In order to implement this, I refactored workspaces; instead of using the name of the workspace as the key, I moved the name into the nested table. workspaces is now a "array" (a table indexed with integers, rather than strings). The order of workspaces will now be maintained.

This makes the definition of workspaces a little more verbose, but it makes the implementation cleaner IMO:
obsidian.setup({
  workspaces = {
    {
      name = 'personal',
      path = '~/notes/personal',
    },
    {
      name = 'work',
      path = '~/notes/work',
    }
  },
  detect_cwd = false, -- default
})

If we want named workspaces then this is the way to go. But is the naming really necessary? Just wondering the use case here and can't figure out anything but easier quick switch using a command. If using a picker like telescope then the name doesn't really matter much.

But the logic is now much easier to follow and I agree, the implementation looks much clearer as well. Kudos!

epwalsh

Thank you all for the discussion! I just have a few minor requests and then I think we can get this merged ✅

epwalsh · 2023-08-14T16:02:41Z

lua/obsidian/init.lua

-  self.dir = Path:new(vim.fs.normalize(tostring(opts.dir and opts.dir or "./")))
+
+  self.current_workspace = obsidian.workspace.get_from_opts(opts)
+  self.dir = Path:new(self.current_workspace.path)


I think we should still normalize dir to avoid breaking changes.

Suggested change

self.dir = Path:new(self.current_workspace.path)

self.dir = Path:new(vim.fs.normalize(tostring(self.current_workspace.path)))

Hi! I finally have time to swing back around on these...

The workspace paths should already be normalized.

Here, in workspace.new(), the path is normalized:

workspace.new = function(name, path) local self = setmetatable({}, { __index = workspace }) self.name = name self.path = vim.fs.normalize(path) return self end

Paths are also normalized when reading from the config in client.ClientOpts.normalize():

for key, value in pairs(opts.workspaces) do opts.workspaces[key].path = vim.fs.normalize(tostring(value.path)) end

There could be an edge-case I haven't fully considered, but I think it's safe to omit the redundant path normalization. However, if you still prefer normalizing the paths where you suggested I won't argue with you 😄

epwalsh · 2023-08-14T16:03:22Z

lua/obsidian/command.lua

+  client.current_workspace = workspace
+
+  echo.info("Switching to workspace '" .. workspace.name .. "' (" .. workspace.path .. ")", client.opts.log_level)
+  client.dir = Path:new(workspace.path)


Same suggestion here about normalizing:

Suggested change

client.dir = Path:new(workspace.path)

client.dir = Path:new(vim.fs.normalize(tostring(workspace.path)))

See my comment in a previous suggestion

Ok fair enough! Could you add a comment here at least:

Suggested change

client.dir = Path:new(workspace.path)

-- NOTE: workspace.path has already been normalized

client.dir = Path:new(workspace.path)

Fixed with 598bc0a

epwalsh · 2023-08-14T16:04:40Z

lua/obsidian/workspace.lua

+workspace.get_workspace_from_cwd = function(workspaces)
+  local cwd = vim.fn.getcwd()
+  local _, value = next(vim.tbl_filter(function(w)
+    if w.path == cwd then


We might need to normalize these paths before comparing them, otherwise it seems a bit brittle.

See my comment in a previous suggestion

weskoerber · 2023-08-18T19:30:23Z

Thank you all for the discussion! I just have a few minor requests and then I think we can get this merged ✅

Sorry for the delay. I'll have some time this weekend to address these concerns!

MathieuDR · 2023-09-16T13:41:23Z

I would love this feature, what's the state of this? @weskoerber

I would like to thank you already for your work!

john-okeefe · 2023-10-11T00:05:07Z

Is there any update on this? I'm really excited for this feature!

sebassdc · 2023-10-12T20:53:23Z

Looking forward! great work on this!

- when called with no arguments, print the current workspace name and path - when called with a single argument, try to switch to the workspace

epwalsh

Thanks for getting back to this @weskoerber! Just a couple more comments

epwalsh · 2023-10-17T16:50:30Z

doc/obsidian.txt

This file gets updated automatically from the README. So could you please update the README instead?

Ugh sorry, I might have resolved conflicts in this file poorly when I rebased.

Fixed with 48ae1f1

epwalsh · 2023-10-17T16:52:47Z

lua/obsidian/command.lua

+  client.current_workspace = workspace
+
+  echo.info("Switching to workspace '" .. workspace.name .. "' (" .. workspace.path .. ")", client.opts.log_level)
+  client.dir = Path:new(workspace.path)


Ok fair enough! Could you add a comment here at least:

Suggested change

client.dir = Path:new(workspace.path)

-- NOTE: workspace.path has already been normalized

client.dir = Path:new(workspace.path)

epwalsh · 2023-10-17T16:56:07Z

lua/obsidian/init.lua

@@ -37,7 +41,7 @@ end
 ---@return obsidian.Client
 obsidian.new_from_dir = function(dir)
  local opts = config.ClientOpts.default()
-  opts.dir = vim.fs.normalize(dir)
+  opts.workspaces = vim.tbl_extend("force", { obsidian.workspace.new("test_vault", dir) }, opts.workspaces)


Maybe name it by the name of the directory instead of "test_vault"?

Fixed with 1e1ba52

epwalsh · 2023-10-17T16:58:23Z

lua/obsidian/config.lua

I would like to keep backwards compatibility for when people upgrade. This should be fairly easy:

Just check if dir is set instead of workspaces and convert dir into a workspace object.

Fixed in f422f98

- names the workspace the name of the dir

- previously using obfuscatory "test_vault"

- if `dir` is set, convert it to a workspace object

epwalsh · 2023-10-17T19:22:22Z

lua/obsidian/config.lua


 local config = {}

 ---[[ Options specs ]]---

 ---@class obsidian.config.ClientOpts
---@field dir string
+---@field dir string|?
+---@field workspaces table


I think this has the obisidan.Workspace type, right?

Suggested change

---@field workspaces table

---@field workspaces obsidian.Workspace

It's a table of obsidian.workspaces:

require('obsidian').setup({ -- ... workspaces = { { name = 'personal', path = '~/Documents/notes/personal', }, { name = 'work', path = '~/Documents/notes/acsd', }, } -- ... )}

Ah, right, then we can give it a type hint like this

Suggested change

---@field workspaces table

---@field workspaces obsidian.Workspace[]|?

- if `dir` is set, use that as the default workspace - even if workspaces are defined, `dir` is used as the default

epwalsh

LGTM! Thanks for getting this over the finish line @weskoerber :)

epwalsh · 2023-10-18T19:01:00Z

lua/obsidian/config.lua


 local config = {}

 ---[[ Options specs ]]---

 ---@class obsidian.config.ClientOpts
---@field dir string
+---@field dir string|?
+---@field workspaces table


Ah, right, then we can give it a type hint like this

Suggested change

---@field workspaces table

---@field workspaces obsidian.Workspace[]|?

weskoerber · 2023-10-18T19:26:12Z

Ah, right, then we can give it a type hint like this

I didn't know you could type hint like that, thanks for the tip!

LGTM! Thanks for getting this over the finish line @weskoerber :)

Right on! I appreciate you being patient with me and guiding me through this PR!

epwalsh reviewed Jul 21, 2023

View reviewed changes

weskoerber force-pushed the feat/multiple-vaults branch from 21ba6c1 to 9b3e415 Compare August 3, 2023 02:01

weskoerber force-pushed the feat/multiple-vaults branch from 8d119d9 to bc6b82b Compare August 4, 2023 20:11

weskoerber marked this pull request as ready for review August 4, 2023 21:33

weskoerber marked this pull request as draft August 4, 2023 21:35

weskoerber marked this pull request as ready for review August 4, 2023 22:12

weskoerber force-pushed the feat/multiple-vaults branch from a4f40a8 to f40c132 Compare August 8, 2023 23:38

epwalsh reviewed Aug 14, 2023

View reviewed changes

epwalsh mentioned this pull request Sep 11, 2023

Multi vault support #190

Closed

weskoerber added 13 commits October 16, 2023 18:11

chore: sync doc changes with main

8d71474

chore: maintain current workspace in client

c5a3a8f

refactor: workspace command changes

170dddb

- when called with no arguments, print the current workspace name and path - when called with a single argument, try to switch to the workspace

chore: commit stylua changes

9c19046

chore: update readme

591a116

fix: default_workspace type in readme

d780e10

chore: workspace class

acd5f23

refactor: use workspace class

a5931ba

fix: run luacheck and stylua

c31a654

chore: update readme

d3a93c5

fix: passing wrong table to vim.tbl_extend

417eb91

chore: clarify table type in workspace class

c741398

chore: add workspace tests

46b4a83

weskoerber force-pushed the feat/multiple-vaults branch from 825ac0d to 46b4a83 Compare October 16, 2023 22:13

epwalsh reviewed Oct 17, 2023

View reviewed changes

weskoerber added 7 commits October 17, 2023 14:10

chore: add comments clarifying workspace path normalization

598bc0a

chore: revert changes to doc/obsidian.txt to upstream/main

48ae1f1

chore: add function to create new workspace from dir

e190304

- names the workspace the name of the dir

chore: default workspace name to the name of the directory

1e1ba52

- previously using obfuscatory "test_vault"

fix: backward incompatibility

04dd838

- if `dir` is set, convert it to a workspace object

chore: run stylua and luacheck

852895f

refactor: use dir name for default vault instead of "."

79118e1

epwalsh reviewed Oct 17, 2023

View reviewed changes

weskoerber and others added 4 commits October 17, 2023 16:06

fix: backward incompatibility

f422f98

- if `dir` is set, use that as the default workspace - even if workspaces are defined, `dir` is used as the default

chore: update readme

9402d59

fix: failed test using wrong workspace name for current directory

bc228a0

update type hints

242fdbe

epwalsh approved these changes Oct 18, 2023

View reviewed changes

epwalsh merged commit 1194d4c into epwalsh:main Oct 18, 2023
5 checks passed

	self.dir = Path:new(self.current_workspace.path)
	self.dir = Path:new(vim.fs.normalize(tostring(self.current_workspace.path)))

	client.dir = Path:new(workspace.path)
	client.dir = Path:new(vim.fs.normalize(tostring(workspace.path)))

	client.dir = Path:new(workspace.path)
	-- NOTE: workspace.path has already been normalized
	client.dir = Path:new(workspace.path)

	---@field workspaces table
	---@field workspaces obsidian.Workspace

	---@field workspaces table
	---@field workspaces obsidian.Workspace[]\|?

feat: support workspaces config option #155

feat: support workspaces config option #155

Conversation

weskoerber commented Jul 21, 2023 • edited Loading

"Workspaces" feature:

:ObsidianWorkspace:

epwalsh left a comment

Choose a reason for hiding this comment

weskoerber commented Jul 21, 2023

weskoerber commented Jul 21, 2023

epwalsh commented Jul 21, 2023

weskoerber commented Jul 21, 2023 • edited Loading

weskoerber commented Jul 21, 2023 • edited Loading

epwalsh commented Jul 24, 2023

sadtab commented Aug 1, 2023 • edited Loading

weskoerber commented Aug 2, 2023 • edited Loading

weskoerber commented Aug 3, 2023

epwalsh commented Aug 3, 2023

weskoerber commented Aug 3, 2023

okuuva commented Aug 6, 2023

okuuva commented Aug 6, 2023

weskoerber commented Aug 8, 2023

okuuva commented Aug 8, 2023

weskoerber commented Aug 9, 2023 • edited Loading

okuuva commented Aug 9, 2023

epwalsh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weskoerber commented Aug 18, 2023

MathieuDR commented Sep 16, 2023

john-okeefe commented Oct 11, 2023

sebassdc commented Oct 12, 2023

epwalsh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epwalsh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weskoerber commented Oct 18, 2023

weskoerber commented Jul 21, 2023 •

edited

Loading

`:ObsidianWorkspace`:

weskoerber commented Jul 21, 2023 •

edited

Loading

weskoerber commented Jul 21, 2023 •

edited

Loading

sadtab commented Aug 1, 2023 •

edited

Loading

weskoerber commented Aug 2, 2023 •

edited

Loading

weskoerber commented Aug 9, 2023 •

edited

Loading