Skip to content

Feature/element styling state aware#62

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

Merged
andreamancuso merged 10 commits into from
Oct 12, 2024
Merged

Feature/element styling state aware #62

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cf879de
Update CMakeLists.txt, element.h, and 7 more files...
andreamancuso Oct 10, 2024
6a160e9
Update element.h, element.cpp, and styled_widget.cpp
andreamancuso Oct 10, 2024
e1c3f8f
Update element.h, element.cpp, and styled_widget.cpp
andreamancuso Oct 11, 2024
945131e
Update element.h, element.cpp, and 3 more files...
andreamancuso Oct 11, 2024
9812abb
Update CMakeLists.txt, element.cpp, and Tabs.tsx
andreamancuso Oct 12, 2024
e4f6c1c
Update element.cpp, layout_node.cpp, and Tabs.tsx
andreamancuso Oct 12, 2024
70210e9
Update element.cpp
andreamancuso Oct 12, 2024
57f5e51
Update README.md and clion-toolchain.png
andreamancuso Oct 12, 2024
47b1c53
Update element.h, element.cpp, and 5 more files...
andreamancuso Oct 12, 2024
a527808
Update element.cpp, reactimgui.cpp, and 4 more files...
andreamancuso Oct 12, 2024
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
4 changes: 3 additions & 1 deletion packages/dear-imgui/README.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -4,7 +4,9 @@ Please note that only Chrome, Edge and [Firefox Nightlies](https://www.mozilla.o

## Building instructions

We provide instructions for manual setup on Windows as well as a devcontainer configuration. We **strongly** recommend to use the Dev Container approach.
We provide instructions for manual setup on Windows as well as a devcontainer configuration. We recommend to use the Dev Container approach or JetBrain's CLion:

![React Dear Imgui Electron demo](/screenshots/dear-imgui/clion-toolchain.png?raw=true)
Comment on lines +7 to +9

@coderabbitaicoderabbitaiBotOct 12, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Approve changes with minor grammatical correction

The addition of JetBrain's CLion as a recommended setup option and the inclusion of a visual aid are valuable improvements to the documentation. However, there's a minor grammatical issue that should be addressed.

Please apply the following change to correct the grammar:

-We recommend to use the Dev Container approach or JetBrain's CLion:+We recommend using the Dev Container approach or JetBrain's CLion:

This change ensures proper usage of the verb "recommend" with the gerund form.

📝 Committable suggestion

‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
We provide instructions for manual setup on Windows as well as a devcontainer configuration. We recommend to use the Dev Container approach or JetBrain's CLion:
![React Dear Imgui Electron demo](/screenshots/dear-imgui/clion-toolchain.png?raw=true)
We provide instructions for manual setup on Windows as well as a devcontainer configuration. We recommend using the Dev Container approach or JetBrain's CLion:
![React Dear Imgui Electron demo](/screenshots/dear-imgui/clion-toolchain.png?raw=true)
🧰 Tools
🪛 LanguageTool

[uncategorized] ~7-~7: Possible missing comma found.
Context: ...rovide instructions for manual setup on Windows as well as a devcontainer configuration...

(AI_HYDRA_LEO_MISSING_COMMA)

[grammar] ~7-~7: The verb ‘recommend’ is used with the gerund form.
Context: ...ell as a devcontainer configuration. We recommend to use the Dev Container approach or JetBrain'...

(ADMIT_ENJOY_VB)


### Requirements for devcontainer-based setup (massive kudos to [@genautz](https://github.com/genautz))

Expand Down
32 changes: 25 additions & 7 deletions packages/dear-imgui/cpp/include/element/element.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -23,9 +23,8 @@ struct BorderStyle {
float thickness = 0;
};

BorderStyle extractBorderStyle(const json& borderStyleDef);

struct BaseDrawStyle {
struct ElementStyleParts {
json styleDef;
std::optional<ImVec4> backgroundColor;
std::optional<BorderStyle> borderTop;
std::optional<BorderStyle> borderRight;
Expand All@@ -36,6 +35,16 @@ struct BaseDrawStyle {
ImDrawFlags drawFlags = ImDrawFlags_RoundCornersNone;
};

struct ElementStyle {
std::optional<ElementStyleParts> maybeBase;
std::optional<ElementStyleParts> maybeDisabled;
std::optional<ElementStyleParts> maybeHover;
std::optional<ElementStyleParts> maybeActive;
};

BorderStyle extractBorderStyle(const json& borderStyleDef);
ElementStyleParts extractStyleParts(const json& styleDef);

class Element {
public:
int m_id;
Expand All@@ -44,11 +53,14 @@ class Element {
bool m_handlesChildrenWithinRenderMethod;
bool m_isRoot;
bool m_cull;
bool m_hovered;
bool m_isHovered = false;
bool m_isActive = false;
bool m_isFocused = false;
bool m_trackMouseClickEvents = false;
Comment on lines +56 to +59

@coderabbitaicoderabbitaiBotOct 12, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Ensure Consistent Naming Convention

The member variables m_isHovered, m_isActive, and m_isFocused use the m_is prefix for boolean flags. For consistency, consider renaming m_trackMouseClickEvents to m_isTrackingMouseClickEvents.

Apply the following diff to maintain consistent naming:

- bool m_trackMouseClickEvents = false;+ bool m_isTrackingMouseClickEvents = false;
📝 Committable suggestion

‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
boolm_isHovered= false;
boolm_isActive= false;
boolm_isFocused= false;
boolm_trackMouseClickEvents= false;
boolm_isHovered= false;
boolm_isActive= false;
boolm_isFocused= false;
boolm_isTrackingMouseClickEvents= false;

std::unique_ptr<LayoutNode> m_layoutNode;
std::optional<BaseDrawStyle> m_baseDrawStyle;
std::optional<ElementStyle> m_elementStyle;

Element(ReactImgui* view, int id, bool isRoot, bool cull);
Element(ReactImgui* view, int id, bool isRoot, bool cull, bool trackMouseClickEvents);

@coderabbitaicoderabbitaiBotOct 12, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Update Constructor Parameter Naming

If you rename the member variable for consistency, ensure the constructor parameter trackMouseClickEvents is also updated to match the new naming convention. This will keep the codebase consistent and reduce potential confusion.

Apply the following diff:

- Element(ReactImgui* view, int id, bool isRoot, bool cull, bool trackMouseClickEvents);+ Element(ReactImgui* view, int id, bool isRoot, bool cull, bool isTrackingMouseClickEvents);
📝 Committable suggestion

‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Element(ReactImgui*view, intid, boolisRoot, boolcull, booltrackMouseClickEvents);
Element(ReactImgui*view, intid, boolisRoot, boolcull, boolisTrackingMouseClickEvents);


static std::unique_ptr<Element> makeElement(const json& val, ReactImgui* view);

Expand All@@ -72,14 +84,20 @@ class Element {

virtual ElementState GetState() const;

bool HasStyle(ElementState state);

Comment on lines +87 to +88

@coderabbitaicoderabbitaiBotOct 12, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider Marking HasStyle Method as const

The method bool HasStyle(ElementState state); appears to evaluate whether a style exists for a given state without modifying the object's state. Declaring this method as const would clarify its intent and allow it to be called on constant instances of the class.

Apply the following diff to mark the method as const:

- bool HasStyle(ElementState state);+ bool HasStyle(ElementState state) const;
📝 Committable suggestion

‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
boolHasStyle(ElementStatestate);
boolHasStyle(ElementStatestate)const;

[[nodiscard]] const std::optional<ElementStyleParts>& GetElementStyleParts(ElementState state) const;

@coderabbitaicoderabbitaiBotOct 12, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid Returning Const Reference to std::optional

Returning a const std::optional<ElementStyleParts>& in GetElementStyleParts might lead to dangling references or unexpected behavior if the optional is disengaged. It's safer to return by value to ensure the caller receives a valid copy.

Apply the following diff to return by value:

- [[nodiscard]] const std::optional<ElementStyleParts>& GetElementStyleParts(ElementState state) const;+ [[nodiscard]] std::optional<ElementStyleParts> GetElementStyleParts(ElementState state) const;
📝 Committable suggestion

‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
[[nodiscard]] conststd::optional<ElementStyleParts>&GetElementStyleParts(ElementStatestate) const;
[[nodiscard]] std::optional<ElementStyleParts>GetElementStyleParts(ElementStatestate) const;


void DrawBaseEffects() const;

void ResetStyle();

void SetStyle(const json& styleDef);
void ApplyStyle();

ImRect GetScrollingAwareViewport();

virtual std::optional<ElementStyle> ExtractStyle(const json& elementDef);

Comment on lines +99 to +100

@coderabbitaicoderabbitaiBotOct 12, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider Marking ExtractStyle Method as const

If the ExtractStyle method does not alter the object's state and solely processes the provided elementDef, it could be declared as const. This change would enhance the method's clarity and usage flexibility.

Apply the following diff to mark the method as const:

- virtual std::optional<ElementStyle> ExtractStyle(const json& elementDef);+ virtual std::optional<ElementStyle> ExtractStyle(const json& elementDef) const;
📝 Committable suggestion

‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
virtualstd::optional<ElementStyle>ExtractStyle(constjson&elementDef);
virtualstd::optional<ElementStyle>ExtractStyle(constjson&elementDef)const;

virtual void Patch(const json& elementPatchDef, ReactImgui* view);

virtual bool HasInternalOps();
Expand Down
2 changes: 1 addition & 1 deletion packages/dear-imgui/cpp/include/reactimgui.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -63,7 +63,7 @@ class ReactImgui : public ImPlotView {

std::unordered_map<int, std::unique_ptr<char[]>> m_floatFormatChars;

ImGuiStyle m_widgetStyle;
ImGuiStyle m_appStyle;

OnInitCallback m_onInit;
OnTextChangedCallback m_onInputTextChange;
Expand Down
2 changes: 1 addition & 1 deletion packages/dear-imgui/cpp/include/widget/button.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -25,7 +25,7 @@ class Button final : public StyledWidget {
if (context) {
const auto widget = static_cast<Button*>(context);

size.width = (widget->m_view->m_widgetStyle.FramePadding.x * 2.0f) + widget->m_view->CalcTextSize(widget, widget->m_label.c_str()).x;
size.width = (widget->m_view->m_appStyle.FramePadding.x * 2.0f) + widget->m_view->CalcTextSize(widget, widget->m_label.c_str()).x;
size.height = widget->m_view->GetFrameHeight(widget);

if (widget->HasCustomStyles() && widget->HasCustomStyleVar(ImGuiStyleVar_FramePadding)) {
Expand Down
2 changes: 1 addition & 1 deletion packages/dear-imgui/cpp/include/widget/styled_widget.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -27,7 +27,7 @@ struct WidgetStyle {
StyleVarValue GetCustomStyleVar(std::optional<ElementState> widgetState, ImGuiStyleVar key);
};

WidgetStyleParts extractStyle(const json& styleDef, ReactImgui* view);
WidgetStyleParts extractStyleParts(const json& styleDef, ReactImgui* view);

class StyledWidget : public Widget {
public:
Expand Down
Loading