murdos / musicbrainz-userscripts

Collection of userscripts for MusicBrainz, by various authors
554 stars 92 forks source link

Discogs: fix missing links on various pages #546

Closed atj closed 1 year ago

atj commented 1 year ago

This PR should cover all entity links on artist, label, master and release pages. I replaced all the existing calls to add_mblinks as there was a lot of cruft and it was easier to start afresh. I've grouped the calls by page to make it easier to maintain in future. If I've missed anything please let me know.

Calls to setTimeout are for elements which are loaded dynamically after the initial page rendering. Timeouts have been chosen arbitrarily - setting them any lower caused random failures on my machine. Calls to setInterval are for elements that are rendered dynamically based on user input (e.g. clicking the "N versions" button on master releases). This approach is not ideal but I don't see another way without making major changes. Again, intervals were set arbitraily and could probably be increased.

vzell commented 1 year ago

This looks promising ... When can we expect a merge and new release of the plugin .. I'm struggling a lot with the new artist layout over at Discogs recently?

atj commented 1 year ago

kellnerd is aware of the PR and will review the changes when he has the bandwidth. In the meantime, if you want to use my updated version, it can be installed from here:

https://github.com/atj/musicbrainz-userscripts/raw/fix-discogs/discogs_importer.user.js

The links in the metadata header have not been changed and still point to this repo, so your userscript manager will still be able to query and download future versions.

vzell commented 1 year ago

@atj: Juhuuuuuuuu ... You made my day ... it works like a charm ... I was in the middle of synching Springsteen releases (thousends) when Discogs decided to switch to the new artist layout ... what a night mare ... but now back to work ... :-) thx for doing this

vzell commented 1 year ago

Actually for quite long listy for example this one

atj commented 1 year ago

@vzell: Thanks for the link. The number of release versions on that page is so big that it takes much longer to load. I just updated the userscript to wait until the versions progress bar is hidden before selecting the links. It seems to work for me, can you install the updated version using the same link above and let me know it it resolves the issue?

vzell commented 1 year ago

Hi Thx for the prompt reaction ... that link now loads like a charm

vzell commented 1 year ago

Hi me again The Discogs artist pages allow to choose from a drop down list the number of releases to display/show. With your latest patches the Musicbrainz identifiers getting rendered for 25, 50 and 100 but NOT 250 and 500. Is that expected ? Also when I'm on a page with rendered Musicbrainz identifiers (say displaying 100) and then switching to some other value, lets say 25, nothing gets rendered, ONLY if I do a SHIFT refresh on the page with the new value 25. Is that expected ?

atj commented 1 year ago

When you change the number of items or select one of the filters on the artist page the content is updated dynamically via Javascript. Userscripts are executed on initial page load and there isn't a straightforward way for them to detect or be notified of dynamic updates like this. The last update I made to fix the issue on the master release page literally just runs a function every second to see if the progress bar element is hidden, as this is the best indicator I can find that the releases have finished loading. Unfortunately these kinds of hacks are pretty much the best we can do given the limitations mentioned above.

So, yes, I'm afraid it is expected behaviour that the userscript doesn't handle dynamic page updates. However leave it with me and I'll see if I can resolve these particular issues.

vzell commented 1 year ago

Thx for the info ... and thx for working on this ...

atj commented 1 year ago

@vzell: can you update the script again please? I think I've fixed the issues you reported.

vzell commented 1 year ago

Give Me a Second... I need to switch on my laptop

vzell commented 1 year ago

I tried with https://www.discogs.com/artist/760706-Bruce-Springsteen-The-E-Street-Band?superFilter=Unofficial and choose 500 from the pull down list ..... and IT WORKS ... good work ... thx a lot

kellnerd commented 1 year ago

Calls to setInterval are for elements that are rendered dynamically based on user input (e.g. clicking the "N versions" button on master releases). This approach is not ideal but I don't see another way without making major changes.

We could probably make use of MutationObserver instead, but this PR is already a great improvement as is, so I don't want to unnecessarily delay it further.

atj commented 1 year ago

Thanks for the review. I've committed your suggestions and reworded the commit messages for consistency.

We could probably make use of MutationObserver instead

MutationObserver is what I had in mind when I wrote that comment. It seems like the only reliable option when dealing with modern sites that dynamically update the DOM. However in my limited experience it requires a fair bit of effort to make it work reliably.

vzell commented 1 year ago

Hi Thx for the merge ... Is it possible to extend the support of Musicbrainz identifiers to the Discogs submission pages like for example

kellnerd commented 1 year ago

@atj Whoops, I've just found a local version 2022.7.29.1 which fixes some of these links which were broken for over a year, lol. For example we missed the release artist link in this PR. I've pushed some additional changes from my local version directly to master in 0eae2b96ba55b18d06c25132f0e0e1ebe1a71a6f.

@vzell Your link is broken for me, but probably? I don't know whether that makes sense though.

vzell commented 1 year ago

@kellnerd Maybe that link only works if your logged in with my account .. just try the generic https://www.discogs.com/submissions link