search

nikobojs / manuscrape_electron

ManuScrape desktop app for Windows 11
5 stars 2 forks source link

#78 fix: update statusText when processing scrollshot #80

Closed samuelhimmelstrup closed 6 months ago

samuelhimmelstrup commented 6 months ago

the general purpose useOnMarkedAreaCallback was only used in two places (single screenshot and scrollshot), but it didn't allow for the status text to be updated during the callback. I have defined the callback function directly in the createScrollScreenshot, which now consists of two parts (captureScrollshit + processScrollshot) with a change of statusText in between.

To DRY it up a bit, I have extracted the logic for error handling and the "clean up" logic in the finally block.

I might dive back into this part of the code, and rethink the structure. My gut feeling is that better separation of concerns, perhaps with more single responsibility classes, could lead to better readability.

nikobojs commented 6 months ago

Totally agree that that part of the codebase is messy... I remember myself hacking it all together and it was a soo difficult to get to work correctly with the canceling of scrollshot while keeping the overlay stuff generic and so on.

Do you want to continue committing on this PR or should i merge it? It looks great btw <3

samuelhimmelstrup commented 6 months ago

It is a bit messy but it works great! I have ideas for refactors to improve readability. But I would like to do that on a separate branch, so go ahead and merge this one. I have just tried compiling it and running the new version on my mac, and it seems to have solved the issue of the first scrollshot not working as well (will need further testing to determine with certainty)

1, 2, 3 -> Merge ;)

nikobojs commented 6 months ago

I will do thorough testing on windows before merging unstable into stable :+1: