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

Fix and Update the Installer #121

Merged
merged 63 commits into from
Oct 27, 2024
Merged

Conversation

Ecdcaeb
Copy link
Contributor

@Ecdcaeb Ecdcaeb commented Apr 21, 2024

Why this pr here

The existing installer is missing the licenses!
The existing installer icon is incorrect!

What this pr brings

Fix Extra Files

Forge

image

Cleanroom 3029

image

This PR

image

url is not imported by this pr.

Icon

image

from twitter, remove the background to adapt the install theme.

@Ecdcaeb
Copy link
Contributor Author

Ecdcaeb commented Oct 20, 2024

change to neoforge installer
image
but the field of icon is invalid, use https://github.com/neoforged/LegacyInstaller/blob/main/src/main/java/net/minecraftforge/installer/ui/Images.java#L31

TODO: find out how to replace it

@Ecdcaeb
Copy link
Contributor Author

Ecdcaeb commented Oct 21, 2024

due to https://github.com/neoforged/LegacyInstaller/pull/34#issuecomment-2425937887
I have done my best. Use it? Fork it?
Ready for rewiew

@Ecdcaeb Ecdcaeb changed the title Fix extra_files and icon for installer Fix and Update the Installer Oct 21, 2024
@Rongmario
Copy link
Member

Is there a reason why NeoForge's installer tools is used instead?

@Ecdcaeb
Copy link
Contributor Author

Ecdcaeb commented Oct 23, 2024

Is there a reason why NeoForge's installer tools is used instead?

1.support custom title
2.i10n
3.More support will be available. The MinecraftForge Installer is updated slowly.

@KorewaLidesu
Copy link
Collaborator

I don't think there is much change on NeoForge fork to consider switching since most of features already completed.
If the later development of CRL required a change on Installer, we will consider switch or create new one instead.

@Ecdcaeb
Copy link
Contributor Author

Ecdcaeb commented Oct 24, 2024

I don't think there is much change on NeoForge fork to consider switching since most of features already completed. If the later development of CRL required a change on Installer, we will consider switch or create new one instead.

This PR has been stagnant for a while, and LegacyInstaller can do all my needs. With my meager wisdom, I can't foresee the disadvantages here. I can only see the benefits of the conversion and the pleasing new features, while I can't understand why this change is unpleasant.

image

Done.

@KorewaLidesu
Copy link
Collaborator

Seem good, just move the icons to separate folder and I will check out for merging.

@KorewaLidesu KorewaLidesu self-requested a review October 24, 2024 21:08
@Ecdcaeb Ecdcaeb marked this pull request as draft October 25, 2024 16:06
@Ecdcaeb
Copy link
Contributor Author

Ecdcaeb commented Oct 26, 2024

https://github.com/neoforged/LegacyInstaller/issues/36

@Ecdcaeb
Copy link
Contributor Author

Ecdcaeb commented Oct 26, 2024

1.12.2 lacks official mapping, which will cause an exception when making an offline fat installer containing the mc body. https://github.com/neoforged/LegacyInstaller/blob/f4d8f3fae5e0497732dac454232aeac70dabb786/src/main/java/net/minecraftforge/installer/actions/FatInstallerAction.java#L65

external use of the installer is not really a supported use case
(this is related to your other issue too)
fork it if you want to use it and need changes

If this needs to be solved, although it is not commonly used, then a fork is needed.

@Ecdcaeb Ecdcaeb marked this pull request as ready for review October 26, 2024 11:39
@Ecdcaeb
Copy link
Contributor Author

Ecdcaeb commented Oct 26, 2024

Done

Copy link
Collaborator

@KorewaLidesu KorewaLidesu left a comment

Choose a reason for hiding this comment

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

LGTM.

icon copy.png Outdated Show resolved Hide resolved
icon.png Outdated Show resolved Hide resolved
@KorewaLidesu KorewaLidesu merged commit a245727 into CleanroomMC:main Oct 27, 2024
1 check passed
@Ecdcaeb Ecdcaeb deleted the extra_files&icon branch October 27, 2024 14:56
KorewaLidesu added a commit that referenced this pull request Oct 27, 2024
KorewaLidesu added a commit that referenced this pull request Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants