-
Notifications
You must be signed in to change notification settings - Fork 6
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
Search for plugins to install on demand #51
Conversation
I like this @goanpeca |
Glad you like it, indeed is snappy :)
Yes!, this was a quick POC to see how it could work hence all the UI issues remain like this one you correctly point out. If this is something we would like to pursue, I would need to merge the other remaining PRs and then continue here, although I would probably just do a full rebase from main and add the few changes (those are actually small now that the other PRs ara almost in) Also @Czaki suggested we could have on the bottom a "suggested" plugins or, most downloaded or something, so that it is not empty empty, but showcases the most installed ones. Anyway food for thought :) Thanks for the reviews! |
51c2830
to
1cd209d
Compare
1cd209d
to
f0ef8bb
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
==========================================
- Coverage 93.54% 89.54% -4.00%
==========================================
Files 10 10
Lines 1813 1856 +43
==========================================
- Hits 1696 1662 -34
- Misses 117 194 +77 ☔ View full report in Codecov by Sentry. |
e99d937
to
a27b6b6
Compare
Meant to post this here: The gist was that the current approach is likely untenable in the long term, but the merged-to-main fixes are a significant improvement, so lets look at a release? Then, approach here can be a followup, with some extra UI polish prior to being released, such as additional filter categories--assuming they are available anyways. |
@psobolewskiPhD haha yes I understood :) All good making a release today 🚀 |
d47ab79
to
9ffe960
Compare
1b55efe
to
7908294
Compare
0d96d14
to
bdf3844
Compare
Ok, back to work here :) @dalthviz @psobolewskiPhD do you think you could give it a try to see how it feels, works? Thanks! |
Did a quick check and seems like things are working as displayed over the OP video 👍 However, a couple of things I noticed (maybe the last one not completly related with the work here):
A preview of the behaviors described above: |
Hi there, with the little change pushed above. now things here work something like: As seen over the preview, I'm still experiencing the issue with the connectivity. Still not sure if that's something on my end or over the web service that provides the plugins info to be honest 🤔 Anyhow, let me know if the change pushed above makes sense @jaimergp @goanpeca |
I doubt Vercel is having issues, and I don't see anything in https://www.vercel-status.com/ |
@psobolewskiPhD @Czaki any further comments? |
If i type something into the filter and click Edit: |
Just in case, regarding the connectivity issue, I was able to reproduce it using the main branch so I think is not completly related with this (although maybe is more easily triggered with this PR for some reason?). Also, checking what is being catch to show the connectivity issue message, the exception says: HTTP Error 500: Internal Server Error Further checking with my browser on an incognito window (Firefox), the first time I try to get something from the web service (for example 🤔 |
Maybe changing the search lineedit placeholder text could help with that too? So something like: |
Co-authored-by: Peter Sobolewski <[email protected]> Co-authored-by: jaimergp <[email protected]>
for more information, see https://pre-commit.ci
Seems like the latest CI fail was caused due to a HTTP 500 error triggered while testing the np2api endpoints (particulary related with the https://npe2api.vercel.app/api/conda endpoint): https://github.com/napari/napari-plugin-manager/actions/runs/11840208794/job/32993483316?pr=51#step:7:2371 Launched a job re-run and the test now passes: https://github.com/napari/napari-plugin-manager/actions/runs/11840208794/job/32995746148?pr=51 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you like it, I like it. LGTM! I'll let someone from @napari/core-devs do the honors with the merge, though.
@psobolewskiPhD do you want to do the honours here? (I have little experience with the plugin manager, to put it mildly.) |
Hi @dalthviz thanbks for updating the PR. Indeed I also experienced the 500 errors, it seems the endpoint was crashing more and more, and this PR for some reason I do not fully comprehend, was making the error more apparent. The refresh button can solve this but in some cases the plugin list does not populate because of this. I am not sure if the endpoint is doing some blocking in which case we could/should evaluate a different provider? or see if vercel needs to have some rules added with respect to "known requesters". That is why I added the flaky test checker :) PR looks good! |
I'll start a 48h timer. If we don't hear back in that time we'll do the merge by Thursday EOD. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, I like it!
Thanks y'all!
Fixes #83
Fixes #76
As discussed in #37 as more and more plugins are added to the ecosystem loading a big list of plugins might become unsustainable, specially if we keep using complex custom and styles widgets within the
Qlistwidgetitems
.Another approach to this, that could still preserve the rich styling of widgets is to only display the items after a search, that way we only populate the list on demand if a user is looking for something. This is what other tools like VSCode handle it. Added a small preview, of course this would require some styling, but this work fits nicely into what has already been provided by the latest PRs