Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I discarded the renamed fields in different locales as I felt this change was unwarranted |
|
The upload is now more configurable :) |
|
Unfortunately even with the three configuration options it's still too rigid. I did a quick analysis (using AI) to determine what formats other image hosts expect for the uploader. I think the proper approach here would be to do what I mentioned in my previous comment:
You would create an interface which accepts an uploader callback as well as a group of custom settings which are depicted as an object but parsed to the interface. Works (or should) with current config
Does not work without extending the client
|
|
So you'd like me to provide some pre-defined working settings for different image hosts. That's okay, but which image hosts would you like me to implement then? Your first table depicts the hosts that already work with the current implementation. I could then show different config settings per selected image host, depending on what they need. For example, for |
|
Just implement your preferred host to start (uguu.se).
Exactly. |
|
I won't have a lot of time the coming days so it'll take a while for me to implement, but when I do I'll also include Thank you for the quick review :) |
|
I implemented interfaces for Adding another image proxy should be as easy as exposing the user-configurable config items in Feishin UI, defining its interface, passing the user config items through IPC and implementing its upload function where it uses the config items. Could you review the code once more? If you have any questions, feel free to ask me through my Discord at the same username. |
| "discordServerType_uguu": "uguu.se", | ||
| "discordServerType_litterbox": "litterbox.catbox.moe", |
There was a problem hiding this comment.
URLs do not need a translation string.
| { | ||
| control: ( | ||
| <TextInput | ||
| onChange={(e) => { | ||
| setSettings({ | ||
| discord: { | ||
| litterboxTime: e.currentTarget.value, | ||
| }, | ||
| }); | ||
| }} | ||
| value={settings.litterboxTime} | ||
| /> | ||
| ), | ||
| description: t('setting.discordLitterboxTimeField', { | ||
| context: 'description', | ||
| }), | ||
| isHidden: !isElectron() || settings.serverType !== DiscordServerType.LITTERBOX, | ||
| title: t('setting.discordLitterboxTimeField'), | ||
| }, |
There was a problem hiding this comment.
I'd rather not allow specific settings like this as it adds additional overhead especially if additional uploaders are added in the future each with their own specific settings.
I would prefer you refactor this into a JSON setting and input which allows the user to free-form a settings config (JSON) with a link to external documentation added to the repo docs as markdown for each uploader.
Formatted as <uploader>_<setting>, you could then add an infinite number of settings that could then be passed to the uploader to parse and perform actions on. Now we can add specific settings per uploader without having to add settings UI and translation strings to each one.
{
"litterbox_time": "72h"
}
| clientId: '1165957668758900787', | ||
| displayType: DiscordDisplayType.FEISHIN, | ||
| enabled: false, | ||
| imageProxyServerLink: 'https://uguu.se/upload', |
There was a problem hiding this comment.
They each specify their own defaults. I kept the URL as a user config item because I felt that users should have the option to point it somewhere else, for example at their own APIs that conform to the same spec. The URLs for each image proxy have their defaults point to the URL you'd expect, so users can simply choose an image proxy while not configuring anything, and it should work for them.
From what you said, this is treated as an override? In this case shouldn't the default setting be empty?
| case DiscordServerType.LITTERBOX: | ||
| if ( | ||
| !['1h', '12h', '24h', '72h'].includes( | ||
| discordSettings.litterboxTime, | ||
| ) | ||
| ) { | ||
| config = {}; | ||
| break; | ||
| } | ||
| config = { | ||
| time: discordSettings.litterboxTime as LitterboxImageProxyConfig['time'], | ||
| }; | ||
| break; | ||
| case DiscordServerType.UGUU: { | ||
| config = {}; | ||
| break; | ||
| } |
There was a problem hiding this comment.
In addition to the JSON settings change, the raw config should be passed to the uploader which handles parsing and consuming the config rather than at the discord rpc hook level.
2 participants
What does this fix?
I personally use Navidrome, and embed my cover arts directly into the files. I then instruct Navidrome to use these embeds as the first priority for viewing the cover art, rather than the associated album image. This does not interface well with Feishin Discord RPC, as Feishin asks for the album image directly on the album id. Rather than doing this, we can do the following:
This feature works out of the box if
Image proxyis selected for the server type, and the linkhttps://uguu.se/uploadis given.Verification steps
uguu.se.Things I'm not too sure about
Serve discord images from serverand its text.Additional notes
I would like to say that I have been enjoying Feishin a lot and do not plan to return to any other platform after experiencing this one. Thank you all for making such a great app!
Second, this is my first open-source contribution. I do not have much experience working in big codebases like these. Feel free to let me know what I could've done better :)