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

cloth-config dependency support #204

Merged
merged 2 commits into from
Jul 9, 2023
Merged

cloth-config dependency support #204

merged 2 commits into from
Jul 9, 2023

Conversation

MattSturgeon
Copy link
Contributor

@MattSturgeon MattSturgeon commented May 7, 2023 •
edited
Loading

Following on from my initial draft PR #195, here's a subset of the changes focused on adding dependency support to cloth config.

I identified the main cause of complexity in the draft PR was auto-generating human readable descriptions of potentially complex dependencies. In hindsight, the result isn't worth the added complexity, instead it is recommended that users write their own dependency descriptions in tooltips or labels.

This PR does add a simple "Dependencies not met" line to the tooltip when isEnabled() is false. This could be removed/reformatted/reworded depending on your preference.

Currently, the Dependency may be checked multiple times per frame. If this proves performance intenseive, it may make sense to instead check once-per-tick on the main thread and store the result in an AtomicBoolean to reference from the render thread. implemented

I plan to add an implementation for autoconfig in a follow up PR, based on what I have currently in the initial draft, but ideally this should be merged first - or at least the design agreed upon.

Fixes #26

@shedaniel shedaniel self-assigned this May 7, 2023
@MattSturgeon
Copy link
Contributor Author

@shedaniel do you think you'll get time to review this soon? I'm open to feedback and discussing the overall design if you have a different approach in mind.

@shedaniel
Copy link
Owner

Thank you for this PR and sorry that I have completely forgotten about this, please continue to notify me if I forget in the future.
I will take a quick look later.

shedaniel
shedaniel requested changes Jun 26, 2023
View reviewed changes
Copy link
Owner

@shedaniel shedaniel left a comment

Choose a reason for hiding this comment

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

technically these all are impl classes, but Cloth Config is so widely used now that we can't afford to break anyone's code, so we have to keep binary compat even in internal code

common/src/main/java/me/shedaniel/clothconfig2/gui/entries/TooltipListEntry.java Outdated Show resolved Hide resolved
common/src/main/java/me/shedaniel/clothconfig2/gui/entries/TooltipListEntry.java Outdated Show resolved Hide resolved
common/src/main/java/me/shedaniel/clothconfig2/gui/widget/DynamicEntryListWidget.java Outdated Show resolved Hide resolved
common/src/main/java/me/shedaniel/clothconfig2/gui/widget/DynamicEntryListWidget.java Outdated Show resolved Hide resolved
common/src/main/java/me/shedaniel/clothconfig2/gui/widget/DynamicEntryListWidget.java Outdated Show resolved Hide resolved
common/src/main/resources/assets/cloth-config2/lang/en_us.json Show resolved Hide resolved
@MattSturgeon

This comment was marked as resolved.

Sign in to view
shedaniel
shedaniel requested changes Jul 3, 2023
View reviewed changes
Copy link
Owner

@shedaniel shedaniel left a comment

Choose a reason for hiding this comment

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

Final minor nitpicks and also would need to resolve the previous issues, then I think this is good to go.
Thank you for your work so far!!

common/src/main/java/me/shedaniel/clothconfig2/gui/widget/DynamicEntryListWidget.java Show resolved Hide resolved
common/src/main/java/me/shedaniel/clothconfig2/gui/widget/DynamicEntryListWidget.java Outdated Show resolved Hide resolved
@MattSturgeon
Copy link
Contributor Author

Fixed your latest points. Also found and fixed a bug where a hidden boolean toggle entry was still handling click events: aa22d5e

@shedaniel
Copy link
Owner

Also I am okay for explicit @nullable, just no for explicit @NotNulls, it is fine to keep the @nullable

@MattSturgeon
Enable/Display config entries using dependencies
8eb1416 
Allows users to define dependency `Requirement`s as lambda functions, or generate them via factory methods.

A standard requirement controls whether a Config Entry is "enabled" or "disabled". When "disabled" the Config Entry is greyed-out and cannot be interacted with.

A "display" requirement controls whether a Config Entry appears _at all_ in the GUI. It's still technically present, but won't be rendered or take up any space in the list GUI.

A hidden Config Entry is also disabled automatically.
@MattSturgeon MattSturgeon changed the title Initial basic dependency implementation cloth-config dependency support Jul 3, 2023
shedaniel
shedaniel approved these changes Jul 3, 2023
View reviewed changes
Copy link
Owner

@shedaniel shedaniel 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.

LGTM! Thank you for everything you’ve done! I am currently on a trip, I will merge this the next weekend

@MattSturgeon
Copy link
Contributor Author

Great, thanks for all your help and guidance. 🎉

Would you like the PR to target a different branch or stick with v8?

Enjoy your trip!

@shedaniel shedaniel merged commit 3777a5b into shedaniel:v8 Jul 9, 2023
shedaniel added a commit that referenced this pull request Jul 9, 2023
@MattSturgeon @shedaniel
cloth-config dependency support (#204)
c54aa59 
* Enable/Display config entries using dependencies

Allows users to define dependency `Requirement`s as lambda functions, or generate them via factory methods.

A standard requirement controls whether a Config Entry is "enabled" or "disabled". When "disabled" the Config Entry is greyed-out and cannot be interacted with.

A "display" requirement controls whether a Config Entry appears _at all_ in the GUI. It's still technically present, but won't be rendered or take up any space in the list GUI.

A hidden Config Entry is also disabled automatically.

* Apply license headers

---------

Co-authored-by: shedaniel <daniel@shedaniel.me>
shedaniel added a commit that referenced this pull request Jul 9, 2023
@MattSturgeon @shedaniel
cloth-config dependency support (#204)
c8eb040 
* Enable/Display config entries using dependencies

Allows users to define dependency `Requirement`s as lambda functions, or generate them via factory methods.

A standard requirement controls whether a Config Entry is "enabled" or "disabled". When "disabled" the Config Entry is greyed-out and cannot be interacted with.

A "display" requirement controls whether a Config Entry appears _at all_ in the GUI. It's still technically present, but won't be rendered or take up any space in the list GUI.

A hidden Config Entry is also disabled automatically.

* Apply license headers

---------

Co-authored-by: shedaniel <daniel@shedaniel.me>
@MattSturgeon MattSturgeon deleted the dependencies-v2 branch July 9, 2023 14:26
@MattSturgeon MattSturgeon mentioned this pull request Jul 21, 2023
Open
MattSturgeon added a commit to MattSturgeon/cloth-config that referenced this pull request Jul 21, 2023
@MattSturgeon
Autoconfig dependency support
276d538 
Follow up from shedaniel#204, adds annotation based dependency support to autoconfig.
@MattSturgeon MattSturgeon mentioned this pull request Dec 3, 2023
Merged
@MattSturgeon MattSturgeon mentioned this pull request Dec 19, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shedaniel shedaniel shedaniel approved these changes

Assignees

@shedaniel shedaniel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Suggestion: Navigation
2 participants
@MattSturgeon @shedaniel