YACReader
Fix a crash and clean up YACReaderFlowGL
There is also a crash on exit from YACReader Library after removing a comic or a folder. I have already fixed it and will create a pull request soon.
How did you tested it? Moving quickly between folders in YACReaderLibrary, or fast opening next/previous comic in YACReader while the flow is shown (using CTRL + Left/Right) can be good ways of testing this.
I have some memories about the worker loading an image from the previous content while the flow is repopulated, causing a wrong image being displayed in the flow, because once the right one finishes loading the flow will reject it because it thinks it is already loaded.
How did you tested it? Moving quickly between folders in YACReaderLibrary, or fast opening next/previous comic in YACReader while the flow is shown (using CTRL + Left/Right) can be good ways of testing this.
Just tested Library and Reader like you described at this branch. Haven't spotted a wrong image near the beginning. But detecting that a wrong image is displayed somewhere far from the first one is obviously difficult.
I have some memories about the worker loading an image from the previous content while the flow is repopulated, causing a wrong image being displayed in the flow, because once the right one finishes loading the flow will reject it because it thinks it is already loaded.
Your description resembles a symptom of a thread safety bug. Sadly, in the presence of data races any refactoring or fix can cause a regression. I still plan to implement a follow-up pull request to #99. But I have forgotten the details of that fix/refactoring, so not sure whether it would guarantee thread safety here.
Sadly, in the presence of data races any refactoring or fix can cause a regression.
That's why I usually don't like changing code just for the sake of it. This worker implementation comes from the original PictureFlow implementation, and it's been years since I touched it. So I don't think the benefits of these kind of changes are worth the risks.
But I am happy to see the crash fixed :), nice catch!
This pull request does not touch the workers. The changes here seem to be pretty safe to me. The removed if (images[item].index == item) check in YACReaderFlowGL::replace() looks like a leftover from a different implementation, because the current code in develop consistently updates the index data member on each insertion/removal/reordering in YACReaderFlowGL::images. Except for the one place that crashes in Debug mode.
That's why I usually don't like changing code just for the sake of it. This worker implementation comes from the original PictureFlow implementation, and it's been years since I touched it. So I don't think the benefits of these kind of changes are worth the risks.
My reimplementation of ComicFlow's worker doesn't seem to be causing issues in YACReader 9.7, does it? Keeping the old code intact when it can be proven to be thread-unsafe isn't a viable long-term strategy. Things can break on a compiler or framework or hardware update. And it will be difficult to figure out what exactly is happening and how to fix it. Best to get rid of data races altogether. Encapsulating and unifying multithreading code makes this easier by eliminating the need to analyze duplicate code and allowing to inspect loosely coupled classes more or less separately.
I have some memories about the worker loading an image from the previous content while the flow is repopulated, causing a wrong image being displayed in the flow, because once the right one finishes loading the flow will reject it because it thinks it is already loaded.
Just looked at the code again and realized that you might have confused the removed YACReader3DImage:index with a worker's idx (which is not affected by this pull request). The worker's idx is reset (set to -1) every time the data is changed. YACReaderFlowGL::loaded is adjusted too when a change happens. These ensure that a wrong image is not displayed. As far as I can tell, this code is completely unrelated to the removed index.