Skip to content

draft pr - bug fix - image load at startup #91

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from

Conversation

maifeeulasad
Copy link

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.

small bug - it loads image on startup and all other requirments met except failed to reload if url edited, gets stuck in loading icon
need to add a logic for that,
make structure simple
@math2001
Copy link
Owner

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 load method 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
Copy link
Author

I think let's just leave it here, as it not satisfactory, not even to me.

I am working on it, I will let you know, if I do some improvement.

You can wish me luck and luck for you exam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants