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

fix(windows): Check registry report usage value in setup process #13215

Open
wants to merge 4 commits into
base: beta
Choose a base branch
from
Open

fix(windows): Check registry report usage value in setup process #13215

wants to merge 4 commits into from
+14 −4

Conversation

rc-swag
Copy link
Contributor

@rc-swag rc-swag commented Feb 13, 2025 •
edited
Loading

Fixes: #12660

User Testing

TEST_USAGE_STATS

  1. Download the Keyman build from this PR
  2. Start the install select the install options and uncheck “Share anonymous usage statistics with keyman.com”
  3. Also make sure the version being installed is this PR's version
    Expected Result:
  4. After installation has finished go to Keyman options and make sure the option “Share anonymous usage statistics with keyman.com” is unchecked

@rc-swag rc-swag self-assigned this Feb 13, 2025
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Feb 13, 2025
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Feb 13, 2025 •
edited
Loading

User Test Results

Test specification and instructions

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the B18S1 milestone Feb 13, 2025
@rc-swag
fix(windows): link setup options with registry value
46413f8 
the automatic report usage result from the lblOptions window in setup
was not assigned on mrOK. Therefore the check box selection was lost.
@darcywong00 darcywong00 modified the milestones: B18S1, B18S2 Feb 15, 2025
@rc-swag
fix(windows): automaticaly report usage defualt false in options
ba22be5 
The process is quite confusing. The options from the setup install
options will only resualt in a command line option being past through
to firstrun if it is true. If is false the options will not be passed
through. Therefore if this is the first installation once when the
Keyman Configuration is opened it will show the default option
configured in utilkeymanoptions.pas, in kmcomapi. This result of all
this was a user would deselect the option in setup. When they opened
configuration it was set to true.
@rc-swag rc-swag marked this pull request as ready for review February 19, 2025 01:41
mcdurdin
mcdurdin reviewed Feb 20, 2025
View reviewed changes
windows/src/desktop/setup/UfrmRunDesktop.pas
@@ -1033,16 +1034,25 @@ procedure TfrmRunDesktop.GetDefaultSettings; // I2651
begin
FStartWithWindows := True; // I2607
FCheckForUpdates := True; // I2609
FAutomaticallyReportUsage := True;
Copy link
Member

Choose a reason for hiding this comment

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

You have the default True here but seems to be False in utilkeymanoption.pas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see this commit message ba22be5

Copy link
Member

Choose a reason for hiding this comment

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

I'm still not really 100% clear (and I think I wrote the code), but I don't understand why this would differ from the koAutomaticallyReportErrors flag?

@keymanapp-test-bot keymanapp-test-bot bot added user-test-required User tests have not been completed and removed user-test-missing User tests have not yet been defined for the PR labels Feb 20, 2025
@dinakaranr
Copy link

Test Results

I tested this issue with the attached Keyman"18.0.196-beta-test-13215" build(20/02/2025) on Windows 10. Here I am sharing my observation.

  • TEST_USAGE_STATS (Passed):
  1. Download the Keyman build from this PR
  2. Install the Keyman installer file.
  3. Start the Keyman app.
  4. Click the "install options" link button.
  5. Uncheck the “Share anonymous usage statistics with keyman.com” option.
  6. Click the "Install" button.
  7. Open the configuration dialog.
  8. Click on the "Support" tab.
  9. Verified that the keyman version matched with the attached PR's version.
  10. Click on the "option" tab.
  11. Verified that the “Share anonymous usage statistics with keyman.com” check box appeared unchecked.
    It works well. Thank you.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Feb 20, 2025
@rc-swag
Copy link
Contributor Author

rc-swag commented Feb 20, 2025

Putting the commit message here to help future me.

The process is quite confusing. The options from the setup install
options will only result in a command line option being passed through
to firstrun if it is true. If is false the options will not be passed
through. Therefore if this is the first installation once when the
Keyman Configuration is opened it will show the default option
configured in utilkeymanoptions.pas, in kmcomapi. The result of all
this was a user would deselect the option in setup. When they opened
configuration it was set to true.

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

@mcdurdin mcdurdin mcdurdin left review comments

@ermshiperete ermshiperete ermshiperete approved these changes

Assignees

@rc-swag rc-swag

Labels
fix has-user-test windows/config/ windows/engine/ windows/
Projects
Keyman
Status: No status
Milestone
B18S2
Development

Successfully merging this pull request may close these issues.
5 participants
@rc-swag @dinakaranr @ermshiperete @mcdurdin @darcywong00