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

Feature: Flowstate helper #2561

Merged
merged 31 commits into from
Oct 13, 2024
Merged

Feature: Flowstate helper #2561

merged 31 commits into from
Oct 13, 2024

Conversation

martimavocado
Copy link
Contributor

@martimavocado martimavocado commented Sep 21, 2024 •
edited by hannibal002
Loading

What

adds a status gui with the current state of the flowstate enchantment bonus

Images

image

Changelog New Features

  • Added Flowstate Helper. - martimavocado
    • Displays stats for the Flowstate enchantment on mining tools.

Thunderblade73
Thunderblade73 suggested changes Sep 21, 2024
View reviewed changes
src/main/java/at/hannibal2/skyhanni/utils/ItemUtils.kt Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
@hannibal002 hannibal002 added this to the Version 0.28 milestone Sep 21, 2024
@github-actions github-actions bot added the Merge Conflicts There are open merge conflicts with the beta branch. label Sep 21, 2024
@github-actions GitHub Actions
Copy link

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

@martimavocado
Merge remote-tracking branch 'hannibal002/beta' into feature/flowstat…
9796dca 
…e-bro

# Conflicts:
#	src/main/java/at/hannibal2/skyhanni/config/features/mining/MiningConfig.java
@github-actions github-actions bot removed the Merge Conflicts There are open merge conflicts with the beta branch. label Sep 21, 2024
@github-actions GitHub Actions
Copy link

Conflicts have been resolved! 🎉

CalMWolfs
CalMWolfs requested changes Sep 22, 2024
View reviewed changes
Copy link
Collaborator

@CalMWolfs CalMWolfs left a comment

Choose a reason for hiding this comment

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

just a few things i saw (didnt test)
also could you call the hasFlowstate function on item change and then cache that value as flowstateCache instead of also having the hasChangedItem variable

src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
hannibal002
hannibal002 requested changes Sep 23, 2024
View reviewed changes
Copy link
Owner

@hannibal002 hannibal002 left a comment

Choose a reason for hiding this comment

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

Not tested in game yet.
Just a bunch of code cleanup suggestions. If you need help with anything please ask

src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/config/features/mining/MiningConfig.java Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/config/features/mining/MiningConfig.java Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
CalMWolfs
CalMWolfs requested changes Sep 23, 2024
View reviewed changes
Copy link
Collaborator

@CalMWolfs CalMWolfs left a comment

Choose a reason for hiding this comment

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

code looks fine apart from this enum name

src/main/java/at/hannibal2/skyhanni/features/mining/FlowstateHelper.kt Outdated Show resolved Hide resolved
CalMWolfs
CalMWolfs approved these changes Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@CalMWolfs CalMWolfs left a comment

Choose a reason for hiding this comment

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

code lgtm

@github-actions github-actions bot added the Merge Conflicts There are open merge conflicts with the beta branch. label Oct 11, 2024
@github-actions GitHub Actions
Copy link

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

@hannibal002 hannibal002 added the Soon This Pull Request will be merged within the next couple of betas label Oct 11, 2024
@martimavocado
Merge remote-tracking branch 'hannibal002/beta' into feature/flowstat…
17b4f5d 
…e-bro

# Conflicts:
#	src/main/java/at/hannibal2/skyhanni/config/features/mining/MiningConfig.java
@martimavocado
hi empa
8b65b11
@github-actions github-actions bot removed the Merge Conflicts There are open merge conflicts with the beta branch. label Oct 11, 2024
@github-actions GitHub Actions
Copy link

Conflicts have been resolved! 🎉

hannibal002
hannibal002 requested changes Oct 13, 2024
View reviewed changes
Copy link
Owner

@hannibal002 hannibal002 left a comment

Choose a reason for hiding this comment

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

Works as expected.

Couple sugestions:

Show max streak while not yet met

image

Show flowstate display even when not holding a mining tool in hand, while the time is still remaining. maybe make it an option (default enabled)

the "auto hide" option doesnt seem to work, also i dont think this is necessary as an seting. simply hide after the 10 seconds are over.

change the color of the time to blue, for consistency over the whole mod

hannibal002
hannibal002 requested changes Oct 13, 2024
View reviewed changes
Copy link
Owner

@hannibal002 hannibal002 left a comment

Choose a reason for hiding this comment

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

The display disappears after reaching 200 streaks, even when still actively mining.

@martimavocado
Copy link
Contributor Author

The display disappears after reaching 200 streaks, even when still actively mining.

this is intended, in my opinion there isn't much of a point to keep showing it when it has no more useful info

@ItsEmpa
Copy link
Contributor

ItsEmpa commented Oct 13, 2024

The display disappears after reaching 200 streaks, even when still actively mining.

this is intended, in my opinion there isn't much of a point to keep showing it when it has no more useful info

but i wanna see funny number go up

@hannibal002 hannibal002 self-requested a review October 13, 2024 19:21
@hannibal002 hannibal002 merged commit 78c293e into hannibal002:beta Oct 13, 2024
4 checks passed
@github-actions github-actions bot removed the Soon This Pull Request will be merged within the next couple of betas label Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CalMWolfs CalMWolfs CalMWolfs approved these changes

@Thunderblade73 Thunderblade73 Awaiting requested review from Thunderblade73

@hannibal002 hannibal002 Awaiting requested review from hannibal002

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Version 0.28
Development

Successfully merging this pull request may close these issues.
5 participants
@martimavocado @ItsEmpa @hannibal002 @Thunderblade73 @CalMWolfs