-
Notifications
You must be signed in to change notification settings - Fork 1.3k
For #21732 - Adds inactive tabs survey on disable #21862
For #21732 - Adds inactive tabs survey on disable #21862
Conversation
Request for data collection review
|
|
||
// Sets the Radio buttons' text | ||
radioButtonsMap.forEach { | ||
inactiveTabsSurveyBinding.surveyGroup.findViewById<RadioButton>(it.key)?.text = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why set the text here instead of in the XML?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed to send the user's selected feedback back through Glean.
- I couldn't use the actual text on screen as that is dependent on the user's language setting. So no
radioButton.text
can be used to get the selected feedback. - I don't know how to get the StringID from a view.
- So I decided to use a map of the StringIDs so that onSEND I can easily get the English text to add to the Glean event.
- But how do I know which StringID correlates to which Radio button? Yeah I know their order and
@id
s but that's a rename/refactor away from being broken. - So I wanted a tighter coupling between ViewIDs and StringIDs and went with the
mapOf<ViewID, StringID>
. - Now if someone wants to change the string resource of one of the RadioButtons they won't just change in the XML and forget about the .kt file, they will be forced to changed in the .kt file. 😅
Not sure if it's the best solution but 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I wanted a tighter coupling between ViewIDs and StringIDs and went with the mapOf<ViewID, StringID>.
One thing about this approach is if we change the string, the data in the telemetry will show a divergence of options.
Example: The string "I don't understand how it works" changes to "I don't know it works" will mean we start to get two results from the one option.
We could map the options to an internal value that we submit to telemetry (which defeats the problem you were trying to solve). 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that if we ever want to change the strings we should change the Event as well.
Otherwise it will be like running a survey on the street and halfway through you change the answer choices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MozillaNoah @jonalmeida I plan to finish this after Codrut left so I'm just wondering if I should keep the currently proposed implementation (which I also favor, kinda like having it all in one place, easier to follow) or should I add the string values in xml and then read the text of the selected radio button - infer this in place, don't use the map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong feelings for either solution. 🙂
Data Review
Yes, through the metrics.yaml and the Glean Dictionary
Yes, through the "Send Usage Data" preference in the application settings
N/A, collection set to end or be renewed by 2022-02-01
Category 2, Interaction data
default-on
No
Yes
No Resultdata-review+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be sure and update the data_sensitivity
in the metrics.yaml, as well as the bugs
and data_reviews
to point at this PR. Thanks!
data-review+
extra_keys: | ||
feedback: | ||
description: | | ||
The user's feedback regarding inactive tabs feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend adding a type
, following the example from the Glean docs here:
https://mozilla.github.io/glean/book/reference/metrics/event.html#extra_keys
I will also caution you that the limitation for this extra will truncate the value at 100 UTF8 characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added type: string
for the extra key.
Currently 100 characters is more than enough but I'll also leave a comment in code about this for the future, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the type for the extra key:
Seems like with this change the generated code for the extra keys will have them as properties of a data class:
data class TurnOffInactiveTabsSurveyExtra(
val feedback: String? = null
) : EventExtras {
override fun toFfiExtra(): Pair<IntArray, List<String>> {
val keys = mutableListOf<Int>()
val values = mutableListOf<String>()
this.feedback?.let {
keys.add(0)
values.add(it.toString())
}
return Pair(IntArray(keys.size, { keys[it] }), values)
}
}
type which is incompatible with what our code expects when parsing the extras - a map.
So atm it would probably be easier to just go with the old way of setting extras - without a type.
Seems like migrating to use the new APIs is planned for #19967
This pull request has conflicts when rebasing. Could you fix it @codrut-topliceanu? 🙏 |
6b133da
to
acaa1a4
Compare
Talked with Kimmy about these changes since there wasn't a DO ticket filed. |
acaa1a4
to
22b8548
Compare
Rebased and did some small updates to the original PR:
|
Sorry for asking for a re-review Travis. Not intended. I see the PR still has your approval. |
22b8548
to
7d99b6d
Compare
Kimmy asked to add a new probe for when the survey shows up. |
Request for data collection review
|
@travis79 It seems like there really is need for data-review again. |
Data Review
Yes, through the metrics.yaml and the Glean Dictionary
Yes, through the "Send Usage Data" preference in the application settings
N/A, collection set to end or be renewed by 2022-02-01
Category 2, Interaction data
default-on
No
Yes
No Resultdata-review+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data-review+ for adding the event
7d99b6d
to
86a508a
Compare
Seemingly unrelated ui test failures. |
@Mergifyio backport releases_v94.0.0 |
✅ Backports have been created
|
For #21732
Pull Request checklist
To download an APK when reviewing a PR: