Implemented a thread for opening a file using mmap in the decrypt_fil… #82
Conversation
…e() function in /gui/app.py. It was listed as a TODO item to replace the with open() call.
Codecov Report
@@ Coverage Diff @@
## main #82 +/- ##
=======================================
Coverage 74.96% 74.96%
=======================================
Files 23 23
Lines 2169 2169
Branches 509 509
=======================================
Hits 1626 1626
Misses 422 422
Partials 121 121 Continue to review full report at Codecov.
|
| # with open(file, "rb") as f: | ||
| # data = f.read() | ||
| # MY FIX | ||
| with open(file, mode="r", encoding="utf8") as f: |
There was a problem hiding this comment.
Not sure what the original intention / objective was, but this is not a threading solution. Creating an mmap object and reading through it does does not prevent blocking the single threaded application.
Also, please do not leave old code uncommented intact.
There was a problem hiding this comment.
Apologies, I thought I had removed the commented code. I had some issues with setting up the repo. I'll make sure to remove it.
As for the question, there was a TODO item commented out prior to my fix. This was an attempt to change regular python I/O to an mmap equivalent I/O. Perhaps I misunderstood the TODO, but it wasn't meant to be a final fix. I was focusing on speeding up the performance of the file read time, which I achieved. My tests show a speed difference from the previous code.
If that was not the intention of the TODO, please disregard.
There was a problem hiding this comment.
I'm guessing the original intent was to improve responsiveness of the GUI when a large file is being decrypted. But I don't know what the original author intended. @covert-encryption Do we have a clear target? Anyway, there should be a use-case that clearly shows that the current approach produces undesired effects, after which we can test new solutions to fix the issue, if any.
There was a problem hiding this comment.
@heikkiorsila I believe he picked unfinished checkboxes from #37. Yes, the intent is responsiveness, especially when files of several gigabytes are being processed.
@technokowski The GUI freezes, "app not responding", until the SLOT function returns. Thus any long-running operations should be performed in background threads instead, and any event handlers should return (almost) immediately.
This is not an easy task to do, because it will need a ThreadPoolExecutor or plain thread handles somewhere (like in the main application object) to keep running any threaded tasks in the background without disturbing the GUI. Note that the threads cannot call any GUI code but instead the GUI also needs to run a QTimer that periodically checks for threads' results or progress made e.g. every 100 ms.
If you are not already familiar with threads, I would suggest starting with some easier things instead. Contributions on the GUI would certainly be most welcome, although you may find tasks within the CLI/core a good starting point at familiarising yourself with the project first.
Within the GUI, supporting drag&drop of attachment files and displaying image files (or other media) directly inside the GUI would be nice, and fairly isolated from other functions so you don't need to look much at other code. The current way that attachments are displayed (both in the new message dialog, and in the decrypt view) may be entirely replaced if you find another way of implementing them better.
Please always use the b mode for file I/O as in the original code, instead of text mode I/O that has various issues (like handling newlines differently depending on platform, which we don't want).
|
Closing as no further comments have been made and it doesn't seem likely for this to advance. @technokowski Feel free to make another PR if you wish to work on some other part, or reopen this if you plan to advance with this one. Any contributions are appreciated. |
Implemented a thread for opening a file using mmap in the decrypt_file() function in /gui/app.py. It was listed as a TODO item to replace the with open() call.