Skip to content

Conversation

@JohnTheGr8
Copy link
Member

The main change introduced here is the use of conditional http requests when fetching the plugin manifest json. This allows us to always check for changes to the manifest repo when running one of pm install, pm update, with very little performance overhead (for me it is about 50ms). It also allows us to simplify the relevant code a bit.

Also some quick refactoring in PluginsManager and a couple quality-of-life enhancements:

  1. typing pm inst and hitting tab would autocomplete to pm install without a space at the end (same for the other base commands)
  2. fix incorrect link (for some plugins) when selecting to open an issue from the context menu

PS: I missed #682 until I started writing this PR text... my apologies. I see similarities, perhaps we can pick some of the missing changes into this PR?

@JohnTheGr8 JohnTheGr8 added the enhancement New feature or request label Jan 11, 2022
@JohnTheGr8 JohnTheGr8 requested review from jjw24 and taooceros January 11, 2022 07:14
@JohnTheGr8 JohnTheGr8 self-assigned this Jan 11, 2022
@taooceros
Copy link
Member

Should we expand the helper class instead of creating a new httpclient for plugins manager only? Also this may not follow the proxy setting.

@JohnTheGr8
Copy link
Member Author

Should we expand the helper class instead of creating a new httpclient for plugins manager only? Also this may not follow the proxy setting.

Yup, you're right, I wanted to keep it simple but I forgot about proxy settings... Will add a helper method inside Http that accepts a HttpRequestMessage

@JohnTheGr8
Copy link
Member Author

There's one issue I just noticed: launching Flow and opening Settings > Plugin Store (without first using pm install/update) will show no plugins...

Possible workarounds:

  1. actually fetch the plugin data in PluginsManager's InitAsync (which will add a few ms to the init time)
  2. call PluginsManifest.UpdateManifestAsync() when the user opens settings (or something similar)

thoughts?

@taooceros
Copy link
Member

I would consider that we just call download manifest in init? We can decide not to await that so it won't change init time.

@taooceros taooceros added this to the 1.10.0 milestone Jan 12, 2022
@JohnTheGr8
Copy link
Member Author

I would consider that we just call download manifest in init? We can decide not to await that so it won't change init time.

Alright, was hoping for this since I am not very familiar with WPF 😛 It's pretty fast with the CDN link, anyways...

@taooceros
Copy link
Member

I would consider that we just call download manifest in init? We can decide not to await that so it won't change init time.

Alright, was hoping for this since I am not very familiar with WPF 😛 It's pretty fast with the CDN link, anyways...

lol it's not related to WPF. Flow waits for plugin init before showing up.

@JohnTheGr8
Copy link
Member Author

I would consider that we just call download manifest in init? We can decide not to await that so it won't change init time.

Alright, was hoping for this since I am not very familiar with WPF 😛 It's pretty fast with the CDN link, anyways...

lol it's not related to WPF. Flow waits for plugin init before showing up.

I meant my other solution, the second one ;)

Rename them to something more accurate, switch them to be const
@JohnTheGr8 JohnTheGr8 requested a review from taooceros January 12, 2022 21:49
@JohnTheGr8 JohnTheGr8 requested a review from taooceros January 12, 2022 22:45
taooceros
taooceros previously approved these changes Jan 13, 2022
@jjw24 jjw24 enabled auto-merge January 13, 2022 20:59
@jjw24 jjw24 disabled auto-merge January 13, 2022 21:02
@jjw24 jjw24 enabled auto-merge January 13, 2022 21:03
@jjw24 jjw24 merged commit b9048f5 into dev Jan 13, 2022
@jjw24 jjw24 deleted the refactor_plugin_manifest branch January 13, 2022 21:12
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

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants