Skip to content

Refactor settings #11413

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

Closed
armele wants to merge 2 commits into from
Closed

Refactor settings #11413

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3eefae5
Settings window support of modified UI layout
armele Dec 1, 2025
cf387f0
Merge branch 'ldtteam:version/main' into refactor_settings
armele Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/main/java/com/minecolonies/core/client/gui/modules/SettingsModuleWindow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public class SettingsModuleWindow extends AbstractModuleWindow<SettingsModuleVie
*/
public SettingsModuleWindow(final SettingsModuleView moduleView)
{
super(moduleView, new ResourceLocation(Constants.MOD_ID, "gui/layouthuts/layoutsettings.xml"));
super(moduleView, getWindowResourceLocation());
settingsList = window.findPaneOfTypeByID(LIST_SETTINGS, ScrollingList.class);
}

Expand Down Expand Up @@ -112,4 +112,13 @@ public void updateElement(final int index, @NotNull final Pane rowPane)
}
});
}

/**
* Gets the resource location for the window's layout.
* @return the resource location
*/
protected static ResourceLocation getWindowResourceLocation()
Copy link
Contributor

@someaddons someaddons Dec 1, 2025

Choose a reason for hiding this comment

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

static?

Copy link
Contributor Author

@armele armele Dec 2, 2025

Choose a reason for hiding this comment

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

Doesn't change per instance - but more importantly can't call a non static function from the super() call in the constructor.

Copy link
Contributor

@Thodor12 Thodor12 Dec 2, 2025

Choose a reason for hiding this comment

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

Protected static is a bad idea, give an alternative constructor in that case. However realistically, is this needed? The setting components can already provide their own view, this specific view is just a simple list that aggregates all the settings.

Copy link
Contributor

@someaddons someaddons Dec 2, 2025

Choose a reason for hiding this comment

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

when this call is a static util this PR simply does nothing. Compiler will inline the call and its literally the same as before? :D

Copy link
Contributor Author

@armele armele Dec 2, 2025

Choose a reason for hiding this comment

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

Ah, silly me... I thought I could still override it in a child. Ok, I will close this PR and rethink the approach.

Copy link
Contributor

@Thodor12 Thodor12 Dec 2, 2025

Choose a reason for hiding this comment

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

    public SettingsModuleWindow(final SettingsModuleView moduleView)
    {
        this(moduleView, new ResourceLocation(Constants.MOD_ID, "gui/layouthuts/layoutsettings.xml"));
    }

    protected SettingsModuleWindow(final SettingsModuleView moduleView, final ResourceLocation windowId)
    {
        super(moduleView, windowId);
        settingsList = window.findPaneOfTypeByID(LIST_SETTINGS, ScrollingList.class);
    }

Copy link
Contributor

@Thodor12 Thodor12 Dec 2, 2025

Choose a reason for hiding this comment

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

That being said, I do not think this is a good idea. These windows enforce the existence of certain GUI objects. I do not want to leave that up to chance to an addon to fill correctly. That's why you should be making your own modules for this stuff.

Copy link
Contributor Author

@armele armele Dec 2, 2025

Choose a reason for hiding this comment

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

That's fair. My approach so far has been to introduce new modules for things significantly different (trade connections, recycler status, pet assignment, etc) and to adapt modules where things are very similar (item lists). I'll bias more towards new modules unless the functionality is identical.

Copy link
Contributor

@Thodor12 Thodor12 Dec 2, 2025

Choose a reason for hiding this comment

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

I'd recommend you actually create a custom setting class that actually incorporates your button you wanted. That way you do not have to stray from the settings module default implementation at all. You can design individual setting options exactly how you feel like.

Copy link
Contributor Author

@armele armele Dec 2, 2025

Choose a reason for hiding this comment

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

Excellent, thanks for the tip... I'll explore that.

{
return new ResourceLocation(Constants.MOD_ID, "gui/layouthuts/layoutsettings.xml");
}
}
Loading