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: Telemetry prompt in Welcome page #1927

Merged
merged 2 commits into from
Apr 4, 2023

Conversation

deboer-tim
Copy link
Collaborator

What does this PR do?

Moving the telemetry prompt from a standalone dialog to a section in the Welcome page. This indirectly fixes the problem of the prompt appearing underneath the main window on some OSes and provides a more natural experience.

The telemetry prompt enablement is intentionally independent from the welcome, so for example:

  • New users will see the welcome with telemetry.
  • Someone who used an older release (e.g. 0.11, prior to welcome) will have already responded to telemetry, so they will see the welcome without it.
  • If we improve the welcome in a future release, likewise new users would see both but upgrading users wouldn't be prompted for telemetry again.

Implementation notes:

  • Extracted telemetry constants into a settings file so that they can be used elsewhere cleanly.
  • Removed telemetry dialog and exposed configureTelemetry() to trigger the telemetry system.
  • The render layer needs to be able to access the trigger, so I exposed it to the window via index.ts.
  • Added functions to welcome-utils.ts to check if we need to prompt for telemetry, and save the result.
  • Added new section to WelcomePage and reformatted the container engines to match design.
  • Added tests to verify when telemetry section appears.

Screenshot/screencast of this PR

Screenshot 2023-04-03 at 12 55 00 PM

What issues does this PR fix or reference?

Issue #664.

How to test this PR?

To see the welcome and telemetry prompt again, delete the configuration file (e.g. .local/share/containers/podman-desktop/configuration/settings.json on Mac) or remove the telemetry and welcome settings.

Moving the telemetry prompt from a standalone dialog to a section in the Welcome page. This indirectly fixes the problem of the prompt appearing underneath the main window on some OSes and provides a more natural experience.

The telemetry prompt enablement is intentionally independent from the welcome, so for example:
- New users will see the welcome with telemetry.
- Someone who used an older release (e.g. 0.11, prior to welcome) will have already responded to telemetry, so they will see the welcome without it.
- If we improve the welcome in a future release, likewise new users would see both but upgrading users wouldn't be prompted for telemetry again.

Implementation notes:
- Extracted telemetry constants into a settings file so that they can be used elsewhere cleanly.
- Removed telemetry dialog and exposed configureTelemetry() to trigger the telemetry system.
- The render layer needs to be able to access the trigger, so I exposed it to the window via index.ts.
- Added functions to welcome-utils.ts to check if we need to prompt for telemetry, and save the result.
- Added new section to WelcomePage and reformatted the container engines to match design.
- Added tests to verify when telemetry section appears.

Signed-off-by: Tim deBoer <git@tdeboer.ca>
@deboer-tim deboer-tim requested review from a team and benoitf as code owners April 3, 2023 17:01
@deboer-tim deboer-tim requested review from jeffmaury, lstocchi and mairin and removed request for a team April 3, 2023 17:01
Copy link
Member

@mairin mairin left a comment

Choose a reason for hiding this comment

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

LGTM!

@slemeur
Copy link
Collaborator

slemeur commented Apr 3, 2023

👍

Copy link
Contributor

@dylanmtaylor dylanmtaylor left a comment

Choose a reason for hiding this comment

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

I'm not a maintainer here or anything, but I like this approach a lot 👍

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

testing all workflows I noticed that changing the telemetry enablement value is not taken into account until the next restart. Will open an issue as it does not seem related to this PR

packages/main/src/plugin/telemetry/telemetry.ts Outdated Show resolved Hide resolved
packages/main/src/plugin/telemetry/telemetry.ts Outdated Show resolved Hide resolved
packages/renderer/src/lib/welcome/WelcomePage.svelte Outdated Show resolved Hide resolved
packages/renderer/src/lib/welcome/WelcomePage.svelte Outdated Show resolved Hide resolved
Fixed formatting (added braces) and renamed enableTelemetry() to setTelemetry().

Signed-off-by: Tim deBoer <git@tdeboer.ca>
@deboer-tim deboer-tim merged commit 6300271 into containers:main Apr 4, 2023
@podman-desktop-bot podman-desktop-bot added this to the 0.14.0 milestone Apr 4, 2023
@deboer-tim deboer-tim deleted the message-telemetry branch May 4, 2023 18:20
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

6 participants