-
Notifications
You must be signed in to change notification settings - Fork 40
Adding options to disable certain API per media type #133
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
Conversation
Currently only has the plugin settings and they don't do anything yet
I'm close but I don't now how to get the MediaType that the plugin called
|
I've added a mediaType variable that gets sent with the search query so adding media by media type is currently functional. |
src/settings/Settings.ts
Outdated
| openNoteInNewTab: boolean; | ||
| useDefaultFrontMatter: boolean; | ||
| enableTemplaterIntegration: boolean; | ||
| OMDbAPImovie: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would organize them into sub-objects, by media type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also make the code for figuring out which APIs to use simpler.
| }); | ||
| }); | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better if these settings were automatically generated. That would mean less maintenance work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree but don't know how to do so yet, especially because it's only for certain APIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if you don't know how to do that, then I will rewrite this after merging.
src/api/APIManager.ts
Outdated
| * @param apisToQuery | ||
| */ | ||
| async query(query: string, apisToQuery: string[]): Promise<MediaTypeModel[]> { | ||
| async query(query: string, apisToQuery: string[], mediaType: string): Promise<MediaTypeModel[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be better if you did the filtering of which APIs to query (based on the user's settings) before calling this method and leave the method as it was.
|
@mProjectsCode I found the mistake I mentioned and simplified the code by removing the whole "default" API setting, now Advanced Search and Media Type Search both work correctly! I'm looking through your notes now and seeing what I need to change |
For MAL, MusicBrainz and Wikipedia
|
I reverted changes to main.ts so the I've tested it out and it correctly skips the disabled APIs when selecting only 1 media type, however if you select multiple media types (movies and games) where the same API (omdb) has been turned off for one media type (movies) but not for another media type (games) then it will still return movies as well as games for omdb. I'm not sure if this is wanted/intentional or not. |
|
Seeing as the plugin is simultaneously searching for games and movies I don't see how it would be possible to exclude just the games returned by omdb without really overcomplicating the code. Unless I'm missing an easy solution I'd suggest maybe only being able to toggle one media type at a time and if you toggle a second one it un-toggles the previous one? |
|
I pushed too soon, there's still a mistake in my toggle thingy |
|
Okay it's fixed now |
|
Please let me know if I should change anything else or if it's good like this |
|
I am currently quite busy, I will review this maybe next week. |
|
That's okay, please take your time. Thank you for looking into it. |
src/api/APIModel.ts
Outdated
|
|
||
| hasType(type: MediaType): boolean { | ||
| return this.types.contains(type); | ||
| if (this.types.contains(type) && !(this.plugin.settings[[this.apiName, type].filter(s => s).join('') as keyof typeof this.plugin.settings] === false)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[this.apiName, type].filter(s => s).join('') as keyof typeof this.plugin.settings
Why not do (this.apiName + type) as keyof MediaDbPluginSettings?
However, I would still recommend organizing the settings into sub-objects for each api.
| } | ||
| apiToggleComponent.onChange(value => { | ||
| this.selectedTypes.find(x => x.name === mediaType).selected = value; | ||
| if (value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly is this code doing? Are you forcing the value of the toggle to be off in certain conditions? If so there is a better way to disable the toggle component. See the setDisabled(value) prop of the toggle component.
| }); | ||
| }); | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if you don't know how to do that, then I will rewrite this after merging.
|
Thank you for looking over the code. I'm going to address some of the problems right now and the other ones I'll probably get to tomorrow. |
|
Hello @mProjectsCode, I finally succeeded turning the API toggles into sub objects like you asked, the code additions are once again fully functional (at least according to my tests). I believe I have finished with this pull request but please let me know if I forgot something |
|
I will just merge this and then make some changes myself |
|
That's great, thank you merging and also fixing up the code. |
The code is currently not yet functional because I don't know how to get the MediaType so I can add it to the API name. To test if the rest of the code works I modified this line
And it correctly filters out the APIs that are not checked. Same works for
moviesorseries.@mProjectsCode is there a way to get the media type after it calls APIManager?