Skip to content

Ametric percent#714

Open
cubekii wants to merge 5 commits intoaui-framework:developfrom
cubekii:ametric-percent
Open

Ametric percent#714
cubekii wants to merge 5 commits intoaui-framework:developfrom
cubekii:ametric-percent

Conversation

@cubekii
Copy link
Copy Markdown

@cubekii cubekii commented Mar 19, 2026

closes: #439
add operator""_pr for AMetric
you can check this with aui.example.bordercheck

@gemini-code-assist
Copy link
Copy Markdown

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the flexibility of UI layout by integrating percentage-based metrics into the AMetric system. This allows UI elements to scale more dynamically and responsively, particularly for properties like BorderRadius, which can now adapt relative to the view's dimensions. The changes provide a more intuitive way to define scalable UI components.

Highlights

  • New Percentage Unit for AMetric: Introduced a new unit type, T_PR (percent), to the AMetric system, allowing UI properties to be defined as percentages.
  • User-Defined Literal operator""_pr: Added a C++ user-defined literal operator""_pr for AMetric, enabling concise syntax like 10_pr for percentage values.
  • Percentage-Based Border Radius: The BorderRadius property now supports percentage values, dynamically calculating the radius based on the minimum of the view's width and height.
  • New Example Application: A new example, aui.example.bordercheck, was added to demonstrate the usage and behavior of percentage-based border radii.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a percentage-based unit (_pr) for AMetric, which is a great addition for flexible UI layouts. However, I've found a few critical issues that need to be addressed. The formula for calculating border radius from percentages is incorrect, and a newly declared function getValuePx(float) is missing its implementation, which will cause linker errors. There's also a design concern with how getValuePx() handles the new percentage unit, which could lead to subtle bugs. Finally, the new example code for demonstrating this feature has a confusing layout structure that could be simplified.

void ass::prop::Property<ass::BorderRadius>::applyFor(AView* view) {
view->setBorderRadius(mInfo.radius);
if (mInfo.radius.getUnit() == AMetric::T_PR) {
view->setBorderRadius((glm::min(view->getWidth(), view->getHeight()) * 2) * (mInfo.radius.getValuePx() * 0.01));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

The calculation for percentage-based border radius appears to be incorrect. The multiplication by 2 will result in a radius twice as large as expected. For example, for a square view and a 50_pr border-radius, the desired outcome is typically a circle, which requires a radius of half the side length. The current formula calculates a radius equal to the full side length.

To fix this, you should remove the * 2 from the calculation.

Additionally, consider centralizing the percentage-to-pixel conversion logic within the AMetric class by implementing the new getValuePx(float referenceDimension) method and using it here. This would improve code clarity and maintainability.

Suggested change
view->setBorderRadius((glm::min(view->getWidth(), view->getHeight()) * 2) * (mInfo.radius.getValuePx() * 0.01));
view->setBorderRadius(glm::min(view->getWidth(), view->getHeight()) * (mInfo.radius.getValuePx() * 0.01f));

* @param referenceDimension The reference dimension to use for percentage calculations (T_PR unit).
* @return The value in pixels.
*/
[[nodiscard]] float getValuePx(float referenceDimension) const;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

The new function getValuePx(float referenceDimension) is declared here but is not implemented in AMetric.cpp. This will lead to a linker error if this function is used. You should provide an implementation for it.

An example implementation could be:

float AMetric::getValuePx(float referenceDimension) const {
    if (mUnit == T_PR) {
        return referenceDimension * mValue * 0.01f;
    }
    return getValuePx();
}

@@ -141,6 +144,13 @@ class API_AUI_VIEWS AMetric
[[nodiscard]] float getValuePx() const;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Calling getValuePx() on an AMetric with T_PR unit is ambiguous as it requires a reference dimension to be converted to pixels. Currently, it falls through to the default case in AMetric.cpp and returns the raw percentage value. This is misleading and error-prone, especially with operator float() implicitly using this conversion.

It would be safer to make getValuePx() on a T_PR metric an error to enforce the use of the context-aware getValuePx(float referenceDimension) overload. You could add an assertion in getValuePx() for the T_PR case.

Example for AMetric.cpp:

// in AMetric::getValuePx()
switch (mUnit) {
    // ... other cases
    case T_PR:
        AUI_ASSERT_NOT_REACHED("AMetric with T_PR unit cannot be converted to pixels without a reference dimension. Use getValuePx(float) instead.");
        return 0.f;
    default:
        return mValue;
}

Comment on lines +23 to +58
Vertical {
Centered {
_new<ALabel>("10%") AUI_OVERRIDE_STYLE {
BorderRadius { 10_pr },
FixedSize { 200_px , 40_px },
BackgroundSolid { 100_rgb },
},

_new<ALabel>("20%") AUI_OVERRIDE_STYLE {
Margin { 100_px, 0, 0, 0},
BorderRadius { 20_pr },
FixedSize { 200_px , 40_px },
BackgroundSolid { 150_rgb },
},

_new<ALabel>("50%") AUI_OVERRIDE_STYLE {
Margin { 200_px, 0, 0, 0},
BorderRadius { 50_pr },
FixedSize { 200_px , 40_px },
BackgroundSolid { 200_rgb },
},
_new<ALabel>("100%") AUI_OVERRIDE_STYLE {
Margin { 300_px, 0, 0, 0},
BorderRadius { 100_pr },
FixedSize { 200_px , 40_px },
BackgroundSolid { 300_rgb },
},
//Circle!
_new<ALabel>("") AUI_OVERRIDE_STYLE {
Margin { 400_px, 0, 0, 0},
BorderRadius { 50_pr },
FixedSize { 40_px },
BackgroundSolid { 5148_rgb },
}
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The layout structure here is a bit confusing. Centered layout stacks all its children on top of each other at the center. You are then using Margin to offset them vertically. A much simpler and clearer way to achieve a vertical arrangement would be to make the labels direct children of the Vertical layout.

    window->setContents(
        Vertical {
            _new<ALabel>("10%") AUI_OVERRIDE_STYLE {
                BorderRadius { 10_pr },
                FixedSize { 200_px , 40_px },
                BackgroundSolid { 100_rgb },
            },
            // ... other labels as direct children of Vertical
        }
    );

This would make the example easier to understand and maintain.

return raster;
} catch (const AException& e) {
#if AUI_BUILD_TESTS
if (url.full()!="builtin://__aui/icon_512x512.png")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thank you

void ass::prop::Property<ass::BorderRadius>::applyFor(AView* view) {
view->setBorderRadius(mInfo.radius);
if (mInfo.radius.getUnit() == AMetric::T_PR) {
view->setBorderRadius((glm::min(view->getWidth(), view->getHeight())) * (mInfo.radius.getValuePx() * 0.01f));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i'm not sure this would work perfectly fine but your solution is definitely small and simple. Give me some time to test please.

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.

2 participants