Skip to content
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

feat(caa dims): add support for new event art archive (EAA) #779

Merged
merged 3 commits into from
Jul 25, 2024

Conversation

jesus2099
Copy link
Contributor

@jesus2099 jesus2099 commented Jul 21, 2024

@vzell request:

https://community.metabrainz.org/t/ropdebees-userscripts-support-thread/551947/182

Could the userscript MB: Display CAA image dimensions be extended to the new event-art-archive?

Tested on https://musicbrainz.org/event/d1bd6d40-55f6-4e1a-91df-4c9395f95401 and sub-pages (/event-art, /edits)

@kellnerd
Copy link
Collaborator

/deploy-preview

You can use npm run build to build the userscripts locally.

I will see if I find some time to review your PRs as ROpdebee seems to be busy.

github-actions bot added a commit that referenced this pull request Jul 21, 2024
feat(caa dims): add support for new event art archive (EAA) (#779)
Copy link

github-actions bot commented Jul 21, 2024

This PR changes 1 built userscript(s):

See all changes

@kellnerd kellnerd added enhancement New feature or request mb_caa_dimensions labels Jul 21, 2024
Copy link
Collaborator

@kellnerd kellnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I've tested the preview and everything works as expected.
Regarding the code I have one minor suggestion.

P.S. There are a few more places where the wording could be changed from specifically CAA to the more general "image archives" (the name which is also used on MetaBrainz Jira), but that is not required here and I will leave that decision up to ROpdebee.

@@ -110,7 +110,7 @@ function urlToCacheKey(fullSizeUrl: string, thumbnailUrl?: string): string {
// Ideally, the cache key for RG covers would be the full size URL of the release cover,
// but we unfortunately cannot get the original image's extension here, so we cannot construct
// it.
if (urlObject.host === CAA_DOMAIN && urlObject.pathname.startsWith('/release-group/')) {
if (AA_DOMAINS.test(urlObject.host) && urlObject.pathname.startsWith('/release-group/')) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this code branch only deals with release group cover art, you can revert this change (and restore the CAA_DOMAIN constant in addition to the regex).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not.
But that would make two constants (CAA_DOMAIN and AA_DOMAINS*) instead of one.
What do you think @ROpdebee?

* AA_DOMAINSshould probably become IMG_DOMAINS, see #779 (review)
CAA project was renamed to IMG in https://tickets.metabrainz.org where IMG = CAA + EAA 😉

Copy link
Contributor Author

@jesus2099 jesus2099 Jul 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I included this revert change in 2041f59.

jesus2099 added a commit to jesus2099/ROpdebee_mb-userscripts that referenced this pull request Jul 21, 2024
- Revert unnecessary release group change
  ROpdebee#779 (comment)

- Use IMG (image archives https://tickets.metabrainz.org/projects/IMG)
  instead of AA (art archives)
  ROpdebee#779 (review)
@jesus2099
Copy link
Contributor Author

You can use npm run build to build the userscripts locally.

I didn't manage tu build (npm run build failed). :-/

$ npm run build

> [email protected] build
> NODE_ENV=production tsx build/build.ts

'NODE_ENV' n’est pas reconnu en tant que commande interne
ou externe, un programme exécutable ou un fichier de commandes.

I tried some tips from stackoverflow but nevermind.

@jesus2099
Copy link
Contributor Author

P.S. There are a few more places where the wording could be changed from specifically CAA to the more general "image archives" (the name which is also used on MetaBrainz Jira), but that is not required here and I will leave that decision up to ROpdebee.

I renamed my made-up AA into IMG, like in https://tickets.metabrainz.org/projects/IMG
Please check.

@kellnerd
Copy link
Collaborator

/deploy-preview

Looks good to me. I will give @ROpdebee a chance to react before I will merge, though.

I didn't manage tu build (npm run build failed). :-/

Your terminal does not support the NODE_ENV=production syntax before the actual command (which just sets an environment variable), this happens on Windows computers for example.

Alternatively you can also run npm run build-dev which is basically the same command but without the environment variable. The difference doesn't matter for local testing, it just skips minification of some code blocks IIRC.
(And yes, you might have to run npm install if your node_modules/ folder is not up-to-date.)

github-actions bot added a commit that referenced this pull request Jul 22, 2024
feat(caa dims): add support for new event art archive (EAA) (#779)
- Revert unnecessary release group change
  ROpdebee#779 (comment)

- Use IMG (image archives https://tickets.metabrainz.org/projects/IMG)
  instead of AA (art archives)
  ROpdebee#779 (review)
@jesus2099 jesus2099 force-pushed the add-eaa-support-on-main-branch branch from 2041f59 to 7ec78bd Compare July 22, 2024 10:05
@jesus2099
Copy link
Contributor Author

I forced-pushed an amended commit that is preserving original constant order.
I don't remember why I swapped them, probably on oversight.

I failed building again.
It seems not possible on Windows, even in git bash, like here:

$ npm run build-dev

> [email protected] build-dev
> tsx build/build.ts

Building mb_caa_dimensions
Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only URLs with a scheme in: file, data, and node are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'c:'
    at throwIfUnsupportedURLScheme (node:internal/modules/esm/load:236:11)
    at defaultLoad (node:internal/modules/esm/load:128:3)
    at nextLoad (node:internal/modules/esm/hooks:865:28)
    at Z (file:///C:/Users/▒▒▒▒▒▒▒/git/_forks/ROpdebee_mb-userscripts/node_modules/tsx/dist/esm/index.mjs:5:1650)
    at nextLoad (node:internal/modules/esm/hooks:865:28)
    at Hooks.load (node:internal/modules/esm/hooks:448:26)
    at MessagePort.handleMessage (node:internal/modules/esm/worker:196:24)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:814:20)
    at exports.emitMessage (node:internal/per_context/messageport:23:28) {
  code: 'PLUGIN_ERROR',
  pluginCode: 'ERR_UNSUPPORTED_ESM_URL_SCHEME',
  plugin: 'UserscriptPlugin',
  hook: 'transform',
  id: '\x00virtual:index.js'
}

Copy link
Collaborator

@kellnerd kellnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not delay this feature further, it is definitely good enough for the initial release.
Code clean-up can be done later (or not).

@kellnerd kellnerd merged commit 3bcc540 into ROpdebee:main Jul 25, 2024
11 checks passed
kellnerd pushed a commit that referenced this pull request Jul 25, 2024
Apply eslint unreadable unicorn/prefer-regexp-test preference
To pass the tests in #779
github-actions bot added a commit that referenced this pull request Jul 25, 2024
feat(caa dims): add support for new event art archive (EAA) (#779)
Copy link

🚀 Released 1 new userscript version(s):

  • mb_caa_dimensions 2024.7.25 in 412e67a

@jesus2099 jesus2099 deleted the add-eaa-support-on-main-branch branch July 25, 2024 15:44
@jesus2099
Copy link
Contributor Author

Which code cleanup?

@kellnerd
Copy link
Collaborator

Oh, maybe cleanup wasn't the best term, what I meant is rather renaming/rewording.
Like updating all identifiers and comments which are still talking about CAA and releases but also cover EAA and events now.

@jesus2099
Copy link
Contributor Author

Ah OK! I forgot about that. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants