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

widgets: add onclick feature #653

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

Memoraike
Copy link

Implement feature requested in #366.

Copy link
Collaborator

@PaideiaDilemma PaideiaDilemma left a comment

Choose a reason for hiding this comment

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

Generally looks good.
It would be even better if we had something like CLICKABLE analogous to SHADOWABLE, to easily add that to all widgets.
But we could also add that in a next step.

@vaxerski
Copy link
Member

future things to consider:

  • hover
  • pointer

 - add CLICKABLE macro for onclick configuration
 - replace direct onclick assignment with CLICKABLE macro
 - ensure pointer is available before setting cursor shape
 - initialize cursor shape device if not already done
 - implement onHover method to manage widget hover states
 - update cursor shape based on hover status
 - ensure all outputs are redrawn after state changes
 - add setHover and isHovered methods to manage hover state
 - implement containsPoint method for hit testing
 - override getBoundingBox in CLabel for accurate positioning
 - add onHover method in CLabel to change cursor shape
 - invoke onHover method with current mouse location
 - add getBoundingBox method to calculate the widget's bounding box
 - implement onHover method to update cursor shape on hover
@Memoraike
Copy link
Author

Moved 'containsPoint' to 'IWidget', added a virtual method 'getBoundingBox'.
Added a virtual 'onHover' method and a 'hovered' state to 'IWidget'. For example, I added a cursor change for the 'CLabel' and 'CPasswordInputField' widgets on hover.
Added a 'CLICKABLE' macro for future expansion.

 - modify cursor shape setting to only apply when onclickCommand is not empty
 - Improve hover state tracking for widgets
 - reduce unnecessary redraw calls by tracking hover changes
 - remove redundant renderAllOutputs() call
outputNeedsRedraw = true;
}

if (!cursorChanged) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

single if no {}

@@ -22,6 +22,9 @@ class CLabel : public IWidget {
void renderUpdate();
void onTimerUpdate();
void plantTimer();
CBox getBoundingBox() const override;
Copy link
Collaborator

Choose a reason for hiding this comment

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

style: just virtual, no override.

@@ -17,6 +18,13 @@ class IWidget {
static int roundingForBox(const CBox& box, int roundingConfig);
static int roundingForBorderBox(const CBox& borderBox, int roundingConfig, int thickness);

virtual CBox getBoundingBox() const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: maybe add a comment, or rename it to make it clear that this has to return Top-left position and size.

Copy link
Author

Choose a reason for hiding this comment

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

What about getBoundingClientRect?

Copy link
Collaborator

Choose a reason for hiding this comment

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

posFromHVAlign returns bottom-left positions, while wayland typically uses top-left. So just getBoundingBoxWL would work. getBoundingClientRect is also fine :)

@PaideiaDilemma
Copy link
Collaborator

works nicely!

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.

3 participants