lc-soft / LCUI

C library for building user interfaces
https://lcui-dev.github.io
MIT License
4.13k stars 356 forks source link

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

Closed jduo closed 4 years ago

jduo commented 4 years ago

Fixes #191.

Changes proposed in this pull request:

@lc-soft


IssueHunt Summary ### Referenced issues This pull request has been submitted to: - [#191: Add custom settings](https://issuehunt.io/repos/5293802/issues/191) ---
commit-lint[bot] commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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.

jduo commented 4 years ago

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

Thanks. I have no other changes planned.

lc-soft commented 4 years ago

@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 commented 4 years ago

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