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.

Sign up for GitHub

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

Jump to bottom

Pagination for 1.6.4 controls screen #175

Merged
merged 4 commits into from
Jul 2, 2024
Merged

Pagination for 1.6.4 controls screen #175

merged 4 commits into from
Jul 2, 2024

Conversation

thecatcore
Copy link
Member

1.6.4 Controls screen doesn't support dynamic amount of keybindings, this pr provides a replacement for it to fix this issue.

Vanilla Screen:
Screenshot from 2024-06-27 21-17-05

New Screen (Ignore inverted color) :
Screenshot from 2024-06-29 19-53-46
Screenshot from 2024-06-29 19-53-56
Screenshot from 2024-06-29 19-54-03
Screenshot from 2024-06-29 19-54-11

@thecatcore thecatcore added bug Something isn't working MC1.6.4 Related to Minecraft 1.6.4 labels Jun 29, 2024
@thecatcore thecatcore added this to the 1.10.0 milestone Jun 29, 2024
@thecatcore thecatcore self-assigned this Jun 29, 2024
BluCobalt
BluCobalt approved these changes Jun 29, 2024
View reviewed changes
Copy link
Member

@BluCobalt BluCobalt left a comment

Choose a reason for hiding this comment

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

very cool

BoogieMonster1O1
BoogieMonster1O1 requested changes Jun 30, 2024
View reviewed changes
Copy link
Contributor

@BoogieMonster1O1 BoogieMonster1O1 left a comment

Choose a reason for hiding this comment

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

I don’t think completely bypassing the vanilla controls screen is the right approach.

.../1.6.4/src/main/java/net/legacyfabric/fabric/impl/client/keybinding/OptimizedListWidget.java Outdated Show resolved Hide resolved
...c/main/java/net/legacyfabric/fabric/impl/client/keybinding/DynamicControlsOptionsScreen.java Outdated Show resolved Hide resolved
...1.6.4/src/main/java/net/legacyfabric/fabric/mixin/client/keybinding/SettingsScreenMixin.java Outdated Show resolved Hide resolved
@thecatcore
Copy link
Member Author

I don’t think completely bypassing the vanilla controls screen is the right approach.

What do you think is the right approach in this case then?
The current implementation modifies the screen quite heavily, it would be difficult to keep mixins to the vanilla screen working with it.

@BoogieMonster1O1
Copy link
Contributor

I don’t think completely bypassing the vanilla controls screen is the right approach.

What do you think is the right approach in this case then? The current implementation modifies the screen quite heavily, it would be difficult to keep mixins to the vanilla screen working with it.

I'd suggest de-hardcoding the existing keybinding buttons, and then using pagination to show other mod's keybindings.

@thecatcore
Copy link
Member Author

Update with Pagination system instead.
Screenshot from 2024-07-01 20-01-11
Screenshot from 2024-07-01 20-01-17

@thecatcore thecatcore marked this pull request as ready for review July 1, 2024 19:44
BoogieMonster1O1
BoogieMonster1O1 reviewed Jul 2, 2024
View reviewed changes
Copy link
Contributor

@BoogieMonster1O1 BoogieMonster1O1 left a comment

Choose a reason for hiding this comment

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

Nice. very simple implementation

...main/java/net/legacyfabric/fabric/impl/client/keybinding/FabricControlsScreenComponents.java Outdated Show resolved Hide resolved
@thecatcore thecatcore changed the title Dynamic keybinding screen for 1.6.4 Pagination for 1.6.4 controls screen Jul 2, 2024
@thecatcore thecatcore merged commit 88808f6 into Legacy-Fabric:main Jul 2, 2024
2 checks passed
@thecatcore thecatcore deleted the feature/1.6.4-dynamic-keybinding-screen branch July 2, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BoogieMonster1O1 BoogieMonster1O1 BoogieMonster1O1 requested changes

@BluCobalt BluCobalt Awaiting requested review from BluCobalt

Assignees

@thecatcore thecatcore

Labels
bug Something isn't working MC1.6.4 Related to Minecraft 1.6.4 merge me please
Projects
Legacy Fabric API
Status: Done
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@thecatcore @BoogieMonster1O1 @BluCobalt