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

Now edge property field types aren't overwritten on plugin load #581

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Signynt
Copy link

@Signynt Signynt commented Nov 11, 2024

Moved the code setting the right property types to onLayoutReady, since it's previous position in onload caused this.app.metadataTypeManager.getAllProperties(); to run before it was initialized.

Also made a minimal change to prevent property types to be overwritten if they are already declared.

Moved the code block setting the right property types to `onLayoutReady`, since it's previous position in `onload` caused `this.app.metadataTypeManager.getAllProperties();` to run before it was initialized.
Also made a minimal change to prevent property types to be overwritten if they are already declared.
Copy link
Owner

@SkepticMystic SkepticMystic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there, thank you for your PR! It's always appreciated. Check out the comment I left and let me know what you think.
I'd like to zoom out though. Why do you want the field type not to be "multitext"? What type do you use?

this.app.metadataTypeManager.getAllProperties();

for (const field of this.settings.edge_fields) {
if (all_properties[field.label]?.type) continue;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type will always be there, so this if branch will always continue. I understand what you intend to do, but don't think this achieves it.
Maybe try something like if (!["text", "multitext"].includes(all_properties[field.label]?.type)) { continue }. In other words, if the field type has already been set to something other than "text" (the default), or "multitext" (the BC target), then skip it. The assumption being that the user overrode that field

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't type only be there if the property has already been set? Meaning that if the properties.yaml doesn't contain the property field yet, a type won't be set, and it will then run this.app.metadataTypeManager.setType(field.label, "multitext");.

However the only two types that make sense here are text or multitext, so I'd be fine with changing this to your suggestion, I can modify it.

@Signynt
Copy link
Author

Signynt commented Nov 12, 2024

Hey, I addressed why I want this change in issue #580 (I use the type "text", since I want to avoid a single file having more than one "previous" note, even by accident).

@SkepticMystic
Copy link
Owner

Ok, I understand your usecase better. I think this would be better as a setting then. Because in either of our implementations, it can't know the user's intention. Let's rather let them set that explicitly, and only override the field types if that setting is enabled. I'd argue the default should be enabled, as that's the current behaviour, and desirable to most users.
What do you think?

@Signynt
Copy link
Author

Signynt commented Nov 12, 2024

Having more options is always a good thing.

I'll let you decide what the default should be, but here's my argument for having it not overwrite the property type by default:

  1. It's an unintuitive user experience for property types to be overwritten after a user has changed them, especially since Obsidian never does this natively, and there is no documentation on Breadcrumbs doing this.
  2. Setting a property type on every load doesn't seem to be necessary, as once a property type is set, it usually doesn't change.

To share my personal experience with 1, I was really confused as to why my property type kept changing, since I hadn't experienced any plugins doing this previously, and at first thought it was some Obsidian Sync issue, and started looking for potential bugs there, before I had the suspicion that it might be due to the Breadcrumbs plugin, and checked the plugin code. I was using the "previous" property before I started using Breadcrumbs, and I found it strange that the plugin would modify it's type without letting me know it was doing this.
Regarding 2, I don't exactly see what advantage setting the property type each load has, since the plugin works regardless of if the property type is "text" or "multitext". Am I missing something?

I thus find it a reasonable default to have a plugin not perform any modifications to the vault, unless the user explicitly asks it to.

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

Successfully merging this pull request may close these issues.

2 participants