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(lv_spinbox): support both right-to-left and left-to-right digit steps when clicking encoder button #2644

Merged
merged 14 commits into from
Oct 14, 2021

Conversation

gesture1968
Copy link
Contributor

@gesture1968 gesture1968 commented Oct 7, 2021

Description of the feature or fix

Adding Spinbox support for moving the digitposition both from left-to-right and right-to-left when editing a spinbox and clicking the encoder button. The current behaviour is clicking the encoder button only moves the digitposition from right to left (from MSB to LSB)

Checkpoints

* Added support for moving the Spinbox digit position from right-to-left when clicking the button on an encoder. The default behaviour is when clicking the encoder button, the digit is moved from left-to-right (MSB to LSB). 
* Added a check to see if the spinbox digit-count is just one. In that case it is pointless to check the buttonclick
* See also the spinbox.h file
@embeddedt
Copy link
Member

Please submit these as one PR. We cannot be merging features in two separate PRs.

Even if you are just using the GitHub web editor, you should be able to navigate to your patch-1 branch and keep editing files in it instead of creating a new branch for each file.

@embeddedt embeddedt mentioned this pull request Oct 7, 2021
3 tasks
* Forgot the implementation of the setter function
* forgot a ;
Adding Spinbox support for moving the digitposition both from left-to-right and right-to-left when editing a spinbox and clicking the encoder button. The current behaviour is clicking the encoder button only moves the digitposition from right to left (from MSB to LSB)
@gesture1968 gesture1968 changed the title Update lv_spinbox.c Update lv_spinbox Oct 7, 2021
@gesture1968
Copy link
Contributor Author

Ok, it's my first pull request. I now understand all files within (my) branch are updated here :)

@gesture1968 gesture1968 changed the title Update lv_spinbox Update lv_spinbox to support both right-to-left and left-to-right digit steps when clicking encoder button Oct 7, 2021
Added brief / comment to new function
More clear Brief / Comment
@gesture1968 gesture1968 changed the title Update lv_spinbox to support both right-to-left and left-to-right digit steps when clicking encoder button feat(lv_spinbox): support both right-to-left and left-to-right digit steps when clicking encoder button Oct 7, 2021
Comment on lines 348 to 357
uint32_t Power10(uint8_t exp)
{
uint32_t res = 1;
for (uint8_t t = 0; t < exp; t++)
{
res = res * 10;
}
return res;
}

Copy link
Member

Choose a reason for hiding this comment

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

You can't nest functions like this in C99-compliant code, but I think you can replace this with lv_pow(10, exp) anyways. Any reason for the separate function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did not know about the lv_pow function, I will change it

src/extra/widgets/spinbox/lv_spinbox.h Outdated Show resolved Hide resolved
src/extra/widgets/spinbox/lv_spinbox.h Outdated Show resolved Hide resolved
Comment on lines 128 to 129
bool
lv_spinbox_get_rollover(lv_obj_t *obj);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bool
lv_spinbox_get_rollover(lv_obj_t *obj);
bool lv_spinbox_get_rollover(lv_obj_t *obj);

nested function replaced by lv_pow fiunction
removed spaces
* @param spinbox pointer to spinbox
* @param direction the direction (LV_SPINBOX_DIGIT_DIR_TO_RIGHT or LV_SPINBOX_DIGIT_DIR_TO_LEFT)
*/
void lv_spinbox_set_digit_step_direction(lv_obj_t *obj, uint8_t direction)
Copy link
Member

Choose a reason for hiding this comment

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

We can use lv_dir_t with LV_DIR_LEFT/RIGHT values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have updated the source to use the lv_dir_t

@kisvegabor
Copy link
Member

Thanks, please don't forget to update the docs too. 🙂

gesture1968 and others added 5 commits October 8, 2021 12:18
Replaced type used for direction of digit step when clicking an encoder with existing LVGL lv_dir_t
Replaced type used for direction of digit step when clicking an encoder with existing LVGL lv_dir_t
Added comment for the new function 'lv_spinbox_set_digit_step_direction'
Co-authored-by: embeddedt <42941056+embeddedt@users.noreply.github.com>
Co-authored-by: embeddedt <42941056+embeddedt@users.noreply.github.com>
Copy link
Member

@kisvegabor kisvegabor left a comment

Choose a reason for hiding this comment

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

Looks good to me, except one minor thing.

@@ -318,6 +332,7 @@ static void lv_spinbox_constructor(const lv_obj_class_t * class_p, lv_obj_t * ob
spinbox->range_max = 99999;
spinbox->range_min = -99999;
spinbox->rollover = false;
spinbox->digit_step_dir = LV_SPINBOX_DIGIT_DIR_TO_RIGHT;
Copy link
Member

Choose a reason for hiding this comment

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

It should be updated to LV_DIR_LEFT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it's corrected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've set it to LV_DIR_RIGHT, as it is the default before my contribution.

bug: old definition LV_SPINBOX_DIGIT_DIR_TO_RIGHT changed to LV_DIR_RIGHT
@@ -113,7 +122,8 @@ void lv_spinbox_set_pos(lv_obj_t * obj, uint8_t pos);
* Get spinbox rollover function status
* @param spinbox pointer to spinbox
*/
bool lv_spinbox_get_rollover(lv_obj_t * obj);
bool
lv_spinbox_get_rollover(lv_obj_t *obj);
Copy link
Member

Choose a reason for hiding this comment

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

Thank you! One last nitpick, can you fix the spurious newline added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Extra linefeed removed
@kisvegabor
Copy link
Member

Looks good, thank you!

@kisvegabor kisvegabor merged commit 2a701ee into lvgl:master Oct 14, 2021
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.

None yet

3 participants