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

FIX: Overlay mode isn't applied via config file. #41

Merged
merged 6 commits into from
Oct 4, 2021

Conversation

SaeGon-Heo
Copy link
Contributor

@SaeGon-Heo SaeGon-Heo commented Oct 1, 2021

If I drag & drop config.kpsconf3 file on KeysPerSecond-v8.4.exe to run it with saved config which Overlay mode is true,
Overlay mode option is enabled but doesn't applied.
And this PR may fix this.

@RoanH RoanH self-requested a review October 3, 2021 22:57
@RoanH RoanH self-assigned this Oct 3, 2021
@RoanH RoanH added the bug label Oct 3, 2021
@RoanH
Copy link
Owner

RoanH commented Oct 3, 2021

Hey, thank you for the bug report and pull request. Nice job finding this bug. Your fix works properly, but the underlying issue is a bit more involved (honestly configuration files should be handled better, I'll get around to that someday). The stats saving is actually suffering from the exact same issue, for some reason I decided to manually manage the state of a few things even though I have a subroutine called reconfigure to do exactly that... And it seems like the manual managing did not work out for the overlay option and stats saving.

I'll push a few commits of my own to fix stats saving and then I'll merge this if you're fine with the changes I've made.

Copy link
Owner

@RoanH RoanH left a comment

Choose a reason for hiding this comment

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

I've made all the changes I wanted to make, if you're fine with the changes I made please approve or leave a comment and I'll merge this. Thanks again for contributing!

@SaeGon-Heo
Copy link
Contributor Author

SaeGon-Heo commented Oct 4, 2021

Looks cool. I didn't noticed reconfigure() function does at all :)
You may go ahead with that. Thanks to patch a bug!

@RoanH
Copy link
Owner

RoanH commented Oct 4, 2021

Awesome, thanks for the contribution.

@RoanH RoanH merged commit bed52cf into RoanH:master Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants