OK. Well, in general, I agree that the design is somewhat flawed. I came across many of the same questions as you - only without the benefit of your in-depth understanding of Picard's guts :P At the very least, the current design can benefit from some more changes - some of which I discussed in the PR. At the most, possibly a complete re-do with only a recycling of the parse code could be in order.

However, my efforts here were mainly targeted towards "just get it working". Not sure where you're at on your release cycle, but it may be prudent to get the current changes into the next release and then allow some time for discussion on the design & subsequent associated implementation. It may be helpful to hear from the original authors, if they are still around, on their rationale on the existing design and as well, they may want to make some changes too.

Of course... holding back the changes in this PR on all that may not be prudent as the plugin is pretty broken in the current state & with my current PR changes, it does work much better.

Having said that.. if you feel strongly to hold off til a more robust solution is in place and we don't hear from the original authors - I'd be happy to dig in and revamp it. Of course, I'd need some time.

I will go ahead and provide some feedback on your feedback in the mean time :O)

Since plugin changes can be published independently of Picard we are not bound to any release schedule here. I'm happy to merge this as a step forward if the config issue gets resolved.

But I still think this needs some more cleanup before we can consider it fixed.

Good call. I will try to help you out further & rework it some more if we don't hear from the authors on any of this.

One more thing... you have a design flaw with all the calls to the protected album member _finalize_loading. This can be dangerous. You should consider a wrapper for this. You could use the wrapper for additional checks and logic like checking for recursions to avert plugins sinking the whole ship, as we saw here when using the 'Other releases" context item.

you have a design flaw with all the calls to the protected album member _finalize_loading. This can be dangerous. You should consider a wrapper for this.

See the existing ticket at https://tickets.metabrainz.org/browse/PICARD-1403 . Suggestions and code welcome. Especially how to best handle that we ensure only one request decrement for each increment.

OK good!

This plugin is somewhat tricky. I noticed another issue, which actually might be the root problem of some of the issues users have. To check if an album can be removed from the plugins list we have something like this in the code:

if not album._requests:
                self.albums[item_id].remove(album)
                album._finalize_loading(None)

But this will only ever work reliable if wikidata is the only thing dong requests. But this is seldom the case. So if wikidata finishes the requests, but other plugins or covert loaders are still busy, the album will stay in wikidata's list. Any attempt of later reloading this album will get stuck.

Having _finalize_loading already getting called in process_request seems to have been an attempt to fix this. But the proper way is probably for the wikidata plugin to track only it's own requests for an album (in addition to manipulating the album request counter).

I did some debugging here, I will see if I can submit a fix for this directly to this pull request tomorrow.

Not sure I fully understand the dependency of one plugin on other plugins that you are describing, or even the design intent of album._requests. Seems like you would want to have it all managed in the album object or higher & just let the plugin mark an album if a change is needed, so the album would open and finalize as needed - managing requests internally & also eliminate any plugin inter-dependencies. But it seems like the plugins have too much power with the ability to block the main thread, leave albums open, etc... Not being critical, just saying this approach could perhaps be improved over time. In the mean time, some more exhaustive testing of new plugins or plugin mods should be considered.

I have continued to test the current revision of wikidata (the one in this PR) against subsets of a large media library & its been working flawlessly for me. Its truly a massive improvement over what's currently being made available to users. I could test the reloading problem you are describing if you can explain the test procedure more clearly. I am running a couple other plugins - the fanart.tv one, mainly.

I was going to start on the additional improvements we discussed this next week. I was going to create a new JIRA ticket to track that. But if you are going to make changes here maybe its better to wait on those...

My attempt of improving this: pbrackin#1

I am still testing, but so far its been working good. One thing I found though... if there is a network problem, a track may get an incomplete genre set from what it would otherwise get. To the user this shows up like a change and the change is a reduced genre set.

The problem seems to be that the webservice does not send an error indication via the callback so wikidata is kind of in the dark that there was a problem.

Below is a snippet from the log. Here again, I am not sure about the design intent, so maybe you can guide me in on if this is considered "normal" behavior for the plugin to not get an error indication.

I noticed that if I simulate a complete network outage by driving the musicbrainz.org requests to null then I get the red album discs in the UI. This seems goods so as to indicate to the user they need to retry later. But for the case I am describing here, its not turning red.

Probably can address this in a separate ticket, but wanted to bring it up since I see it in my testing on occasion.

E: 07:45:44,103 C:\Users\paul\Documents\GitHub\picard\picard\webservice\__init__._handle_reply:383: Network request error for https://musicbrainz.org:443/ws/2/release-group/467a5697-30e7-34b8-9813-d07e6af9c653?inc=url-rels: Error transferring https://musicbrainz.org:443/ws/2/release-group/467a5697-30e7-34b8-9813-d07e6af9c653?inc=url-rels - server replied: Service Temporarily Unavailable (QT code 403, HTTP code 200)
D: 07:45:44,105 C:\Users\paul\Documents\GitHub\picard\picard\webservice\__init__._handle_reply:395: Retrying https://musicbrainz.org:443/ws/2/release-group/467a5697-30e7-34b8-9813-d07e6af9c653?inc=url-rels (#2)

The problem seems to be that the webservice does not send an error indication via the callback so wikidata is kind of in the dark that there was a problem.

Below is a snippet from the log. Here again, I am not sure about the design intent, so maybe you can guide me in on if this is considered "normal" behavior for the plugin to not get an error indication.

The problem here is that this plugin never passes any error to album._finalize_loading(). To actually show an error the plugin is supposed to call album._finalize_loading(error). Note that currently the call to _finalize_loading is guarded by a check against album._requests being 0 (see also my comment above), which strictly speaking is not necessary since this is checked in _finalize_loading itself. That is especially true if passing an error, which must be done regardless the status of album._requests.

Regarding the design intent you are asking the wrong person, until a few weeks ago I had never looked at the wikidata code in detail. We are kind of in the same boat here :)

LGTM!

But how can the plugin pass an error if it never gets an error back from the webservice to pass? From what I could tell, the callback just never gets called for these cases. I suppose you could add some sort of timer and make an error on timeout, but seems like the right solution is for the webservice to notify the plugin?

But how can the plugin pass an error if it never gets an error back from the webservice to pass?

On a request error the callback will be called with the error object set, see the error handling with if error.

Yeah that is what I thought. That is not happening in this case.

Yeah that is what I thought. That is not happening in this case.

It definitely is supposed to happen, and it does happen here on my tests. If there is any network error (I tested by changing the wikidata.org host to something invalid, but could also be an error response like 500 from the server) the error object will be set and the wikidata logging happens.

If that really does not work for you, can you give some details about the exact error condition?

OK when I get a chance I will open a new ticket with more data on the "lack of callback error" for the one case I was describing. Thanks for y'alls help getting this PR in to make this plugin operational :O)

One more question... I saw the merge happened. When & how does this new plugin version become available? I saw the generate.py script in the plugin repo has to be run. Does that get run by your build system? Or do I need to do something with my local Picard to see it?

Unfortunately not, the website needs to be redeployed manually and I was told this is a rather painful process. We talked about this today and would like to streamline the process, but it'll require some work.

When & how does this new plugin version become available?

New plugin versions were deployed just now. As explained by @phw , it's a tedious process, we have plans to improve this, but for now, it just sucks ;)