tomer8007 / whatsapp-web-incognito

A Chrome extension that disables read receipts and presence updates on WhatsApp Web
MIT License
324 stars 75 forks source link

Add status downloading #151

Closed IsaacHatton closed 11 months ago

IsaacHatton commented 11 months ago

In reference to https://github.com/tomer8007/whatsapp-web-incognito/issues/68

I'm new to this codebase, apologies if I have done anything in non-standard ways (e.g. hardcoding the svg in code or variable names).

Also, when a new status first loads, the src value is something like this, which this code will fail to download, and the icon won't appear: image However, after they have been loaded once they use a string starting with "blob:web.whatsapp.com" as src which can be downloaded which will be detected by this code.

Despite these flaws, it is still useful as you just have to reopen the status (I can add a warning for this if you would like this) and it can be saved.

tomer8007 commented 11 months ago

Hello, thanks for the pull request. I would suggest:

IsaacHatton commented 11 months ago

I have made those changes, is that MutationObserver ok or do I need to find a way to make it more finely scoped?

tomer8007 commented 11 months ago

Please try to make the MutationObserver use its parameters in a more dedicated way, Also can you attach a screenshot of the download button in a status?

IsaacHatton commented 11 months ago

image

I have changed the mutation observer, so that the main one that will be running the most only checks for changes involving a status.

tomer8007 commented 11 months ago

But you are now creating a new MutationObserver inside the previous MutationObserver callback. You should filter by the elements you want to observe like I did. Look at the docs.

IsaacHatton commented 11 months ago

Apologies, I am new to MutationObservers, is this the way that it should be structured?

I tried to copy and modify the code in the section you mentioned.

tomer8007 commented 11 months ago

Yes, this is better. Does the button addition when viewing a status work correctly? Can you download both photos and videos correctly?

tomer8007 commented 11 months ago

Maybe you can create a "re-open this status to download" text when you see a weird src value (not a big modal warning, just instead of the button)

tomer8007 commented 11 months ago

Do you want to add the "re-open this status to download"?

IsaacHatton commented 11 months ago

Sorry yes I think it's a good idea, just haven't had time to work out how to implement.

tomer8007 commented 11 months ago

OK

IsaacHatton commented 11 months ago

To be able to get the re-open prompt to work I had to make some changes, also the old setup was behaving wierdly, I think this should reduce wierd bugs that appear, please check if this is suitable.

tomer8007 commented 11 months ago

P.S Sorry for the delay, I missed your comment

tomer8007 commented 11 months ago

Also, how stable is your solution right now? Are there any bugs? Can you post a photo of how the re-open prompt looks like?

IsaacHatton commented 11 months ago

This is the re-open prompt in the current state: image

I haven't noticed any bugs so far, I think it's quite stable.

tomer8007 commented 11 months ago

If you know you will always need to re-open the status, maybe change the text to "Please re-open the status to download"? Also can you show the prompt in the fuller context (next to the status)?

IsaacHatton commented 11 months ago

![Uploading Screenshot_20231229_154506_Chrome.jpg …]()

IsaacHatton commented 11 months ago

Screenshot_20231229_154506_Chrome.jpg

tomer8007 commented 11 months ago

OK, and how does the new text look like?

IsaacHatton commented 11 months ago

Screenshot_20231229_155638_Chrome.jpg

tomer8007 commented 11 months ago

OK, let me know when you think the PR is ready

IsaacHatton commented 11 months ago

image The lines highlighted in pink can fail if it is not a status, and the one in orange will fail if the previous pink one fails.

I can't think of a way to put this in a try catch efficiently.

IsaacHatton commented 11 months ago

I have just tested it again, and I believe it is functioning as expected.

tomer8007 commented 11 months ago

I can't think of a way to put this in a try catch efficiently.

OK, well, nevermind. At worst we'll just have a javascript error thrown. We just need to make sure the download button won't appear.

IsaacHatton commented 11 months ago

It won't appear as if there's an issue it will always return false image and the only place the function is used is here where false will make it stop: image

tomer8007 commented 11 months ago

But we don't want the error to be silently ignored. So either remove the try/catch block, or throw the original exception.

IsaacHatton commented 11 months ago

Found a solution to the error messages, I am using optional chaining now

tomer8007 commented 11 months ago

Does it still work correctly?

IsaacHatton commented 11 months ago

It is working with the normal (non re-open required) status downloads, give me a sec to test with a new one to trigger that condition.

IsaacHatton commented 11 months ago

It is working with re-open statuses as well

tomer8007 commented 11 months ago

Good. Anything else you want to change before I merge?

IsaacHatton commented 11 months ago

I was just wondering, in terms of maintainability etc what kind of indentation/comments/etc are needed, is everything fine as I can see it doesn't match some other parts of the codebase?

tomer8007 commented 11 months ago

Yeah, I would like it if you could match the indentation to the rest of the code

IsaacHatton commented 11 months ago

Ok, will do.

IsaacHatton commented 11 months ago

I think that matches the code style of the other files

tomer8007 commented 11 months ago

Did you check that everything is still working?

IsaacHatton commented 11 months ago

Yes, it is all still fully functional, the only errors in console have been there the entire time and they are in files and functions completely unrelated to this pull request.

IsaacHatton commented 11 months ago

image

tomer8007 commented 11 months ago

Yes, these errors are really normal. If you find other issues with the extension such as crashes or messages not being sent, I would like to see issues for these. Do you think the PR is ready now?

IsaacHatton commented 11 months ago

Yes I think it is ready, thank you for all of your help.