search

Scotsguy / now-playing

Minecraft mod that shows a 'now playing' popup whenever music changes.
MIT License
11 stars 5 forks source link

Refactor and update to mc1.20.4 #38

Closed NotRyken closed 5 months ago

NotRyken commented 5 months ago

This PR updates the mod to 1.20.3/4 and restructures it for more efficient future maintenance.

Some notes:

The two force-pushes were attempts to retroactively verify the commit, the second of which succeeded.

JayJay1989 commented 5 months ago

Is it possible to change your pull request and use the corresponding branch

NotRyken commented 5 months ago

Given a mc1.20.4 branch on the main repo, sure thing.

Scotsguy commented 5 months ago

Could you edit the metadata so that the mod is called "Now Playing" and not "NowPlaying"? I'd do it myself but mod_name is used everywhere, including class files

Scotsguy commented 5 months ago

Small bug: The Hotbar notification style no longer has the "Now Playing" text, so it doesn't look like vanilla anymore

Vanilla record player: image

Now Playing, hotbar notification style: image

I'm going to commit and push this myself: cc178a2

NotRyken commented 5 months ago

Could you edit the metadata so that the mod is called "Now Playing" and not "NowPlaying"? I'd do it myself but mod_name is used everywhere, including class files

By this are you referring to the mod name, which is defined only in gradle.properties and NowPlaying.java (the latter being for the logger), or the mod id, or both?

Scotsguy commented 5 months ago

Could you edit the metadata so that the mod is called "Now Playing" and not "NowPlaying"? I'd do it myself but mod_name is used everywhere, including class files

By this are you referring to the mod name, which is defined only in gradle.properties and NowPlaying.java (the latter being for the logger), or the mod id, or both?

The mod name that's shown to the player, as appears in the mod list and mods.toml/fabric.mod.json. It had a space before the refactor, which is missing now. The mod id hasn't changed.

NotRyken commented 5 months ago

In this PR the mod id is nowplaying, the former was now-playing

Scotsguy commented 5 months ago

In this PR the mod id is nowplaying, the former was now-playing

Oh, lol, I didn't notice that. For consistency, could you change it back to the old id too? Otherwise there could probably be issues if someone doesn't uninstall the old version before adding the new one.

Scotsguy commented 5 months ago

I've pushed a small addition I noticed while working on the small fix above, hope you don't mind :)

NotRyken commented 5 months ago

Because Neo doesn't allow hyphens in mod IDs I've added an underscored variation for it

Scotsguy commented 5 months ago

Is there anything I need to set up in terms of API keys before merging this? Otherwise I think it's good to go.

NotRyken commented 5 months ago

Modrinth publishing requires a PAT secret MODRINTH_TOKEN, label sync and GitHub publishing use the default GITHUB_TOKEN assuming it has read and edit perms.

NotRyken commented 5 months ago

I wasn't expecting v1.5.0 to be compatible with mc1.20.5/6 but on Fabric/Quilt it is. It isn't compatible with NeoForge 1.20.6, so I guess the question is whether to release v1.5.1 for both Fabric and Neo, or just Neo and add 1.20.5 and 6 to the list of compatible MC versions on the v1.5.0-Fabric-1.20.4 Modrinth release.

I have a mc1.20.6 branch ready to PR from my fork assuming the former case (i.e. no need to temporarily remove Fabric from the release action), changes are minimal.

Scotsguy commented 5 months ago

It's probably fine to release 1.5.1 for fabric/quilt without changes, that way no one asks why there's no 1.5.1 for fabric 1.21 has new music, so that'll need a new release anyway :)