mdelobelle / metadatamenu

For data management enthusiasts : type and manage the metadata of your notes.
https://mdelobelle.github.io/metadatamenu
MIT License
520 stars 27 forks source link

Breaking API and behiavior changes from around the Properties Obsidian update #499

Open RicSanOP opened 9 months ago

RicSanOP commented 9 months ago

Hello @mdelobelle,

Thank you for making this very extensive plugin that works with Obsidian metadata. I would consider myself a bit of an Obsidian poweruser, having written tons of JS scripts and custom plugins mods that allow me to work with my vault in a very automated and streamlined way. I am writing to submit a complaint about some changes that you've made to Metadata Menu in the hopes that you may reconsider some of your philosophies on metadata management in personal knowledge management systems.

The main change that you made that completely broke a lot of my code which used your API is that your plugin now only indexes and manages metadata fields that have been explicitly set within some fileClass (or within the app settings). My main usage of your plugin's API was the fileFields and postValues functions which allowed me to edit multiple metadata fields in a given markdown file at once (something that your main "competitor" Metaedit does not handle gracefully in its API at the moment). Now the obvious solution is to add the fields I need to the settings or a global fileClass (which works in my scenario since I choose unique field names per type and therefore have no need for hierarchical fileClass structures or different IDs for fields with the same name), but each time I add a new field through the modal it just leads to a super long suspension freeze of Obsidian (I imagine some indexing process). Nevertheless, I resolved the issue by manually editing the fileClass YAML.

The other major breaking change is the shift to using a unique ID system as opposed to field names, which has altered some of the API. I will admit that the documentation is lacking information on IDs and indexedPath elements, so perhaps future versions will provide some clarity. However, what I currently see is that fileFields will only return explicitly set fields with an ID and return a dictionary keyed on those arbitrary IDs. I was able to circumvent the issue by setting the id fields on the fileClass YAML to equal the name of the field, but I hope i can suggest an overall solution for my prior complaints.

I see that in your indexing process a Note object is built that is composed of Line and LineNode objects which parse their respective parts of a given markdown file and extract the relevant YAML and DV fields which are ExistingField objects. I also see that at some point in the fileFields API call, the Note object is built and then the ExistingField objects are matched to their associated preset Field and its value, thus ignoring any fields which have not been previously set in either the settings or another fileClass (because they don't have an ID I guess).

I would honestly suggest that the fileFields and postValues API functions be reverted to using field names (or perhaps have a fielClass ignore mode that simply returns all fields in the note as input type fields with their names as the ID key in the dictionary returned). My main reasons for this recommendation on the API level are that field names are more direct and intuitive to work with (the user does not need to know about how fields and field types are organized internally within fileClass hierarchies etc) and because at a note level granularity (which these functions operate at) there is no ambiguity about which variation of a field with the same name should be used. Correct me if i am wrong, given your fileClass precedence rules ( 1. fileClass value in the frontmatter 2. Tag match 3. Path match 4. Bookmark group match 5. fileClassQuery match 6. Global fileClass 7. settings preset fields ), there is no situation where 2 fields of the same name in the same note would ever be identified with 2 different field ID.

Overall, I am simply a user who probably unlike most users, wants to automatically manipulate any and all fields in my vault. Therefore this field presetting requirement has somewhat forced me into your fileClass setting features, which after reading through them in the documentation are extremely extensive and well thought out, but I don't think I should be forced to use them in order to use basic field editing functions such as fileFields and postValues. I mean no offense at all with this issue posting. I simply hope I can convince you to allow a change that could unbreak a lot of the usefulness your plugin's API once provided. Please let me know your thoughts at your earliest convenience, I hope to hear from you soon.

mdelobelle commented 9 months ago

Hi! Yes you're right, I knew this would break. I have an explanation why and an idea to suit your needs.

Explanation Metadata Menu's purpose is to propose a robust data schema for metadata in Obsidian. While having unique names by fileclass was fine with me, I found it limitating going forward. I've started to include more and more nested properties in my YAML section which weren't typed and therefore not manageable by MDM. I really wanted to enable a fully type-able nested structure inside each fileclass to avoid having too many fileclass inheriting from each other. Not to mention that multiple inheritance and mixins would have been a nightmare to manage... That's the point in time where having multiple fields with the same name became a thing: some fields in different hierarchies of a fileClass can have the same name, for example in a Project fileclass, "status" may be a root field, and "status" may be a child field of a root Object field called "design_phase", and we can also have a "status" that is a child of another root Object field called "develop_phase" etc.... That's where I needed field Id to differentiate fields with the same name, and "path" to define their position in the hierarchy. IndexedPath is also defined for ObjectList field items. There's a complex mechanism to identify objectList items and object items that dataview can't provide because it simply doesn't give you the line where each field is found (what MDM can do now).

So I've adapted postValues (that uses indexedPath even though the key is named id...) and fileFields to match these changes. Since postValues is used in many places of the code it will be difficult to revert. Moreover, I aim at making Id-based field management the mainstream way of doing things with mdm.

Proposal That being said the previous fileFields and postValues are pretty standalone methods and would still work with the current fileClass definition, but they would only match type with root fields. Therefore I could reintroduce them but with different name, for example:

What do you think?

RicSanOP commented 9 months ago

Hello @mdelobelle ,

Thank you for your rapid response. I now understand how a single fileClass may have multipcle fields with the same name due to object and list fields within the YAML of a note. On another note, I am assuming you are referring to the fileFields function when you say fieldsValues.

After seeing your response, I think the fundamental issue is the disconnect or lack of differentiation between id and indexedPath (kindly note that the documentation does not document what an indexedPath is. the hyperlink on the API page leads to a broken webpage).

If I were to imagine what indexedPath is with your example, then i could imagine it being something like .status , .design_phase.status and .develop_phase.status for the fields mentioned in your example. Do correct me if I am wrong, but if this is actually what I imagine it to be then with a slight tweak, it should be sufficient to solve the problems with fileFields and postValues API functions.

Please note that currently there is a disconnect between what your documentation states about the key of fileFields return object and what is currently happening. On your documentation on this API function, it states that the indexedPath is the key (which is described as /* the unique idenfier of the path of this field in this file*/), however the key is currently the id (which is described as /* the unique identifier of the field definition in the vault */). Therefore I think there must be a mistake somewhere.

My recommendation to your proposal is that instead of introducing new API functions that would bloat up your currently clean API, I would suggest you strictly differentiate between indexedPath and id or use an indexedPath format that can uniquely identify fields without having to generate random 6-character IDs. I would suggest you use a format that is similar to what I described above with one addition to accomodate fileClass. So you would have a dereferencing string structure (along the lines of x.y.z or x->y->z) such that the first element is always a fileClass identifier. This fileClass identifier can also be composed of 2 steps to accomodate the different ways a fileClass can be assigned to a note. For example, a global fileClass would start with global.FILECLASSNAME.... whilst a file fileClass would start with file.FILECLASSNAME.... and a preset field would start with default.preset.... and a tag fileClass with tag.TAGNAME..... This schema could allow for unique indexing of all fileClass fields in a human readable format that is easy to work with in code. However this schema could be a bit bloated in comparison to an indexedPath that just deconstructs the actual path to a given field as presented in my initial example at the beginning of this response (.status and design_phase.status) should be enough to represent the different fields named status that could exist in the same fileClass.

In addition to the above, my strongest qualm with the breaking changes is that it forces me to define fields within fileClasses. I really want to suggest that there should be a default behavior for handling fields that are not explicitly preset in some fileClass or in the settings. This would be a trivial fix if you were to modify the fileFields to use an indexedPath format instead of an id, as all you would have to do is simply expose the unset fields path and set the field to an input type. For example, say there is a dv field called "unknown" and some YAML where a root field "question" contains a subfield called "unknown". Assume that all the above fields have not been defined anywhere in the fileClass schema. I would suppose that indexedPath driven fileFields call would return an object with the keys .unknown, .question, .question.unknown which are all input type fields (or perhaps the parser can discern that .question is an object type). I think this API would be far more intuitive to a user and encourage more use of your API.

Overall, I hope the above can be considered on your end. I am also generally worried that Properties will soon start providing their own API for manipulating and managing metadata (time will tell). Kindly let me know your thoughts on the above. If you choose to go with your original proposal, that will be more than ok for me, but I do think for a codebase as clean as yours, it would be a shame not to build out a more optimal solution. Wishing you the best.

mdelobelle commented 9 months ago

I am assuming you are referring to the fileFields function when you say fieldsValues.

Yes indeed, I've modified my answer accordingly

I think the fundamental issue is the disconnect or lack of differentiation between id and indexedPath

You're right, I've modified the postValue signature to match the targeted attribute : idindexedPath

kindly note that the documentation does not document what an indexedPath

I'll add it in the next release

A dotted structure has flaws since design_phase.status can also be a valid YAML key, that's the reason why I'd prefer give an that can't conflict with names of other fields (I agree that the odds are pretty small, but I've already found myself spending countless hours in debugging such conflicts).

I'll introduce a postNamedFieldValues in the next build. let me know if it improves your workflow

You can test it in version 0.7.0-beta-1

RicSanOP commented 9 months ago

Understood. I look forward to testing out the new fileNamedFields and postNamedFieldValues in the upcoming release. Thank you for your kind consideration.

mdelobelle commented 9 months ago

They are now available since version 0.7.0