Skip to content

Conversation

@taooceros
Copy link
Member

This branch is intended to fix #313, but haven't done that job.

  1. Don't await download manifest when it is unavailable, but only await when calling install method to allow user using uninstall when manifest is not ready.

@taooceros taooceros added this to the 1.7.0 milestone Jan 29, 2021
@taooceros taooceros added the enhancement New feature or request label Jan 29, 2021
@taooceros taooceros modified the milestones: 1.7.0, 1.8.0 Feb 3, 2021
@jjw24
Copy link
Member

jjw24 commented Feb 3, 2021

@taooceros consistently hitting this in vs debug:
image

@jjw24
Copy link
Member

jjw24 commented Feb 3, 2021

this does not happen with dev branch

@taooceros
Copy link
Member Author

Oh, it was canceled? I will take a look on why this is canceled because it is not supposed to be canceled.

@taooceros
Copy link
Member Author

this does not happen with dev branch

So you mean that this PR fix it?

@jjw24
Copy link
Member

jjw24 commented Feb 3, 2021

No as in the dev branch doesn't run into this cancellation

@taooceros
Copy link
Member Author

No as in the dev branch doesn't run into this cancellation

I am not sure about why there exists the cancellation, because the Get method should not include a cancellation.

@taooceros
Copy link
Member Author

@jjw24 I still don't understand why there will be cancellation...... Is this replicable? If it is, would you please check whether the manifest has been downloaded successfully? If it has downloaded successfully but canceled, let me make the error message only appears on fault?

@jjw24
Copy link
Member

jjw24 commented Feb 3, 2021

I can replicate, will take a look, but I won't be able to do it until later tonight or tomorrow. In the mean time let's roll out the release because I have tested it for a while and haven't run into any issues so pretty comfortable with it. This change can go out with the next release.

What do you think?

@taooceros
Copy link
Member Author

I can replicate, will take a look, but I won't be able to do it until later tonight or tomorrow. In the mean time let's roll out the release because I have tested it for a while and haven't run into any issues so pretty comfortable with it. This change can go out with the next release.

What do you think?

Yeah, sure. I also quite excited on the new 1.7.0 release, as it brought out the new plugin model.

@taooceros taooceros modified the milestones: 1.7.0, 1.8.0 Feb 3, 2021
@taooceros
Copy link
Member Author

Or maybe you can just merge the optimization, and let's solve the issue next time.

@jjw24
Copy link
Member

jjw24 commented Feb 3, 2021

Or maybe you can just merge the optimization, and let's solve the issue next time.

I would definitely not be comfortable with merging with this issue outstanding if I can replicate it with this PR and not with the dev branch build haha

@taooceros
Copy link
Member Author

Or maybe you can just merge the optimization, and let's solve the issue next time.

I would definitely not be comfortable with merging with this issue outstanding if I can replicate it with this PR and not with the dev branch build haha

Haha great. Let's keep it with next release!

@jjw24
Copy link
Member

jjw24 commented Feb 14, 2021

I have not experienced issue 313 any more, do we still want to make this change?

@taooceros
Copy link
Member Author

I have not experienced issue 313 any more, do we still want to make this change?

Ah yes, I have made a couple changes to make the singleton works better. You can take a look on that.

@jjw24
Copy link
Member

jjw24 commented Feb 15, 2021

image

Should we add RanToCompletion as a pass condition as well?

@taooceros
Copy link
Member Author

image

Should we add RanToCompletion as a pass condition as well?

No, because this is used to keep one task running, so that we won't have multiple tasks to update the manifest. If the task is ended, we need to start a new one.

@jjw24
Copy link
Member

jjw24 commented Feb 15, 2021

oh right i see

@jjw24 jjw24 enabled auto-merge February 15, 2021 10:15
@jjw24 jjw24 mentioned this pull request Feb 15, 2021
@jjw24 jjw24 merged commit 55b0a91 into Flow-Launcher:dev Feb 15, 2021
@taooceros taooceros deleted the FixUnexpectedErrorNotice branch February 25, 2021 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plugins Manager check connection message pops up despite manifest loaded

2 participants