Skip to content

Align the look of Startup-Scripts settings with core CSS snippets #34

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Cube707
Copy link

@Cube707 Cube707 commented Feb 21, 2025

Hello again, first of all many thanks for implementing the startup-scripts feature so incredibly quickly.

When I first started on this last year I imagined it beeing much more inline with the "look and feel" of the CSS snippets in obsidian-core. Which is why I took the time to create this proof-of-concept. It currently looks like this:

image

what I believe are pros of this approach:

  1. consistency with the core behavior makes it easier for users to find and use stuff. also feels more "in place"
  2. using .obsidian/snippets as the default location is much more intuitive IMHO
  3. much flatter GUI, everything is directly viewable and not hidden behind popups
  4. toggling the scripts can be usefull when debugging
  5. reducing the snippets to one folder makes it easier to keep startup and libary code seperate

potental cons:

  1. only on folder
  2. usage of an undocumented function openWithDefaultApp()
  3. adds another setting

work in proges stuff

I am unsure about some things and would like your opinion on it:

when to reload?

Should hitting the "refresh" button reload the scripts?

Should the scripts be executed when they are toggled on?

or should they only load on startup?

how to reload?

currently the display() function just gets called again, removing all elements an rebuilding all settings. Should any effort go into making this more smart, recreating only the script-elements?

opening the settings folder, without the folder existing

The implementation for CSS-snippets handles this case by creating the folder first if its missing. But this would required pulling in another undocumented function and creating the folder should be the useres responsibility.

On my (Windows) system the call results in an OS-error message that the path doesn't exists, which is fine I think?

how to handle the default folder

currently the settings field for snippets-folder will be auto-populated with .obsidian/snippets for every falsey value.

This lead to some wired edgecases in that leaving it empty will re-populate it with the default value upon next load.

The default value of .obsidian/snippets will also be written to the settings file after any settings was changed, which is not completely inline with how defaults should work (I belive?), but could also be a feature as it makes the setting stable agains future changes in the default. But it will lead to unexpeted breakage when the user changes his config-folder to somethings else than .obsidian.

Maybe you know a better what to implement a dynamicly constructed default value? I experimented with having the default of startupScriptsDirectory be undefined, but removed that as I had to ad a bounch of ?? in places to gobble the never-happening undefine case.

my shitty TS skills

TypeScript is not my stronges language, so feel free to point out anything that could be improved.


Once we agree on how to proceed I will also get onto documenting this feature

Copy link
Author

@Cube707 Cube707 left a comment

Choose a reason for hiding this comment

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

some more discussion points

@Cube707 Cube707 force-pushed the feat/align-js-and-css-snippet-settings branch from 0c9a40e to 6a92e1c Compare February 22, 2025 15:22
Copy link
Owner

@mProjectsCode mProjectsCode left a comment

Choose a reason for hiding this comment

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

On the one hand this approach is closer to how base Obsidian does things. On the other hand it's a less flexible way of doing things. I am torn.

One other point is the migration of existing settings. I don't really want to break existing user startup scripts.

@mProjectsCode
Copy link
Owner

You might want to consider using Svelte for the UI, as Svelte makes complicated UIs like this easier.

@Cube707
Copy link
Author

Cube707 commented Feb 23, 2025

rebasing onto 0.3.1

@Cube707
Copy link
Author

Cube707 commented Feb 23, 2025

One other point is the migration of existing settings. I don't really want to break existing user startup scripts.

The feature is quite new and not explicitly mentioned anywhere. Mostlike we are the only two knowing about it at the moment.

And if anybody actually dug deep enough into the example-vault or the commit history to find this, they would also probably be able to find this update.

But I could of course implement a migration function that tries to split exiting startupScripts into a startupScriptsDirectory and enabledStartupScripts and throws an error it it failes. But that sounds like a lot of work for it to probably never be used.

Or we deviate from the way obsidian-core handles it and store the full paths and a boolean in the settings instead of only the filename

@Cube707 Cube707 force-pushed the feat/align-js-and-css-snippet-settings branch 2 times, most recently from 0a4632b to 6808563 Compare February 23, 2025 17:57
@Cube707
Copy link
Author

Cube707 commented Feb 23, 2025

This current version keeps full backwards compatibility in that it will still run all startupscripts that have previously been added

BUT it doesn't show them, the user needs to change startupSciptsDirectory to the folder containing the files to remove them. Or edit the JSON directly.

This may be a decent compromise. Or I could add an additional function which will join the existing files with the detected ones.

downside is that the settings now deviate from how obsidian core handles the CSS stuff

Copy link
Owner

@mProjectsCode mProjectsCode left a comment

Choose a reason for hiding this comment

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

This looks better. But I think there is a bug where changing the startup scripts directory, while having some scripts enabled, makes it impossible to disable those scripts. Same goes for migrated scripts.

@mProjectsCode
Copy link
Owner

The feature is quite new and not explicitly mentioned anywhere. Mostlike we are the only two knowing about it at the moment.

And if anybody actually dug deep enough into the example-vault or the commit history to find this, they would also probably be able to find this update.

A user only has to look at the plugin's settings page to find this feature. With due respect, this mentality does not result in user-friendly plugins.

@mProjectsCode
Copy link
Owner

I just had an idea on how to handle enabled scripts that are not in the scripts folder. They should be shown below in their own list with small warning that they should be moved to the scripts folder. The enable/disable toggle should be a remove and a move button, the later of which tries to move the file to the scripts folder while keeping the same name. I think that should be not too difficult to program and offer a nice transition for current users of startup scripts.

@Cube707
Copy link
Author

Cube707 commented Feb 23, 2025

But I think there is a bug where changing the startup scripts directory, while having some scripts enabled, makes it impossible to disable those scripts. Same goes for migrated scripts.

Yes and no. With the retention of old scripts I didn't find a good logic of when to throw out scripts without having wird edgcases where old scripts work for a while and then suddenly stop working after the user changes something seemingly unrelated.

But yes keeping the old scripts around is "invisible" and not really intuitive

@Cube707 Cube707 force-pushed the feat/align-js-and-css-snippet-settings branch from 6808563 to e365545 Compare February 23, 2025 20:23
@Cube707
Copy link
Author

Cube707 commented Feb 23, 2025

Found another problem, the vault api seems to not be reliable with files and folder inside the .obsidian folder. I also found some post suggesting to directly use adapter there. I also remember now that this was a reason the first version used the adapter.list() method.

I will continue to investigate this tomorrow.

@Cube707
Copy link
Author

Cube707 commented Feb 25, 2025

So I did a lot more testing and the whole vault API doesn't support anything inside the .obsidian folder. It internal uses a fileMap of all known file and it ignores everything inside the folder.

I tried adding the .obsidian/scripts folder to the filemap to get it indexed, but that not straight forward. So I would like your oppinion:

Should I continue to try to find a way to get the startupscripts indext on the vault API?
Or would it be acceptable to rewrite executeFile() to read the script directly from adapter if getFileByPath() returnes null (and manually construct the TFile)
Or are both ideas a dealbreaker and we should scrap startupscripts inside .obsidian (even if I would really like having that option)

@mProjectsCode
Copy link
Owner

I feel like at this point there are too many tradeoffs with using a folder inside of .obsidian.

@Cube707 Cube707 force-pushed the feat/align-js-and-css-snippet-settings branch 2 times, most recently from 5517ff0 to e2f9117 Compare February 26, 2025 15:35
@Cube707 Cube707 force-pushed the feat/align-js-and-css-snippet-settings branch from e2f9117 to 654c3ed Compare February 26, 2025 15:49
@Cube707
Copy link
Author

Cube707 commented Feb 26, 2025

The new version looks like this:
image

  • defaults to the vault root for the script-directroy, so it starts out showing all JS files
  • nothing in .obsidian will show up
  • remembering and showing the preexisting scripts works

But there is still some stuff left to discuss

updating the view after changing the folder

This currently requires pressing the "refresh" button.
As the callback gets run for every keypress, running display() every time results in focus loss and is generally not the best.

I could add all "dynamic" elements to a list and write a separate function which clear every element in the list and rerenders the elments, but that crecreases teh readebility quite a bit in my opinion.

When to remove files from startupScripts

Should we just remove entries from the list if the corresponding file is no longer present?
And which parts of the code should do that? The settingstab or executeStartupScripts()

Or is it the users responsibility, which would probably require another list with a delete button for every file.

Documentation

Where should I document this (if at all?).
I feel like the fact .obsidian is not supported should be documented somewhere

@mProjectsCode
Copy link
Owner

updating the view after changing the folder

You could use the focusout event for that.

When to remove files from startupScripts

The best solution would be to show a notice on start-up and an extra section in the settings.

Simply removing them on startup would be an easy solution, but I am not sure how well that interacts with sync services if the data file gets synced before the JS files.

Documentation

There is a separate repo for the docs.

@Cube707
Copy link
Author

Cube707 commented Feb 27, 2025

You could use the focusout event for that.

That works, thanks for the tip. Just need to juggle typescript a bit more, it's currently not happy withe the usage of plain JS stuff (it doesn't know the event.targe.value property?)

Simply removing them on startup would be an easy solution, but I am not sure how well that interacts with sync services if the data file gets synced before the JS files.

Not sure what other services exist, but obsidian desktop only loads the data.json once at startup, so any syncing happing in the background after that has no effect at all (or manually modifying it for that matter)

For the case that a second, remote instance starts up after only the data.json is synced, it shouldn't be a problem. The script will simply not be executed and lie dormant until deletion is synced. The user could see (and activate it again) when opening the settings, but thats a general problem also present when manually deleting and with syncing in general.

The biggest potential problem I see is when the js file is deleted on both sides and both instances start up, try to moddif data.json (see same way). I know that for example OneDrive can decide to handle this as a collision and duplicate the file, even with identical content. But that can again also happen under normal operation.

Just my immediate thought on the matter

@Cube707
Copy link
Author

Cube707 commented Feb 27, 2025

using the focusout event now works as expected.

I also added the toggle to the "old" startupscripts so the user can just toggle them of without having to move them to the new folder first.
They will stay around until the next render, either if the settings are close, through refresh or by changing the folder again, which I find a useful feature. In case something was disabled by accident you can just reeanble it.
image

@Cube707
Copy link
Author

Cube707 commented Jun 1, 2025

Sorry for the long delay, I had a thesis to write...

I have been using the self-compiled version including these changes this howl time during my DnD sessions an have not encountered any issues with syncing (using the _ remotly save_ plugin).
I will do some active testing with syncing and deleting file tomorrow.

@Cube707 Cube707 force-pushed the feat/align-js-and-css-snippet-settings branch from f0de204 to ee870be Compare June 1, 2025 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants