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

Plugin: Media downloader #2349

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

MachineMuse
Copy link
Contributor

@MachineMuse MachineMuse commented Apr 11, 2024

Depends on #2346

Addresses plugin request Vencord/plugin-requests#188

Many TODOs still but it's working pretty well for what it is so I figured I could put it in the review queue and if I get more time to work on it I'll just keep adding on.

Features (each can be enabled/disabled individually):

  • (background) A 'safe' native interface that allows the user to select their media download folder without exposing direct file writing capabilities to the renderer
  • A button in the message hover toolbar ('accessories') which downloads all embedded and attached media to the configured folder in a single click
  • A menu item in the context menu when clicking on an image (in a message or when enlarged) which downloads the image to the preselected folder in one click
    • This currently can only download proxied media and still previews of videos, just like the 'save as' menu option. It might be possible to improve this behaviour but it'll be more invasive.
  • A download hover button over image attachments, in the top-right, like the one over video attachments
    • Only attached images, not embedded ones. Embedded ones will need a separate, more invasive patch (probably my next priority).
  • An option to hijack aforementioned hover button to one-click download to the configured media folder (for videos, images, and audio) instead of opening it in the browser

Copy link
Owner

@Vendicated Vendicated left a comment

Choose a reason for hiding this comment

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

Wow, this looks great so far, especially for a first time contributor!

This is only a partial review for now, mainly cause i feel bad for taking so long to get to it

I'll have to do a more thorough review soon, hopefully tomorrow, due to the size & potential security issues there could be

I still see a bunch of TODO comments... Do you need help with those or are you still working on them?


function DirectoryPickerComponent(props: { setValue(v: any): void; }) {
const [value, setValue] = useState("");
useEffect(() => { Native.getMediaFolder().then(setValue).catch(console.log); });
Copy link
Owner

Choose a reason for hiding this comment

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

this is a bit strange, it'd be better to do something like:

const [signal, forceUpdate] = useForceUpdater(true);
const [value, error, isLoading] = useAwaiter(Native.getMediaFolder, {
	deps: [signal],
	defaultValue: null
})

this will properly handle the promise & error handling for you and calling forceUpdate will re-run it

}

function getNativeSettings() {
if (!NativeSettings.store.plugins) NativeSettings.store.plugins = {};
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if (!NativeSettings.store.plugins) NativeSettings.store.plugins = {};
NativeSettings.store.plugins ??= = {};

return Native
.downloadFile(uri, uri)
.then(success => console.log(success))
.catch(error => console.log(error));
Copy link
Owner

Choose a reason for hiding this comment

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

these should definitely notify the user in some other way, e.g. a toast

{
find: "AnalyticEvents.MEDIA_DOWNLOAD_BUTTON_TAPPED",
replacement: {
match: /href:(\w{1,3}),onClick:(\w{1,3}),target:(\w{1,3}),rel:(\w{1,3}),className:(\w{1,3}),"aria-label":(\w{1,3}).default.Messages.DOWNLOAD,/g,
Copy link
Owner

Choose a reason for hiding this comment

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

Vencord has a custom \i escape sequence that matches identifiers!

Suggested change
match: /href:(\w{1,3}),onClick:(\w{1,3}),target:(\w{1,3}),rel:(\w{1,3}),className:(\w{1,3}),"aria-label":(\w{1,3}).default.Messages.DOWNLOAD,/g,
match: /href:(\i),onClick:(\w{1,3}),target:(\w{1,3}),rel:(\w{1,3}),className:(\w{1,3}),"aria-label":(\w{1,3}).default.Messages.DOWNLOAD,/g,

message,
channel: ChannelStore.getChannel(message.channel_id),
onClick: async () => {
// console.log("Attachments:");
Copy link
Owner

Choose a reason for hiding this comment

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

please remove all these debug lines once you're done

also might i suggest using the debugger instead? it's underrated! you can just search some code of your plugin in devtools (ctrl shift f), jump to the source code (make sure you have js source maps enabled) and put breakpoints

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

Successfully merging this pull request may close these issues.

None yet

2 participants