mProjectsCode / obsidian-media-db-plugin

A plugin that can query multiple APIs for movies, series, anime, games, music and wiki articles, and import them into your vault.
GNU General Public License v3.0
254 stars 31 forks source link

Embed Image links #142

Open ReconVirus opened 6 months ago

ReconVirus commented 6 months ago

Changed the way the image link are outputted so that they are embed ready instead of it just being a web link to the image

image image

this allows dataview to work without any extra steps

image image image image

mProjectsCode commented 6 months ago

Please run the formatter so that the diffs become useful. The relevant command is bun run format in the root of the project, after installing the dependencies with bun i.

ltctceplrm commented 6 months ago

If I might chime in, wouldn't it be easier to just have people who want it to be an embedded image for use in dataview to use a fixed dataview query to embed it? Example:

Table ("!["+image+"|100]("+image+")") as "Poster"
From "Series"
WHERE type = "Series"
SORT file.name ASC

If all the image property becomes an embedded image then it might mess up people's workflows for instance if they use the URL to download images or something similar.

ReconVirus commented 6 months ago

If I might chime in, wouldn't it be easier to just have people who want it to be an embedded image for use in dataview to use a fixed dataview query to embed it? Example:

Table ("!["+image+"|100]("+image+")") as "Poster"
From "Series"
WHERE type = "Series"
SORT file.name ASC

If all the image property becomes an embedded image then it might mess up people's workflows for instance if they use the URL to download images or something similar.

the dataview in the screenshot would be simpler and wouldnt just be limited to dataview, can you would be able to use it in something like =this.image which would pull the information from the image property and already embed the image without futher adding needed work to do so

image

would just be render as

image

and the other set of pictures for example

The image url is still there, it just makes it more useful within obsidian. For workflows that rely on the URL for downloading images, the URL can still be easily accessed and used as before. The change primarily aims to streamline the process of embedding images, making it more efficient in what the plugin does inside of obsidian.

all this does in the end is have the plugin already do the work that would be needed for the end user to do.

you ened up having to do

Table ("!["+image+"|100]("+image+")") as "Poster"
From "Series"
WHERE type = "Series"
SORT file.name ASC

when it could have be as simple as doing

Table image as "Poster"
From "Series"
WHERE type = "Series"
SORT file.name ASC
ReconVirus commented 6 months ago

this would just be a step in the right direction for something like #137 and #134 where something like this would have been useful on properties thought would have benefited from using embeds

ltctceplrm commented 6 months ago

the dataview in the screenshot would be simpler and wouldnt just be limited to dataview, can you would be able to use it in something like =this.image which would pull the information from the image property and already embed the image without futher adding needed work to do so

I believe that =this.image is actually an inline dataview query and would not work without the dataview plugin

The image url is still there, it just makes it more useful within obsidian. For workflows that rely on the URL for downloading images, the URL can still be easily accessed and used as before. The change primarily aims to streamline the process of embedding images, making it more efficient in what the plugin does inside of obsidian.

I'm not convinced that the url is as easy to access as before, you'd have to strip the embed portion of the url each time

Additionally you'd completely lose the ability to choose a specific size for the images, with the current URL I can just do this to embed an image in a note and have it be a specific size: ![poster_movie_The Abyss|350](https://m.media-amazon.com/images/M/MV5BYmU4NmUxZjEtYjY0OC00ZDAwLTg0MGQtMDRkNDk5NWQ0OTQ5XkEyXkFqcGdeQXVyMjUzOTY1NTc@._V1_SX600.jpg)

It's all done automatically with a simple template and I don't see how it would be possible to choose specific sizes for images with this change.

I would like for #137 and #134 to be added as well but those would be a lot less disruptive than changing the image url to an embed.

Maybe if there was a toggle for it in the settings then people can choose?

Edit: Additionally wouldn't it be best to change the other APIs to embed the image as well so the plugin remains consistent?

ReconVirus commented 6 months ago

I believe that =this.image is actually an inline dataview query and would not work without the dataview plugin

yes you are right, this would allow inline dataview query to work as well with embeds from the yaml

Additionally you'd completely lose the ability to choose a specific size for the images, with the current URL I can just do this to embed an image in a note and have it be a specific size: ![poster_movie_The Abyss|350](https://m.media-amazon.com/images/M/MV5BYmU4NmUxZjEtYjY0OC00ZDAwLTg0MGQtMDRkNDk5NWQ0OTQ5XkEyXkFqcGdeQXVyMjUzOTY1NTc@._V1_SX600.jpg)

It's all done automatically with a simple template and I don't see how it would be possible to choose specific sizes for images with this change.

the change is setup in away similar to a template as

`![](${result.images?.jpg?.image_url})`

which would allow the end user to give a title and allow whatever size they perfer to be input after importing the data with a blank [] so this wouldnt stop that from happening

Edit: Additionally wouldn't it be best to change the other APIs to embed the image as well so the plugin remains consistent?

right, i didnt do the other Apis as i thought the embeds would be more useful in something that would commonly have the most use for it. i fear the plugin in of it self isnt the most consistent after looking through the code.

ltctceplrm commented 6 months ago

But again this would force people who use this plugin to also use dataview if they want to be able to do any of that, it's an added dependency and then you notes are less future proof.

Would it be alright if I just add a toggle in the settings and default it to off, so current users aren't impacted but they can change to it if they choose to? I'd also add the embedding to the other apis that have the image property.

ReconVirus commented 6 months ago

Absolutely, adding a toggle in the settings seems like a balanced solution that respects the needs/wants of all users. This way, those who prefer the current functionality can maintain their workflow, while others can opt-in to the new feature if they find it beneficial.

extending the embedding functionality to other APIs that have the image property would enhance the overall utility of the plugin and something that i did overlooked.

So, yes, please proceed with adding the toggle and extending the embedding functionality. I believe these changes will be welcomed by the community.

ltctceplrm commented 6 months ago

I sent you a pull request on your fork so you can merge it if you're okay with the code and then mProjectsCode can check it and see if he's okay with it too.

The text accompanying the toggle in the settings is just "Embed the poster urls as images in the yaml to integrate with dataview" but it can definitely be improved. image