Embed Image links #142

ReconVirus · 2024-05-02T22:43:33Z

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

this allows dataview to work without any extra steps

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

ReconVirus · 2024-05-02T22:48:32Z

src/api/apis/MALAPI.ts

Just reupload the files but the actual changes are on lines

MALAPI: 119, 153, 184

ReconVirus · 2024-05-02T22:49:12Z

src/api/apis/OMDbAPI.ts

Just reupload the files but the actual changes are on lines

OMDbAPI: 145, 177, 207

mProjectsCode · 2024-05-02T22:57:33Z

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 · 2024-05-09T14:43:17Z

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 · 2024-05-11T10:26:44Z

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

would just be render as

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 · 2024-05-11T10:31:03Z

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 · 2024-05-11T14:22:14Z

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 · 2024-05-12T08:53:49Z

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 · 2024-05-12T15:15:31Z

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 · 2024-05-12T17:33:14Z

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.

…her apis

ltctceplrm · 2024-05-13T11:39:11Z

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.

Added an option for the poster embeds and also added embeds to the other apis

ReconVirus · 2024-05-14T10:28:05Z

src/api/apis/MALAPI.ts

@@ -108,7 +108,7 @@ export class MALAPI extends APIModel {
 				url: result.url,
 				id: result.mal_id,

-				plot: result.synopsis,
+				plot: result.synopsis ? result.synopsis.replace('[Written by MAL Rewrite]', '').trim() : '',


This and line 177 isnt need on the MALAPI.ts, this is suppose to remove the end comment return from pulling plot synopsis. No all of them had it and this was something i personally add on my end, that ened up getting pushed to PR, this doesnt need to be added.

this seems useful to keep

ReconVirus · 2024-05-14T10:47:40Z

src/settings/Settings.ts

@@ -279,6 +281,15 @@ export class MediaDbSettingTab extends PluginSettingTab {
 					void this.plugin.saveSettings();
 				});
 			});
+		new Setting(containerEl)
+			.setName('Embed posters in YAML')
+			.setDesc('Embed the poster urls as images in the yaml to integrate with dataview.')


This pretty much sums it up nicely and no other way to put it, just adding a alternative

"Embed the URLs of the poster images within the image property using the image embedding syntax ![](URL). This will enable the images to directly integrate with Dataview, as opposed to simply being a hyperlink."

Save image links as markdown embeds in the frontmatter, for easier handling with e.g. Dataview.

mProjectsCode · 2024-05-27T20:25:23Z

src/api/apis/BoardGameGeekAPI.ts

@@ -75,7 +75,9 @@ export class BoardGameGeekAPI extends APIModel {
 		const boardgame = response.querySelector('boardgame')!;
 		const title = boardgame.querySelector('name[primary=true]')!.textContent!;
 		const year = boardgame.querySelector('yearpublished')?.textContent ?? '';
-		const image = boardgame.querySelector('image')?.textContent ?? undefined;
+		const image = this.plugin.settings.embedPosters


Doing the ternary everywhere seems bad, as it's a lot of code duplication. This would be better as a general post-process step applied to all data models after the individual APIs created them.

mProjectsCode · 2024-05-27T20:26:03Z

src/api/apis/MALAPI.ts

@@ -108,7 +108,7 @@ export class MALAPI extends APIModel {
 				url: result.url,
 				id: result.mal_id,

-				plot: result.synopsis,
+				plot: result.synopsis ? result.synopsis.replace('[Written by MAL Rewrite]', '').trim() : '',


this seems useful to keep

src/settings/Settings.ts

mProjectsCode · 2024-05-27T20:30:23Z

src/settings/Settings.ts

@@ -279,6 +281,15 @@ export class MediaDbSettingTab extends PluginSettingTab {
 					void this.plugin.saveSettings();
 				});
 			});
+		new Setting(containerEl)
+			.setName('Embed posters in YAML')
+			.setDesc('Embed the poster urls as images in the yaml to integrate with dataview.')


Save image links as markdown embeds in the frontmatter, for easier handling with e.g. Dataview.

…-db-plugin

mProjectsCode

With my previous review I wasn't refering to just the one terneray that I marked, but all the ternerys for images. They all do more or less the same, this surrounding the link with an embed can be done once for all media type models in a post process step.

mProjectsCode · 2024-05-31T11:30:03Z

src/api/apis/BoardGameGeekAPI.ts

@@ -18,6 +18,11 @@ export class BoardGameGeekAPI extends APIModel {
 		this.types = [MediaType.BoardGame];
 	}

+	processImageLink(boardgameElement: { querySelector: (arg0: string) => { (): any; new (): any; textContent: any } }) {


There should be a better type for this, no? I am guessing boardgame has a type that can be used here.

Add files via upload

4ec25de

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

ReconVirus commented May 2, 2024

View reviewed changes

src/api/apis/MALAPI.ts

Copy link

Author

ReconVirus May 2, 2024

Choose a reason for hiding this comment

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

Just reupload the files but the actual changes are on lines

MALAPI: 119, 153, 184

ReconVirus commented May 2, 2024

View reviewed changes

src/api/apis/OMDbAPI.ts Outdated

Copy link

Author

ReconVirus May 2, 2024

Choose a reason for hiding this comment

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

Just reupload the files but the actual changes are on lines

OMDbAPI: 145, 177, 207

ran bun run format

ae5e0e4

Added an option for the poster embeds and also added embeds to the ot…

8335112

…her apis

ReconVirus and others added 2 commits May 14, 2024 02:04

Merge pull request #1 from ltctceplrm/tmp-reconvirus

85719d6

Added an option for the poster embeds and also added embeds to the other apis

adding a replace('[Written by MAL Rewrite]'

1254671

ReconVirus commented May 14, 2024

View reviewed changes

adding a replace('[Written by MAL Rewrite]'

0e56ab2

ReconVirus commented May 14, 2024

View reviewed changes

mProjectsCode requested changes May 27, 2024

View reviewed changes

ReconVirus added 2 commits May 30, 2024 06:07

Merge branch 'master' of https://github.com/ReconVirus/obsidian-media…

45eb17b

…-db-plugin

added a processImageLink helper function to handle ternary operator

3d865db

ReconVirus requested a review from mProjectsCode May 30, 2024 13:12

mProjectsCode reviewed May 31, 2024

View reviewed changes

PeterBeckley mentioned this pull request Jul 24, 2024

Save images locally #150

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Embed Image links #142

Embed Image links #142

ReconVirus commented May 2, 2024

ReconVirus May 2, 2024

ReconVirus May 2, 2024

mProjectsCode commented May 2, 2024 •

edited

Loading

ltctceplrm commented May 9, 2024

ReconVirus commented May 11, 2024

ReconVirus commented May 11, 2024

ltctceplrm commented May 11, 2024 •

edited

Loading

ReconVirus commented May 12, 2024

ltctceplrm commented May 12, 2024

ReconVirus commented May 12, 2024

ltctceplrm commented May 13, 2024

ReconVirus May 14, 2024

mProjectsCode May 27, 2024

ReconVirus May 14, 2024

mProjectsCode May 27, 2024

mProjectsCode May 27, 2024

mProjectsCode May 27, 2024

mProjectsCode May 27, 2024

mProjectsCode left a comment

mProjectsCode May 31, 2024

Embed Image links #142

Are you sure you want to change the base?

Embed Image links #142

Conversation

ReconVirus commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mProjectsCode commented May 2, 2024 • edited Loading

ltctceplrm commented May 9, 2024

ReconVirus commented May 11, 2024

ReconVirus commented May 11, 2024

ltctceplrm commented May 11, 2024 • edited Loading

ReconVirus commented May 12, 2024

ltctceplrm commented May 12, 2024

ReconVirus commented May 12, 2024

ltctceplrm commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mProjectsCode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mProjectsCode commented May 2, 2024 •

edited

Loading

ltctceplrm commented May 11, 2024 •

edited

Loading