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.

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

Jump to bottom

feat(util): add settings api (#191) #211

Merged
merged 1 commit into from
Jul 2, 2020
Merged

feat(util): add settings api (#191) #211

merged 1 commit into from
Jul 2, 2020

Conversation

jduo
Copy link
Contributor

@jduo jduo commented Jun 25, 2020 •
edited by issuehunt-oss bot
Loading

Fixes #191.

Changes proposed in this pull request:

  • Add API for retrieving and modifying global settings
  • Utilize this API in the codebase
  • Add event for when global settings are changed
  • Add event handler for updating the frame rate cap
  • Add event handler to notify paint flashing changes

@lc-soft

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

@commit-lint
Copy link

commit-lint bot commented Jun 25, 2020 •
edited
Loading

Features

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

@jduo jduo changed the title feat(util): add settings api (#191) WIP feat(util): add settings api (#191) Jun 25, 2020
lc-soft
lc-soft requested changes Jun 28, 2020
View reviewed changes
src/settings.c Outdated Show resolved Hide resolved
src/settings.c Show resolved Hide resolved
@jduo jduo force-pushed the custom-settings branch from 5b99e68 to d2cce7c Compare June 30, 2020 11:04
@jduo jduo changed the title WIP feat(util): add settings api (#191) feat(util): add settings api (#191) Jun 30, 2020
lc-soft
lc-soft requested changes Jun 30, 2020
View reviewed changes
Copy link
Owner

@lc-soft lc-soft left a comment

Choose a reason for hiding this comment

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

Some tests need to be added.

test/test_settings.c Show resolved Hide resolved
@jduo jduo force-pushed the custom-settings branch 7 times, most recently from 7549e83 to 3903c7a Compare June 30, 2020 19:25
@jduo
Copy link
Contributor Author

jduo commented Jun 30, 2020

@lc-soft , I'm tracking down one test failure related to this and wondering if you might have any ideas.
https://travis-ci.org/github/lc-soft/LCUI/jobs/703612060

The first time we do the frame rate comparison above, it is always using the 120 cap instead of the one supplied in the settings. Subsequent calls to LCUI_ApplySettings do work. It's not related to the frame rate cap value itself, I've tried using 5 the first time and seen it fail.

I feel like event fired by using LCU_ResetSettings() while initializing LCUI is getting evaluated instead of the one fired during LCUI_ApplySettings(), but there should be no event handler bound at this time.

@jduo jduo force-pushed the custom-settings branch 4 times, most recently from d2b4d80 to 879886f Compare July 1, 2020 00:39
lc-soft
lc-soft requested changes Jul 1, 2020
View reviewed changes
src/settings.c Outdated Show resolved Hide resolved
test/test_settings.c Outdated Show resolved Hide resolved
lc-soft
lc-soft requested changes Jul 1, 2020
View reviewed changes
src/main.c Outdated Show resolved Hide resolved
@jduo jduo force-pushed the custom-settings branch 7 times, most recently from 46933c7 to c218614 Compare July 1, 2020 10:46
@jduo
Copy link
Contributor Author

jduo commented Jul 1, 2020

In the output of this run (https://travis-ci.org/github/lc-soft/LCUI/jobs/703866145), the code in the event handler isn't getting reached in the failing test so we're either not firing the event, or we haven't set the handler:
image
vs
image

@jduo jduo force-pushed the custom-settings branch 2 times, most recently from bc463bc to 8f60c60 Compare July 1, 2020 16:01
@jduo jduo force-pushed the custom-settings branch 3 times, most recently from 11b2625 to d77bcef Compare July 1, 2020 16:20
@jduo
Copy link
Contributor Author

jduo commented Jul 1, 2020

I fixed an incorrect call to LCUI_Quit() instead of LCUI_Destroy() in the preceding tests which allows the first frame rate check to pass. However it seems to crash while running the destroy event handler now.

This also happens if I disable preceding settings tests. Still investigating.

lc-soft
lc-soft requested changes Jul 2, 2020
View reviewed changes
test/test_settings.c Outdated Show resolved Hide resolved
src/settings.c Outdated Show resolved Hide resolved
@jduo jduo force-pushed the custom-settings branch 4 times, most recently from e1ad809 to 4fa7eec Compare July 2, 2020 04:56
@jduo
feat(util): add settings api (lc-soft#191)
ea7ccfc 
BREAKING CHANGE: LCUIDisplay_EnablePaintFlashing() has been removed.

- Add API for retrieving and modifying global settings
- Utilize this API in the codebase
- Add event for when global settings are changed
- Add event handler for updating the frame rate cap
- Add event handler to notify paint flashing changes
- Add unit tests

Fixes lc-soft#192
@jduo jduo force-pushed the custom-settings branch from 4fa7eec to ea7ccfc Compare July 2, 2020 05:07
lc-soft
lc-soft approved these changes Jul 2, 2020
View reviewed changes
Copy link
Owner

@lc-soft lc-soft left a comment

Choose a reason for hiding this comment

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

If you have nothing to add, I will merge it.

@jduo
Copy link
Contributor Author

jduo commented Jul 2, 2020

If you have nothing to add, I will merge it.

Thanks. I have no other changes planned.

@lc-soft lc-soft merged commit 209d757 into lc-soft:develop Jul 2, 2020
@lc-soft
Copy link
Owner

lc-soft commented Jul 2, 2020 •
edited
Loading

@jduo

Sorry, I found the BREAKING CHANGE mark when I clicked the merge button:

BREAKING CHANGE: LCUIDisplay_EnablePaintFlashing() has been removed.

Please add a compatible implementation of LCUIDisplay_EnablePaintFlashing(), I don't want to update the version number from 2.0.0 to 3.0.0 because of this BREAKING CHANGE.

jduo added a commit to jduo/LCUI that referenced this pull request Jul 2, 2020
@jduo
feat: add settings api (lc-soft#191) (lc-soft#211)
89f2bd8 
Restore and re-implement LCUIDisplay_EnablePaintFlashing.

Fixes lc-soft#192
jduo added a commit to jduo/LCUI that referenced this pull request Jul 2, 2020
@jduo
feat: add settings api (lc-soft#191) (lc-soft#211)
e1d5849 
Restore and re-implement LCUIDisplay_EnablePaintFlashing.

Fixes lc-soft#191
@commit-lint commit-lint bot mentioned this pull request Jul 2, 2020
Merged
@jduo
Copy link
Contributor Author

jduo commented Jul 2, 2020

I created a separate PR #212 for this.
Please let me know if there's a different process I should follow.

lc-soft pushed a commit that referenced this pull request Jul 2, 2020
@jduo
feat: add settings api (#191) (#211) (#212)
94be958 
Restore and re-implement LCUIDisplay_EnablePaintFlashing.

Fixes #191
lc-soft pushed a commit that referenced this pull request Jul 2, 2020
@jduo @lc-soft
feat: add settings api (#191) (#211) (#212)
1b8c6e6 
- Add API for retrieving and modifying global settings
- Utilize this API in the codebase
- Add event for when global settings are changed
- Add event handler for updating the frame rate cap
- Add event handler to notify paint flashing changes
- Add unit tests

Fixes #192

Co-authored-by: James Duong <[email protected]>

feat: add settings api (#191) (#211) (#212)

Restore and re-implement LCUIDisplay_EnablePaintFlashing.

Fixes #191
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lc-soft lc-soft lc-soft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add custom settings
2 participants
@jduo @lc-soft