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

add keybinds for Wireless (Crafting) Terminal and Portable Storage Cells #6251

Merged
merged 13 commits into from
May 29, 2022
Merged

add keybinds for Wireless (Crafting) Terminal and Portable Storage Cells #6251

merged 13 commits into from
May 29, 2022
+252 −3

Conversation

Mari023
Copy link
Contributor

@Mari023 Mari023 commented May 6, 2022 •
edited
Loading

  • some classes are probably not in the right package
  • some parts of this should be part of the api
  • LocatingService
  • LocatingServices#register
  • maybe LocatingServices#registerPortableCell this can't be in the api, but should still be accessible for addons
  • change the order of so LocatingServices from addons are called before ae2
  • the larger portable storage cells need hotkeys too
  • localize Keybinds

Closes #4968

@Mari023 Mari023 marked this pull request as ready for review May 7, 2022 16:17
@Technici4n
Copy link
Member

Is this ready for review?

@Mari023
Copy link
Contributor Author

Mari023 commented May 20, 2022

yes

Technici4n
Technici4n requested changes May 24, 2022
View reviewed changes
Copy link
Member

@Technici4n Technici4n left a comment

Choose a reason for hiding this comment

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

Quite a few comments, hopefully nothing too bad to fix. Sorry for the delay...

Could you also look into making the API thread-safe?

src/main/java/appeng/hotkeys/LocatingServicesImpl.java Outdated Show resolved Hide resolved
src/main/java/appeng/api/hotkeys/LocatingService.java Outdated Show resolved Hide resolved
src/main/java/appeng/client/Hotkeys.java Outdated Show resolved Hide resolved
src/main/java/appeng/hotkeys/InventoryLocatingService.java Outdated Show resolved Hide resolved
src/main/java/appeng/hotkeys/LocatingServicesImpl.java Outdated Show resolved Hide resolved
@Mari023
rename LocatingService -> HotkeyAction
fd2e46b 
add IDs to API
move wireless crafting terminal hotkey to wireless terminal
@Mari023
register Hotkeys when the Hotkey actions are registered, and not on s…
902efed 
…tatic in Hotkeys
Technici4n
Technici4n requested changes May 28, 2022
View reviewed changes
Copy link
Member

@Technici4n Technici4n left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, very nice and extensible system! Just a few comments and it will be good to go! Sorry for the additional delay in reviewing this... Don't hesitate to ping me on Discord to make me review things faster.

src/main/java/appeng/hotkeys/InventoryHotkeyAction.java Outdated Show resolved Hide resolved
src/main/java/appeng/hotkeys/InventoryHotkeyAction.java Outdated Show resolved Hide resolved
src/main/java/appeng/hotkeys/HotkeyActionsImpl.java Outdated Show resolved Hide resolved
src/main/java/appeng/hotkeys/HotkeyActionsImpl.java Outdated Show resolved Hide resolved
@Technici4n Technici4n added this to the 11.0.1 - 1.18.2 milestone May 28, 2022
@Technici4n
Copy link
Member

For the package, I'd move this stuff to appeng.api.features. Might also want to have the register method in the HotkeyAction interface to avoid exposing many small classes.

For localization, you can add the entries manually to the datagen.

Technici4n
Technici4n approved these changes May 29, 2022
View reviewed changes
Copy link
Member

@Technici4n Technici4n left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good now, thanks a lot!

@Technici4n Technici4n merged commit 7613a18 into AppliedEnergistics:master May 29, 2022
@Mari023 Mari023 deleted the hotkeys branch May 29, 2022 17:12
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Technici4n Technici4n Technici4n approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
11.1.0 - 1.18.2
Development

Successfully merging this pull request may close these issues.

Add keybind to open Wireless Terminal from anywhere in your inventory much easier.
2 participants
@Mari023 @Technici4n