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

Improvement: garden keybinds #3132

Open
wants to merge 33 commits into
base: beta
Choose a base branch
from

Conversation

Cedric-Ab
Copy link
Contributor

@Cedric-Ab Cedric-Ab commented Dec 30, 2024

What

You can now set 5 keybind layouts, each with different keybinds.
You can then for each crop select what layout you'd wish to use.
Also moved it into it's own category

Images

image

Changelog Improvements

  • Improved garden keybinds. - Cédric Ab
    • Now set up to 5 different keybind layouts to choose from with each crop.

do GUI and config part, now only kotlin side has to be done
add disableAll and defaultAll
should work but of course doesn't
@github-actions github-actions bot added the Detekt Has detekt problem label Dec 30, 2024
Copy link

One or more Detekt Failures were detected:

Copy link

One or more Detekt Failures were detected:

fix small mistakes
+ redo getting allKeybindings in onConfigLoad
@Cedric-Ab Cedric-Ab marked this pull request as ready for review December 31, 2024 12:58
@Cedric-Ab
Copy link
Contributor Author

@Thunderblade73
would it be better to use stuff like getting all the variables in the config to then dynamically do the stuff. so you could just adjust the number of layouts in config and then the kotlin would automatically just work with the amount of layouts in the config?
My main concern with this is the unneeded complexity this would generate

Copy link

One or more Detekt Failures were detected:

fix "detekt failure"
Copy link

One or more Detekt Failures were detected:

  • GardenCustomKeybinds.kt#L93: Used in this way a spread operator causes a full copy of the array to be created before calling a method. This may result in a performance penalty.

@Thunderblade73
Copy link
Contributor

@Thunderblade73 would it be better to use stuff like getting all the variables in the config to then dynamically do the stuff. so you could just adjust the number of layouts in config and then the kotlin would automatically just work with the amount of layouts in the config? My main concern with this is the unneeded complexity this would generate

Yeah that would be better imo. If done correctly it should reduce complexity and not add more (as far as I can tell).
Also if you want to give the layout enum more functionallity I would suggest to move it to kotlin, since java code gets more cluttered and the config supports kotlin enums.

Btw. enums have a way to get all entries .values (for java) and .entries for kotlin (before the dot is the Enum Type)

@hannibal002 hannibal002 added this to the Version 0.29 milestone Dec 31, 2024
add workaround for this, as GardenAPI should be calling GardenToolChangeEvent on island change
Copy link

One or more Detekt Failures were detected:

  • GardenCustomKeybinds.kt#L107: Used in this way a spread operator causes a full copy of the array to be created before calling a method. This may result in a performance penalty.

@Cedric-Ab Cedric-Ab marked this pull request as draft December 31, 2024 14:30
@Cedric-Ab

This comment was marked as resolved.

@Cedric-Ab
Copy link
Contributor Author

@Thunderblade73 would it be better to use stuff like getting all the variables in the config to then dynamically do the stuff. so you could just adjust the number of layouts in config and then the kotlin would automatically just work with the amount of layouts in the config? My main concern with this is the unneeded complexity this would generate

Yeah that would be better imo. If done correctly it should reduce complexity and not add more (as far as I can tell). Also if you want to give the layout enum more functionallity I would suggest to move it to kotlin, since java code gets more cluttered and the config supports kotlin enums.

Btw. enums have a way to get all entries .values (for java) and .entries for kotlin (before the dot is the Enum Type)

do you think I could replace the map: layouts with the enum?
and would it need to be in kotlin or could it stay in java?

@Thunderblade73
Copy link
Contributor

do you think I could replace the map: layouts with the enum? and would it need to be in kotlin or could it stay in java?

Yes, you can replace the map.
The can stay in java yes, but makeing them in kotlin is easier.

remove unused enum Layouts
add getAllKeybindings()
+ formatting
also adapt to the enum
@Cedric-Ab
Copy link
Contributor Author

TODO: move GardenCustomKeybinds.kt into keybinds

@Cedric-Ab
Copy link
Contributor Author

Cedric-Ab commented Jan 6, 2025

@Thunderblade73
Is cropLayoutSelection initialized on startup?
is configloadevent called on startup?

@Thunderblade73
Copy link
Contributor

@Thunderblade73 Is cropLayoutSelection initialized on startup? is configloadevent called on startup?

It is loaded via Gson at ConfigLoadEvent (or what ever it is called) or if no value is present in the config json than it calls the constructor.

@Cedric-Ab
Copy link
Contributor Author

@Thunderblade73 Is cropLayoutSelection initialized on startup? is configloadevent called on startup?

It is loaded via Gson at ConfigLoadEvent (or what ever it is called) or if no value is present in the config json than it calls the constructor.

The cropLayoutSelection is a map inside of the kotlin, you got that like this, right?

@Thunderblade73
Copy link
Contributor

The cropLayoutSelection is a map inside of the kotlin, you got that like this, right?

Nope, I saw the value in KeyBindConfig and assume you where referencing it,

@Cedric-Ab
Copy link
Contributor Author

The cropLayoutSelection is a map inside of the kotlin, you got that like this, right?

Nope, I saw the value in KeyBindConfig and assume you where referencing it,

So is ConfigLoadEvent called upon launch?

@Thunderblade73
Copy link
Contributor

is configloadevent called on startup?

yes

Is cropLayoutSelection initialized on startup?

Yes, but you override it every update().
Btw. it does not need to exist since you can simply add a extension function to CropType that gets the value from an when since it does not compute anything since the getLayoutByDisplayName(something.toString()) is redundant because something is already a Layout

@Cedric-Ab
Copy link
Contributor Author

is configloadevent called on startup?

yes

Is cropLayoutSelection initialized on startup?

Yes, but you override it every update(). Btw. it does not need to exist since you can simply add a extension function to CropType that gets the value from an when since it does not compute anything since the getLayoutByDisplayName(something.toString()) is redundant because something is already a Layout

Yes, I see
This is a thing that needs to be done

TODO

KeyBindLayouts.getLayoutByDisplayName(KeyBindLayouts.to_String())
is redundant lol
add extension function to CropType and remove unneeded stuff
@github-actions github-actions bot added the Detekt Has detekt problem label Jan 6, 2025
Copy link

github-actions bot commented Jan 6, 2025

One or more Detekt Failures were detected:

@github-actions github-actions bot removed the Detekt Has detekt problem label Jan 6, 2025
@github-actions github-actions bot added the Merge Conflicts There are open merge conflicts with the beta branch. label Jan 9, 2025
Copy link

github-actions bot commented Jan 9, 2025

This pull request has conflicts with the base branch "beta". Please resolve those so we can test out your changes.

@github-actions github-actions bot removed the Merge Conflicts There are open merge conflicts with the beta branch. label Jan 9, 2025
Copy link

github-actions bot commented Jan 9, 2025

Conflicts have been resolved! 🎉

@github-actions github-actions bot added the Detekt Has detekt problem label Jan 9, 2025
Copy link

github-actions bot commented Jan 9, 2025

One or more Detekt Failures were detected:

@Cedric-Ab
Copy link
Contributor Author

TODO:

  • Move GardenCustomKeybinds.kt into keybinds

@github-actions github-actions bot removed the Detekt Has detekt problem label Jan 9, 2025
Copy link

This pull request has conflicts with the base branch "beta". Please resolve those so we can test out your changes.

@github-actions github-actions bot added the Merge Conflicts There are open merge conflicts with the beta branch. label Jan 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge Conflicts There are open merge conflicts with the beta branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants