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

feat: theme system #5619

Draft
wants to merge 6 commits into
base: nextgen
Choose a base branch
from
Draft

feat: theme system #5619

wants to merge 6 commits into from

Conversation

1zun4
Copy link
Member

@1zun4 1zun4 commented Feb 14, 2025

No description provided.

@1zun4 1zun4 marked this pull request as draft February 14, 2025 12:05
@sqlerrorthing
Copy link
Contributor

please explain in more detail what the "theme system" is, it already exists, doesn't it?

Comment on lines 10 to 14
value = mutableListOf(
*theme.components
.filter { factory -> factory.default }
.map { factory -> factory.new(theme) }.toTypedArray()
)

Check warning

Code scanning / detekt

In most cases using a spread operator causes a full copy of the array to be created before calling a method. This may result in a performance penalty. Warning

In most cases using a spread operator causes a full copy of the array to be created before calling a method. This may result in a performance penalty.
}

val matStack = MatrixStack()
override fun render(context: DrawContext, delta: Float) {

Check warning

Code scanning / detekt

One method should have one responsibility. Long methods tend to handle many things at once. Prefer smaller methods to make them easier to understand. Warning

The function render is too long (75). The maximum length is 60.
*
* @param jsonObject JsonObject
*/
private fun configureConfigurable(configurable: Configurable, jsonObject: JsonObject) {

Check warning

Code scanning / detekt

One method should have one responsibility. Long methods tend to handle many things at once. Prefer smaller methods to make them easier to understand. Warning

The function configureConfigurable is too long (66). The maximum length is 60.
return@onChange enabled
}
val inbuiltThemes = arrayOf(LiquidBounceTheme)
var themes = mutableListOf<Theme>(*inbuiltThemes)

Check warning

Code scanning / detekt

In most cases using a spread operator causes a full copy of the array to be created before calling a method. This may result in a performance penalty. Warning

In most cases using a spread operator causes a full copy of the array to be created before calling a method. This may result in a performance penalty.
Copy link
Contributor

Choose a reason for hiding this comment

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

inbuiltThemes.toMutableList<Theme>()

Copy link
Contributor

Choose a reason for hiding this comment

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

inbuiltThemes.toMutableList<Theme>()

You should use the "Add a suggestion" feature (bound to CTRL + G) instead. It adds some buttons for the PR author to add to the "commit batch", or directly commit it. When they commit it, the commit will also have you (the user making the suggestion) as the co-author.

Suggested change
var themes = mutableListOf<Theme>(*inbuiltThemes)
var themes = inbuiltThemes.toMutableList<Theme>()

FontManager.queueFolder(folder.resolve("assets"))
}

override fun init() { }

Check warning

Code scanning / detekt

Empty block of code detected. As they serve no purpose they should be removed. Warning

This empty block of code can be removed.
Copy link
Contributor

@DataM0del DataM0del left a comment

Choose a reason for hiding this comment

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

there's probably more, but you probably accidentally reverted some of the copyright header changes 💀

/*
* This file is part of LiquidBounce (https://github.com/CCBlueX/LiquidBounce)
*
* Copyright (c) 2024 CCBlueX
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2024 CCBlueX
* Copyright (c) 2024-2025 CCBlueX

lol

/*
* This file is part of LiquidBounce (https://github.com/CCBlueX/LiquidBounce)
*
* Copyright (c) 2024 CCBlueX
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2024 CCBlueX
* Copyright (c) 2024-2025 CCBlueX

@@ -1,7 +1,7 @@
/*
* This file is part of LiquidBounce (https://github.com/CCBlueX/LiquidBounce)
*
* Copyright (c) 2015 - 2025 CCBlueX
* Copyright (c) 2024 CCBlueX
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2024 CCBlueX
* Copyright (c) 2024-2025 CCBlueX

💀

@DataM0del
Copy link
Contributor


This can only mean one thing... No more Chrome?!?

@DataM0del
Copy link
Contributor

please explain in more detail what the "theme system" is, it already exists, doesn't it?

I think this PR is deprecating legacy "web🤮 themes" for normal themes (overriding color palettes), or something else. I don't think LiquidSquid will be very happy about his Catppuccin theme being based off of legacy LiquidBounce technologies known as embedded web browsers. I probably need to review more of it, but that's what I think it is right now.

@DataM0del
Copy link
Contributor

nevermind, it's still web themes...

@DataM0del
Copy link
Contributor

It's just restructuring the theme system, that's it. Pretty much just turning moving like textures, wallpapers, and components from the metadata file into folders. I can probably update catppuccin for LiquidBounce to prepare for this because why not

ThemeManager.themes().filter { it.startsWith(s, true) }
ThemeManager.themes
.map { theme -> theme.name }
.filter { name -> name.startsWith(name, true) }
Copy link
Contributor

Choose a reason for hiding this comment

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

probably name.startsWith(s, true)

get() = blur && !(mc.options.hudHidden && mc.currentScreen == null)

val layouts = choices("Layouts", 0) {
ThemeManager.themes.map { theme -> Layout(theme) }.toTypedArray()
Copy link
Contributor

Choose a reason for hiding this comment

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

mapArray

return@onChange enabled
}
val inbuiltThemes = arrayOf(LiquidBounceTheme)
var themes = mutableListOf<Theme>(*inbuiltThemes)
Copy link
Contributor

Choose a reason for hiding this comment

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

inbuiltThemes.toMutableList<Theme>()

value = mutableListOf(
*theme.components
.filter { factory -> factory.default }
.map { factory -> factory.createComponent(theme) }.toTypedArray()
Copy link
Contributor

Choose a reason for hiding this comment

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

value = theme.components. { ... }.toMutableList()

tweaks: Array<ComponentTweak> = emptyArray()
) : Component(theme, name, enabled, alignment, tweaks) {
abstract fun render(context: DrawContext, delta: Float)
abstract fun size(): Pair<Int, Int>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe IntIntPair.of(x, y)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why you'd want to use IntIntPair instead of Pair<Int, Int>...


identifier
}
} ?: hashMapOf()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe emptyMap()

@1zun4
Copy link
Member Author

1zun4 commented Feb 16, 2025

This is just my playground for refactoring the theme system for better customization - as long as this is a draft, there is no need to review this code :)

@1zun4
Copy link
Member Author

1zun4 commented Feb 16, 2025

This can only mean one thing... No more Chrome?!?

No that does not mean it. It means backwards compatibility with existing LiquidBounce Nextgen themes, as the changes are minor and mostly Data Format changes.

@DataM0del
Copy link
Contributor

This can only mean one thing... No more Chrome?!?

No that does not mean it. It means backwards compatibility with existing LiquidBounce Nextgen themes, as the changes are minor and mostly Data Format changes.

I already knew that, if you scrolled down 2 messages... (also, you forgot a comma after "No", useless spelling gripe)

nevermind, it's still web themes...

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.

4 participants