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

Feature: Block Opening /cf Without Hot Chocolate Mixin #3024

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

Conversation

DavidArthurCole
Copy link
Contributor

@DavidArthurCole DavidArthurCole commented Dec 6, 2024

What

https://discord.com/channels/997079228510117908/1314046959589261372
Adds Hot Chocolate Mixin & God Potion detection to the list of reasons /cf/clicking on CF items can be blocked

Changelog New Features

  • Added ability to block opening Chocolate Factory without Hot Chocolate Mixin active. - Daveed

@github-actions github-actions bot added the Detekt Has detekt problem label Dec 6, 2024
Copy link

github-actions bot commented Dec 6, 2024

One or more Detekt Failures were detected:

@github-actions github-actions bot removed the Detekt Has detekt problem label Dec 6, 2024
@@ -303,6 +306,9 @@ public static class BitsStorage {
public SimpleTimeMark boosterCookieExpiryTime = null;
}

@Expose
public SimpleTimeMark godPotExpiryTime = SimpleTimeMarkFarPast();
Copy link
Owner

Choose a reason for hiding this comment

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

this doesnt look right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate more please

Copy link
Owner

Choose a reason for hiding this comment

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

Have not looked at this in IntelliJ. But this doesn't look like a valid java line. What is the default value of this variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Owner

Choose a reason for hiding this comment

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

Method naming does not follow Java convention. Methods typically start with a lowercase letter, e.g. simpleTimeMarkFarPast().

The method name is not very descriptive. Consider something like getFarPastSimpleTimeMark() to clarify its intent.

If GenericWrapper and getIt() are needed, consider renaming getIt() to something more meaningful.

Copy link
Owner

Choose a reason for hiding this comment

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

Also why is this wrapper needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a new thing, this has existed since June.
image

I'm fine with changing it if you really want me to but this is not a new thing. And the wrapper is needed because you can't assign a Kotlin class value to a Java object unless it's got a static definition, which SimpleTimeMark.farPast() is not. It's the same reason that right below that, we have a wrapper for Duration - they're composed types that simplify to long, making them easy to use in the storage, but initial values have to be wrapped.

Copy link
Owner

Choose a reason for hiding this comment

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

can we use new SimpleTimeMark(0) instead then? really not a fan of this wrapping, and the function name breaks java conversations. so sad i have not noticed this before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

No we cannot.

@hannibal002 hannibal002 added this to the Version 0.29 milestone Dec 6, 2024
@github-actions github-actions bot added the Detekt Has detekt problem label Dec 9, 2024
Copy link

github-actions bot commented Dec 9, 2024

One or more Detekt Failures were detected:

  • ChocolateFactoryBlockOpen.kt#L3: Imports must be ordered in lexicographic order without any empty lines in-between with "java", "javax", "kotlin" and aliases in the end. This should then be followed by pre-processed imports.
  • NonGodPotEffectDisplay.kt#L3: Imports must be ordered in lexicographic order without any empty lines in-between with "java", "javax", "kotlin" and aliases in the end. This should then be followed by pre-processed imports.
  • EffectDurationChangeEvent.kt#L3: Imports must be ordered in lexicographic order without any empty lines in-between with "java", "javax", "kotlin" and aliases in the end. This should then be followed by pre-processed imports.

@github-actions github-actions bot added Merge Conflicts There are open merge conflicts with the beta branch. and removed Detekt Has detekt problem labels Dec 9, 2024
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 removed the Merge Conflicts There are open merge conflicts with the beta branch. label Dec 10, 2024
Copy link

Conflicts have been resolved! 🎉

@github-actions github-actions bot added the Merge Conflicts There are open merge conflicts with the beta branch. label Dec 14, 2024
Copy link

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

…etect

# Conflicts:
#	src/main/java/at/hannibal2/skyhanni/features/misc/NonGodPotEffectDisplay.kt
Copy link

Conflicts have been resolved! 🎉

@github-actions github-actions bot removed the Merge Conflicts There are open merge conflicts with the beta branch. label Dec 14, 2024
@github-actions github-actions bot added the Detekt Has detekt problem label Dec 14, 2024
Copy link

One or more Detekt Failures were detected:

  • ChocolateFactoryBlockOpen.kt#L3: Imports must be ordered in lexicographic order without any empty lines in-between with "java", "javax", "kotlin" and aliases in the end. This should then be followed by pre-processed imports.
  • NonGodPotEffectDisplay.kt#L3: Imports must be ordered in lexicographic order without any empty lines in-between with "java", "javax", "kotlin" and aliases in the end. This should then be followed by pre-processed imports.
  • EffectAPI.kt#L3: Imports must be ordered in lexicographic order without any empty lines in-between with "java", "javax", "kotlin" and aliases in the end. This should then be followed by pre-processed imports.

@github-actions github-actions bot removed the Detekt Has detekt problem label Dec 14, 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.

3 participants