Conversation
Summary of ChangesHello, 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 Highlights
🧠 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 AssistThe 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
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 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
|
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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.
| 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)); |
aui.views/src/AUI/Util/AMetric.h
Outdated
| * @param referenceDimension The reference dimension to use for percentage calculations (T_PR unit). | ||
| * @return The value in pixels. | ||
| */ | ||
| [[nodiscard]] float getValuePx(float referenceDimension) const; |
There was a problem hiding this comment.
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; | |||
There was a problem hiding this comment.
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;
}| 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 }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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") |
| 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)); |
There was a problem hiding this comment.
i'm not sure this would work perfectly fine but your solution is definitely small and simple. Give me some time to test please.
closes: #439
add operator""_pr for AMetric
you can check this with aui.example.bordercheck