math2001
commented
4 years ago Hey @maifeeulasad, thanks for taking the time to contribute!
I haven't tested this code myself, and although I can see that it seems to resolve the problem, it's not the best solution 🙁 . That's OK, here's what is happening vs what I think we should do:
What is happening
it recursively does the job
There is no need for recursion here. Just to make sure I understand everything right, here's what I think your code does:
- The new
loadmethod checks if the ImageManager has finished loading the image, and if it has, update the preview. And then, it starts a new thread 0.5s later to call itself again. - The get method tries the return the image, but if it isn't done, waits 1 second and then calls itself again. Hence, by becoming recursive, it defeats its original purpose, since it blocks. It shouldn't (it has to return None straight away if it hasn't finished loading).
What we should do instead
As soon as the ImageManager notices that it has finished loading an image, it should trigger a callback (a function given by the EventListener) which would re-render the preview. There is no need to "poll" (check again and again in a new thread). It's a lot cheaper because there is just one thread which triggers events, instead of multiple threads polling another one.
One problem remains - it sticks in loading icon, then it need key press.
This cheaper solution will solve this problem at the same time 🙂
As I said in #90, implementing this cheaper solution is going to be hard because this package is pretty much exclusively disorganized code. Hence, I want to re-write the entire thing cleanly. Unfortunately, I won't be free until the end of November 2020, so I doubt it will happen until then.
So, I'm not going to merge this because it makes the entire plugin a lot more expensive to fix a small issue.
Do you agree? What do you think? I really don't want you to take this the wrong way though. This PR might not be the best solution for our problem, but that doesn't mean in any way that I don't want you to contribute 🙂
maifeeulasad
This starts during startup and start a separate thread which always checks for a static variable from image_manager.py weather it should update the view or not. It works.
It may seem slow, but it is intended - here you can see that, it recursively does the job. You can un-comment my comment and see it works something like intended previously
One problem remains - it sticks in loading icon, then it need key press. I think we can solve it.