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

Possible issues with marker target commands #38

Closed
Moonbase59 opened this issue May 5, 2021 · 25 comments
Closed

Possible issues with marker target commands #38

Moonbase59 opened this issue May 5, 2021 · 25 comments

Comments

@Moonbase59
Copy link

Moonbase59 commented May 5, 2021

Re #35:

WARNING: This isn’t very stable and will break

  • if the user switches Obsidian languages (which I do quite often)
    • Example (en→de): Export to PDF becomes Nach PDF exportieren
  • if you happen to have a note with the same name as a command in the base of your vault
    • Example: marker: Location,51,10,Export to PDF will start the command in English setting, open the note in Deutsch setting.
  • if a plugin writer or the Obsidian translators decide to translate a command differently.

I think this simplistic approach is a little too dangerous, don’t you?

Originally posted by @Moonbase59 in #35 (comment)

@valentine195
Copy link
Member

Yeah, probably. This (honestly) was a quick end-of-day feature add to get the functionality built in for the user in issue #35.

Ultimately since Obsidian (I believe) does not expose the registered commands in their API, I'm not sure how safe I can really make this functionality. Probably putting a warning on it in the documentation will be enough, referencing your caveats.

@Moonbase59
Copy link
Author

Hmm. Not to turn someone down, but I doubt the usage metaphor. With a marker on a map, upon hovering or clicking, I’d expect "more info", "zoom to it" or "go there", much like your original version offers. But not, probably unexpectedly, or even dangerous, commands being executed, or files created.

This may be convenient for a (very) few users, but, honestly, if it were my project, I’d turn this FR down. The next user will probably come and want shell scripts executed …

Maybe when an Obsidian command can be executed via some safer mechanism, like a command ID or the like, this could be re-evaluated, or a separate "command plugin" written.

Just my 2¢.

@valentine195
Copy link
Member

I added this specific functionality because there was already a plugin in the community list that exposed it in the same way, and also the fact that a user has to explicitly enter the name of command to make it work. However, I agree that there is a potential collision issue if the user has a note by the same name as some random Obsidian command, so I think I will do two things:

  1. Make it clear in the docs that using this feature could have unintended consequences.
  2. Have a flag that must be set on a marker to tell the plugin that this marker will be executing an Obsidian command.

2 will prevent unintentional collisions with notes.

@valentine195 valentine195 changed the title WARNING: This isn’t very stable and will break re #35 Possible issues with marker target commands May 6, 2021
@valentine195 valentine195 pinned this issue May 6, 2021
@Moonbase59
Copy link
Author

Yeah, if you want to keep it, a flag seems a good idea to avoid erroneous file creations and the like.

valentine195 added a commit that referenced this issue May 6, 2021
- Added explicit `commandMarker` and Command Marker toggles
    - These are **required** if the marker is to execute a command
    - Reference issues #35 and #38
@Moonbase59
Copy link
Author

3.4.0: Best of both worlds, hee hee. I like best the "X No command found!" when working in the "wrong" language. :-)

Hopefully someday will come a better way to access commands.

@valentine195
Copy link
Member

Closing this issue as I think the toggle prevents accidental collisions

@Moonbase59
Copy link
Author

Addendum: They actually do have nice ids, see app.commands in dev console. Wonder if these could be used, since they don’t change with language (plus you get the nice translation string as a bonus!).

obsidian-command-ids

@valentine195
Copy link
Member

Yeah, I saw this when initially putting this functionality in, but these ids aren't revealed to the user at all. I guess I could store the ID when first created, though, instead of the name...

@valentine195 valentine195 reopened this May 15, 2021
@Moonbase59
Copy link
Author

Moonbase59 commented May 15, 2021

Pondering … These currently enabled commands could of course be shown using dataviewjs:

let cmds = dv.array(Object.entries(app.commands.commands))
    .sort(v => v[1].id, 'asc');
dv.paragraph(cmds.length + " commands currently enabled.<br><br>");

dv.table(["Command ID", "Name in current locale"],
  cmds.map(v => [v[1].id, v[1].name])
  );

@valentine195
Copy link
Member

I guess I could store the ID when first created, though, instead of the name...

This change will be in the next release.

@valentine195 valentine195 unpinned this issue May 18, 2021
@Moonbase59
Copy link
Author

Just for the record: I wrote a small DataviewJS script to show the current Command Palette:
https://forum.obsidian.md/t/dataviewjs-snippet-showcase/17847/13?u=moonbase59

@Moonbase59
Copy link
Author

Moonbase59 commented May 24, 2021

I guess I could store the ID when first created, though, instead of the name...

This change will be in the next release.

How then would we add a commandMarker now? Wouldn’t it get problematic when we specified it using Language A, then switch Obsidian to Language B? The map could never be re-created from the codeblock while we were staying in Language B …

Current behaviour (using 3.10.1) is:

  • I specify, say, commandMarker: Location,51,10,Open command palette in English → works
  • I switch Obsidian to German ("Deutsch")
  • I recall the map note and the map won’t render

@valentine195
Copy link
Member

A commandMarker created in the map block would not update, correct. Not sure that's going to be possible to fix cleanly, since the command IDs are not exposed to the user anywhere except in the console.

@Moonbase59
Copy link
Author

Moonbase59 commented May 24, 2021

But how else could I create a command marker? "Bulk edit" shows no option for that (if so, a list could be shown and the command id used internally).

Keeping a list in your docs and using the internal id seems unreasonable, since plugins can introduce lots of new ones. Hm.

Or maybe it could be hidden as a "Marker type" like "Default"? So we could also create command markers on the map using right-clicks. There could be an extra entry like "Command" in there, to make it handle things differently. Then again, we’d lose the marker type capability. Hm again.

Personally, I’d be quite happy using the ids in the commandMarker entry, like commandMarker: Command,51,10,command-palette:open and have it displayed in the currently set language and work in whatever language chosen. But that’s probably too nerdy … Then again, command markers might be a very special and seldomly-used feature.

@valentine195
Copy link
Member

Yeah, it's tricky.

A marker can be "converted" into a commandMarker in the marker context modal (right click on a created marker). That's the only way right now outside of the commandMarker parameter.

@Moonbase59
Copy link
Author

Ah, didn’t know that one! Needs to get into the docs … This actually seems a good way to me—and it works!

So why not tell people to set a marker, convert it, and basta? And just "secretly" have the nerdy feature of using the ids for the code block commandMarker entries?

@valentine195
Copy link
Member

https://github.com/valentine195/obsidian-leaflet-plugin/blob/master/README.md#obsidian-commands-as-links

It actually was, but accidentally under the Bulk Edit heading. I just fixed it. 😁

@valentine195
Copy link
Member

Also - I'm slowly creating an actual documentation website to host on github. Just need to find the time.

@Moonbase59
Copy link
Author

Yeah, I know how that is—we all probably like better to comment code instead of always running after keeping user docs up-to-date … sigh.

@Moonbase59
Copy link
Author

Moonbase59 commented May 24, 2021

Just found that it actually puts the command id into the "file to open" field when bulk editing, hee hee. It can’t be changed there, unfortunately. ;-)

@valentine195
Copy link
Member

Just found that it actually puts the command id into the "file to open" field when bulk editing, hee hee. It can’t be changed there, unfortunately. ;-)

Actually, now that you mention it, it should be able to be changed there (just no command marker <-> note marker toggle). I'm assuming it opens a note list suggestion box on click? I can detect that it's a command marker and open the command suggestion box instead.

@Moonbase59
Copy link
Author

Yep, it tries to open a file then. Interestingly, when setting "command" to "on", it initially even shows the new command id I entered, but when saving, it vanishes. I have to go back in, enable command, and select one.

@Moonbase59
Copy link
Author

I will probably never use a command marker, but this "make a marker and then convert it to a command" might just be the thing for the command marker guys, maybe use a custom map image that shows "functions" and then be able to click around on many commands. :-)

@valentine195
Copy link
Member

Yep! Honestly, probably won't ever use it either, but haven't gotten any complaints yet!

@Moonbase59
Copy link
Author

Moonbase59 commented May 24, 2021

I think I should update my Dataview command list with an id column, just to be sure …
Darn! It has it already … I must be getting old …

valentine195 added a commit that referenced this issue May 24, 2021
## 3.11.1
- Fixed issue where opening a map with a marker type that no longer exists would fail to render the map
- #38 - Fixed issue where bulk editing markers on maps with command markers was not respecting the Command Marker toggle
- #25 - Removed maxZoom constraint from Show All Markers control button
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

No branches or pull requests

2 participants