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

[Feature Request]: Add support for logging device DPI for events #5250

Closed
BenHenning opened this issue Nov 29, 2023 · 5 comments · Fixed by #5270
Closed

[Feature Request]: Add support for logging device DPI for events #5250

BenHenning opened this issue Nov 29, 2023 · 5 comments · Fixed by #5270
Assignees
Labels
enhancement End user-perceivable enhancements. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks.

Comments

@BenHenning
Copy link
Member

BenHenning commented Nov 29, 2023

Is your feature request related to a problem? Please describe.

Events need to include the device's (unscaled) DPI as part of its log so that we can get an idea on how devices span different density formats.

Describe the solution you'd like

An integer for dpi (e.g. 320) should be logged as an event dimension (e.g. in EventBundleCreator). This should NOT be affected by display zoom.

Describe alternatives you've considered

There isn't a good alternative to get display density (one would need to manually look up the device specs).

Additional context

Needed to determine whether we need to address #360, plus it'll help with other decisions in the future.

@BenHenning BenHenning added enhancement End user-perceivable enhancements. triage needed labels Nov 29, 2023
@adhiamboperes adhiamboperes added Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks. labels Dec 4, 2023
@Tejas-67
Copy link
Contributor

Tejas-67 commented Dec 9, 2023

Hi @BenHenning , I would like to work on this issue. Please assign this to me.

In the EventBundleCreator.kt file, we can have something like:
private val screenDensity by lazy { context.resources.displayMetrics.densityDpi }
and in the fillEventBundle function:
bundle.putInt( "screen_density", screenDensity )

rest all remains same.

@Tejas-67
Copy link
Contributor

Should I work on this issue? @BenHenning @adhiamboperes

@adhiamboperes
Copy link
Collaborator

@Tejas-67, how would you address this:

An integer for dpi (e.g. 320) should be logged as an event dimension (e.g. in EventBundleCreator). This should NOT be affected by display zoom.

@Tejas-67
Copy link
Contributor

As mentioned in the previous comment, we can use resources.displayMetrics.densityDpi to get the device density.
@adhiamboperes

@adhiamboperes
Copy link
Collaborator

@BenHenning, does this value need to be inclu ded for every eventlog?

adhiamboperes added a commit that referenced this issue Jan 30, 2024
<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
Fix #5250 In the EventBundleCreator class, introduced a new item in the
bundle to log the screen density of the device. Stored using the key
"screen_density" and value retrieved from
resources.displayMetrics.screenDensity .

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

---------

Co-authored-by: Tejas-67 <[email protected]>
Co-authored-by: Adhiambo Peres <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement End user-perceivable enhancements. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks.
Development

Successfully merging a pull request may close this issue.

4 participants