-
Notifications
You must be signed in to change notification settings - Fork 672
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
Create a proposal for the "new note" fallback chain #683
Conversation
64914f8
to
09c4fb4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some thoughts, let me know if I understood this correctly
* Consistency: with the behaviour of the existing template commands. | ||
* Flexibility: Users could use variables like `$FOAM_TITLE` (and eventually other snippet variables) in the path. | ||
|
||
### Phase 4: Match `SAME_AS_ACTIVE_NOTE` behaviour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if Phase 4, 5, 6 would be better described asa options of Phase 3. The same thing you do in Phase 2.
* This behaviour matches `<workspace>/.foam/templates/new-note.md` behaviour | ||
* **Option B**: Relative to `foam.newNoteDirectory` | ||
* I suspect is what you'd expect if you've already defined `foam.newNoteDirectory` | ||
* **Option C**: Both. Relative to `foam.newNoteDirectory`, only when `foam.newNoteDirectory` is defined. Otherwise, relative to `<home_dir>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you follow the foam.newNoteDirectory
resolution from Phases 3, 4, 5, 6, we could at this level simply go with Option B, which would basically include Option C.
mmm.. not sure I am making sense.
Basically, a relative filepath
would be matched against the resolved foam.newNoteDirectory
.
foam.newNoteDirectory
is resolved using the cases from Phase 3, 4, 5, 6 - and those are a superset of Phase 2 -> Option C
.
Let me know if this makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While Phase 6 is <home_dir>
, it is really included just to handle the extreme edge case of having neither a file, nor a workspace open. If we resolve foam.newNoteDirectory
from Phase 3-6, I expect that it would almost always get resolved by Phase 4, which changes the behaviour from what is currently described.
(i.e. Phase 4 "gets in the way", and is probably not the behaviour we want (creating a note relative to whatever you happen to have open)
I think you raise a good point to think about: Perhaps the resolution could be separated into two parts:
- Resolving from the config files, falling back to
foam.newNoteDirectory
- Resolving
foam.newNoteDirectory
I'll give it some more thought, but I'm not convinced yet.
Potential changes to Phase 2 ("Option D")
I think the desired fallback behaviour of a relative path in Phase 2 might be:
Similar to being relative to Phases 3, 5, 6 (note: not Phase 4):
- Similar to Phase 3: Relative to
foam.newNoteDirectory
- Similar to Phase 5: Relative to Workspace
- Similar to Phase 6: Relative to
<home_dir>
This would just affect Phase 2 with a relative path. In other cases, we'd we continue as described, doing Phase 3-6 (including 4)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
In phase 3, how does a user set
SAME_AS_ACTIVE_NOTE
andWORKSPACE_ROOT
?
are those presented as "options" or "variables? -
an interesting things about phase 2 resolution is that you could have variables that only work in certain context (e.g.
WORKSPACE_ROOT
), I guess in that case they simply don't get expanded or, better, we can error out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not proposing the addition of SAME_AS_ACTIVE_NOTE
and WORKSPACE_ROOT
into Foam, just equivalent behaviour as those.
I've reworded Phase 4 and 5 to be clearer in that regard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand, my question was about how the user could be deliberately set foam.newNoteDirectory
to "same as active note" or "workspace root", is this a supported case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm... 🤔 ... good question...
foam.newNoteDirectory
is defined as matching the spec used for filepath
in the templates.
Once I convince VS Code to give extensions better snippet APIs, we'll be able to use snippet variables in these filepath
s, including:
TM_DIRECTORY
: The directory of the current documentWORKSPACE_FOLDER
: The path of the opened workspace or folder
(Which work today in the rest of the snippets)
So at that point, users would be able to use those to get these behaviours.
SAME_AS_ACTIVE_NOTE
->"foam.newNoteDirectory": "$TM_DIRECTORY/$FOAM_TITLE"
WORKSPACE_ROOT
->"foam.newNoteDirectory": "$WORKSPACE_FOLDER/$FOAM_TITLE"
8687678
to
26c29b2
Compare
@riccardoferretti Here's a reformulation. Let me know what you think. This is the same logic as described in the original proposal, but fills in the edge cases that were glossed over in the original proposal:
Round 2In order to create a note, we need 3 components:
Together, the
So the question is how to determine the other components in each case. This is somewhat precise to communicate, so I found that English was more verbose than I wanted. Pseudocode disclaimers:
Pseudocodefunction newNoteInformation(fallbackContent: string, fallbackFilename: string) {
let contents: string, filepath: Uri;
// Try to get the results from the workspace's template
[content, filepath] = loadFilepathFromTemplate('<workspace>/.foam/templates/new-note.md');
// Note: We don't want to use the `filepath` from the <home_dir> template
// if we've already loaded the content from the <workspace> template.
if (!content) {
// Try to get the results from the <home_dir>'s template
[content, filepath] = loadFromTemplate('<home_dir>/.foam/templates/new-note.md');
}
// Resolve relative paths
// Note: If the filepath came from a template, we don't want to fall through to
// activeTextEditorDirectory, which is why this block exists separate from the one below.
if (filepath && !isAbsolute(filepath)) {
const directory = workspaceDirectory || homeDirectory;
filepath = Uri.joinPath(directory, fallbackFilename);
}
// Fallback to loading filepath from the config setting
filepath ||= resolveSnippetVariablesIn(configuration.get('foam.newNoteDirectory'));
// Fallback further and resolve relative paths
if (!filepath || !isAbsolute(filepath)) {
const directory = activeTextEditorDirectory || workspaceDirectory || homeDirectory;
filepath = Uri.joinPath(directory, fallbackFilename);
}
return [content, filepath];
} |
I think this makes a lot of sense, I have slightly reworded it (the main difference I think is around function newNoteInformation(fallbackContent: string, fallbackFilename: string) {
let contents: string, filepath: Uri;
// Try to get the results from the workspace's template
[content, filepath] = loadFilepathFromTemplate('<workspace>/.foam/templates/new-note.md');
// Note: We don't want to use the `filepath` from the <home_dir> template
// if we've already loaded the content from the <workspace> template.
if (!content) {
// Try to get the results from the <home_dir>'s template
[content, filepath] = loadFromTemplate('<home_dir>/.foam/templates/new-note.md');
}
// if no template is found, use the default content
content ||= defaultContent;
// if filepath is not defined, check variable
filepath ||= resolveSnippetVariablesIn(configuration.get('foam.newNoteDirectory'));
// if filepath variable is not defined, use default
filepath ||= fallbackFilename
const directory = areWeInWorkspace
? workspaceDirectory || homeDirectory
: activeTextEditorDirectory || workspaceDirectory || homeDirectory;
filepath = isAbsolute(filepath) ? filepath : Uri.joinPath(directory, filepath);
return [content, filepath];
} |
@riccardoferretti We're in agreement with the first bunch of lines: function newNoteInformation(fallbackContent: string, fallbackFilename: string) {
let content: string, filepath: Uri;
// Try to get the results from the workspace's template
[content, filepath] = loadFilepathFromTemplate('<workspace>/.foam/templates/new-note.md');
// Note: We don't want to use the `filepath` from the <home_dir> template
// if we've already loaded the content from the <workspace> template.
if (!content) {
// Try to get the results from the <home_dir>'s template
[content, filepath] = loadFromTemplate('<home_dir>/.foam/templates/new-note.md');
}
// if no template is found, use the default content
content ||= fallbackContent; But it seems that you have changed the semantics of the latter portion with the refactor.
Which is laudable. But I don't think what is written is what we want. const directory = areWeInWorkspace
? workspaceDirectory || homeDirectory
: activeTextEditorDirectory || workspaceDirectory || homeDirectory; Which resolution/fallback chain to use doesn't depend on whether we are in a workspace or not, but rather whether the filepath came from a template I didn't find a nicer way to know whether the filepath came from a template other than having the two blocks there. |
Yeah, the later part is what I am struggling with. I think I understand your comment, let me see if I can rewrite it in such a way that it feels clearer to me an aligned with your thoughts. I think part of the challenge is that we are sometimes using Let me try again with a few points.
function newNoteInformation(fallbackContent: string, fallbackFilename: string) {
let contents: string, filepath: Uri;
// Try to get the results from the workspace's template
[content, filepath] = loadFromTemplate('<workspace>/.foam/templates/new-note.md');
// Note: We don't want to use the `filepath` from the <home_dir> template
// if we've already loaded the content from the <workspace> template.
if (!content) {
// Try to get the results from the <home_dir>'s template
[content, filepath] = loadFromTemplate('<home_dir>/.foam/templates/new-note.md');
}
// if no template is found, use the default content
content ||= defaultContent;
// Resolve relative paths
// Note: If the filepath came from a template, we don't want to fall through to
// activeTextEditorDirectory, which is why this block exists separate from the one below.
if (filepath) {
if (!isAbsolute(filepath)) {
const basedir = workspaceDirectory || homeDirectory;
filepath = Uri.joinPath(basedir, fallbackFilename);
}
} else {
// Fallback to loading filepath from the config setting
filepath = resolveSnippetVariablesIn(
configuration.get('foam.newNoteDirectory') ?? fallbackFilename
);
if (!isAbsolute(filepath)) {
const basedir = activeTextEditorDirectory || workspaceDirectory || homeDirectory;
filepath = Uri.joinPath(basedir, fallbackFilename);
}
}
return [content, filepath];
}
|
hi @movermeyer what's your thinking around this PR/proposal? |
**Changes from the initial proposal:** * Renamed `foam.newNoteDirectory` to `foam.rootDirectory` * It's not a filepath, but a directory. * It can no longer accept snippet variables * I can't imagine a scenario where you'd want snippet variables in this * But if such a scenario comes about, we could add it then. * Relative path fallback in Phase 2 no longer considers the active note's directory (as it did in Option D; now only the workspace and homedir) * Assertion: A user will never want to define a path relative to whatever file they happen to have open. * All references to `<home_dir>` (esp. Phase 6) changed to `foam.rootDirectory ?? <home_dir>`
Yeah, looking back, I think that was just a typo. 😅
Yeah. @riccardoferretti The silver lining of coming back to this after a month is that I can see the proposal from a different perspective. I've given it another review and adjusted it after asking myself "What value does each one of these Phases solve?" I've added another commit to update the Markdown proposal document to reflect these changes, which I also describe below. Round 3Goals
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking of the root dir is definitely the right way to go, made me think of a couple more things that I have left in the comments
|
||
This document will guide the development direction that each note-creating-command will eventually be modified to follow. | ||
|
||
Note that throughout this doc, `foam.rootDirectory` is used as a placeholder name to denote a new Foam setting that we would add. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renaming this foam.rootDirectory
makes a world of difference, totally changes the mental model around it - and I agree 100% it's the right way to go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that mindset, I wonder whether we can say we always have:
foam.rootDirectory
, which is the root directory for Foam, and is used e.g. to read config, templates, ...foam.notesRootDirectory
, which is the root directory when scanning for notes, or computing relative paths, ...
- if we are in a VS Code workspace:
foam.rootDirectory
is<workspace>
- and
foam.notesRootDirectory
defaults to<foam.rootDirectory>
(but can be defined in the config -<foam.rootDirectory>/.foam/config.json
or~/.foam/config.json
)
- if we are not in a VS Code workspace:
foam.rootDirectory
is<home_dir>
- and
foam.notesRootDirectory
defaults to<foam.rootDirectory>/foam
(but can be defined in<foam.rootDirectory>/.foam/config.json
)
Once we can assume that these variables are always defined, things should get simpler with the resolution.
E.g. relative paths are always relative to foam.notesRootDirectory
.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full reply is here: #683 (comment)
Q: Why is there a different defaulting behaviour for foam.notesRootDirectory
between the "workspace" and "global" levels?
If there's a need to add a foam
subdirectory, shouldn't it be applied at both levels for consistency? (Aside/nit: default to notes
instead of foam
?)
* Progressive UX: Users start with sensible defaults, then can customize as their usage of Foam gets more sophisticated. | ||
|
||
## Proposal: "New note" path resolution | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add the pseudo-code from the PR (or a mermaid flow chart) in this doc, to summarize the flow (the sections can still be kept to go into deeper detail and share the rationale around each step).
* A relative path in `filepath` in `<workspace>/.foam/templates/new-note.md` should be relative to the workspace. | ||
* **Justification:** Allows users to have customized templates in a given workspace. | ||
* An absolute path in `filepath` in `<workspace>/.foam/templates/new-note.md` should just use that absolute path. | ||
* **Justification:** Allows uses to have their notes for a project inside another repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* **Justification:** Allows uses to have their notes for a project inside another repository. | |
* **Justification:** Allows users to have their notes for a project inside another repository. |
@riccardoferretti I think I've convinced myself that splitting I'm out of time right now, but I will definitely need to do another round of changes to the document. I'm missing the context around // TODO this is still to be improved - foam config should
// not be dependent on vscode but at the moment it's convenient
// to leverage it But I don't know the "Why?" Q: Would
Q: Why do you need a Taking a guess as to why you introduced
Conceptually, I think I want:
This feels like we need a table of user stories to be able to verify the behaviour is reasonable in all scenarios:
I've filled it in with the behaviour as written in the proposal document today (commit Phase 3 as currently defined is perhaps too "strong": As soon as they define an explicit
It's kind of weird that This all seems to indicate that it might make sense to define a
Rename it to something else; it isn't specific to the "root" level, so perhaps drop that?: I really have to write up another draft of the proposal with this new concept, and redo the scenarios chart. I'm out of time for now, but dropping this here to have it recorded. |
Foam is conceived to work as a stand-alone library, and with various clients, amongst which VS Code. |
It is partially for the reason you stated, that users might want to have their Foam repo to be in a subdir (e.g. The main reason why I introduced the concept though was to create a reliable base for workspace relative paths.
In the non VS Code workspace case above, I feel we still need the concept of root for notes (e.g. for computing relative paths), but to make that the user dir sounds weird. So we would need to define one.
I think this is basically the same concept but without giving such path a name? Let me know if it's still unclear, to me it feels right but I might be missing something or not making the point come across (which is itself a yellow flag) |
Regarding your scenarios, I think this is where things become much simpler:
The only open question I can see is if someone who has opened a file (and not a folder) should be considered to be in a workspace that has Thoughts? |
Good point,
if relative, it is relative to
The default for me changes if we are in a workspace (at which point it defaults to Makes sense? |
@riccardoferretti I think its accurate to say you've been trying to model For example, if we always resolve
That is not how I am modelling it. I am treating it as a user setting (like any other) that controls the location of the I know that I started this issue discussing the "new note" fallback chain, but I think this topic is actually more general: How Foam should resolve its configuration settings?. As such here is my new proposal for resolving all Foam configuration values. In summary, each configuration setting will be resolved by falling through a hierarchy of "scopes", stopping at the first scope that resolves a value for the setting.
Pseudocode: function resolveConfigSetting(settingName: string) -> Optional<string> {
return scopes.find(scope => scope.resolveConfigSetting(settingName))
} Claim: I believe this fallback chain unifies the behaviour of configuration value resolution and allows the flexibility that allows advanced users to customize their workflows, while maintaining sensible defaults throughout the user maturation process. It provides a consistent interface in the code for accessing any config value, and maintains the ability to make use of a client's native configuration mechanisms. The case of new note filepath resolutionFinding any note templates follows the same fallback path as any setting, first checking for the template body in:
Aside: If there are "just any other setting", we should probably give the template bodies official settings names: (Undefined: What if Once you have the template body, you parse it to pull out the
The
|
@riccardoferretti and I chatted in real-time and realized that I was getting hung up on trying to handle all the scenarios automatically, which was causing complexity. The breakthrough was the reminder that we could ask the user to confirm our best guesses for note locations in the scenarios where we aren't certain. As a result, I have drafted a design that uses a single
Use cases
ScenariosThere are many many scenarios, which is part of the reason why it was so hard to reason about. 5 Editor states
9 resolved value states When determining where to create a new note, we care about 2 resolved values: There are three possible states for each of them: Together that makes 3 To make sure that we had a sensible answer to each of them, I wrote a little script to output all the states and manually verified that the results looked correct.
|
I agree with that decision. A note on the editor states:
I assume there is a type and you meant: I had some thoughts on the algorithm, but while working on it I came up with the point below, so let me hold those thoughts for myself for now, to avoid confusion. Yet there is something that doesn't feel right in the resolution complexity. If the two of us are struggling so much with it, what are the odds a user would ever understand it and know how to progress up? I wonder if the path here could be simplification via flattening the steps. Based on the following premises:
When a new note is created, we look up the
We populate the default with some best estimate, more or less based on your algorithm above, and we explain in the description of the input box that the reason why we are asking is because some variables could not be resolve (which helps the user learn how the system works) I understand this is possible a drift from the original approach, but I feel the complexity of the resolution is replaced by something that is much more "transparent" from the user's POV. What are your thoughts? |
Yes. Fixed.
I actually think that the mental model that a user has to have is pretty simple, and it is incremental to match their "Foam maturity". Here is what a user's mental model is as they grow in their Foam maturity: Stage 1: New userNew users have nothing configured. Their mental model is: "New notes are created beside my currently open note" Stage 2:
|
I like it. I feel that our ideas are getting very close to each other. Questioning backwards compatibility
(Note: I added I don't think we even need to break backwards compatibility here? When does
Given i.e., I don't think backwards compatibility gets broken? Resolution of the template file
If the template doesn't contain a
Do we actually need
|
Backwards compatibility
The problem here is that we go back to the fallbacks... what if we are not in a workspace? We could prompt the user for confirmation, but why not simply say "hey, this is the way you have to define a template filepath". It's also worth noting that by this point the user is fully onboarded in Foam, so it's a fair request for them to do a bit extra work ;) I should also say that I am not super concerned about backwards compatibility here, because we wouldn't be changing behavior, we would simply tell the user to update the configuration so that we can univocally resolve the path - I consider it a very minor nuisance. Basedir
You are possibly right, and I am not very fussed around this. New notes directory
I strongly believe that the onboarding experience is better this way, I think a setting along these lines (but not quite what we originally thought of, see below) is quite important, as I have heard several comments/questions/issues raised around the location of new notes. I have a feeling this is where 90% of users will settle on. My thinking here:
Code simplicityThere is a point I haven't explicitly made above: by using the variables approach we simplify, possibly quite considerably, our code (and hence its maintenance). Compare "just resolve the variables, is this absolute?" vs "if we are in workspace, do this; if we are with just a file open, do that; ...". Same approach about the "new notes directory": we use that setting only if you don't have a filepath property defined, otherwise it's ignored. To me it feels easier to explain, test, wrap one's head around, and maintain. SummaryI think we are converging, I agree. There are a couple of side loops to wrap but the spiral is closing in :) |
Just curious if theirs been any progress on this feature? I just started using Foam and am already in love, but I would really like to set a directory for newly created notes, lol. Thanks for all your hard work! |
hi @chris-patenaude - yes I need to give this a higher priority, it's been a bit on the backburner. Long story short, I am thinking of having a path that can be defined as a property or in the template which must resolve to an absolute path. If the resulting path is not absolute, a confirmation is asked to the user. One way you can help achieve that is by helping getting 20 upvotes (if you have friends that use Foam please ask them too ;) ) on this issue with VS Code: I am hoping to come back to this issue soon(-ish), its priority in any case is high |
The call to arms has gone out and we've reached 20 up votes for microsoft/vscode#155868! 🥳 |
Closing for lack of activity and because in the meantime a few changes have mitigated the issue (although likely not completely resolved) |
My work on allowing templates in the user's home directory,
Has forced me to consider what the complete new note creation fallback chain ought to be. I realized that I really need to nail it down so that I could enforce consistency between all of Foam's methods of creating notes.
Here's what I've managed to come up with. Feel free to criticize it in any way.
It's probably easiest to view the rendered version of this document.
Let me know what you think so I can get the commands all switched over to use it.
@riccardoferretti