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

Send current synthDriver name and brailleDisplay name to update server for stats gathering. #8217

Merged
merged 10 commits into from
Jun 17, 2018

Conversation

michaelDCurran
Copy link
Member

Link to issue number:

None.

Summary of the issue:

Currently we have no idea what percentage of users use a particular synthDriver or brailleDisplay. This would be extremely useful data to better prioritize future work, including how much effort we put into eSpeak contributions for example.

Description of how this pull request fixes the issue:

Simply sends synthDriver name and brailleDisplay name as variables to the update server.
Eventually it would be nice if we could prepend the add-on name if there is one, but for now this at very least will give some much needed visibility for espeak usage.

Testing performed:

Instructed NVDA to check for update. Confirmed on the server that synthDriver name and brailleDisplay name were being gathered.

Known issues with pull request:

None.

Change log entry:

Changes:

  • When checking for updates, NVDA will now send the name of the current synth driver and braille display in use, to aide in better prioritization for future work on these drivers.

@michaelDCurran
Copy link
Member Author

A reminder that users can uncheck automatically check for updates in the General Settings, if they are uncomfortable with this data going to NV Access. But again, this data will greatly help us understand what we need to work on. There is no point putting large amounts of effort into eSpeak say if it was found athat only 10% of users actually use it.

michaelDCurran added a commit that referenced this pull request May 1, 2018
@LeonarddeR
Copy link
Collaborator

As discussed last week, how about adding the output braille table as well while at it? Cc @bramd

Also, I wonder whether there any privacy concerns. I don't care at all for myself, other's might.

@LeonarddeR
Copy link
Collaborator

@michaelDCurran commented on 1 mei 2018 08:35 CEST:

A reminder that users can uncheck automatically check for updates in the General Settings, if they are uncomfortable with this data going to NV Access. But again, this data will greatly help us understand what we need to work on.

Would it help to have a section in the user guide that explicitly states what data is send to NV Access?

@Brian1Gaff
Copy link

Brian1Gaff commented May 1, 2018 via email

@Brian1Gaff
Copy link

Brian1Gaff commented May 1, 2018 via email

@PratikP1
Copy link

PratikP1 commented May 1, 2018 via email

@bramd
Copy link
Contributor

bramd commented May 1, 2018

Sounds good to me. Braille output table is interesting as well, but only if we know the users' language/locale. So we should gather that as well if we don't already do that.

Regarding GDPR. I'm not a subject expert on the matter, but have lots of (government) clients who are really busy implementing GDPR measures right now, so I've heard a few examples there. Basically, it's best to ask consent for any data you want to gather and that could be qualified as personal data. Even an IP address seems to be classified as personal data as far as I know. Also, you need to be specific where the data is used for and it's not allowed to deny people functionality if they don't want to share their data. For example, if users don't agree to share their synth/braille display name, we might not ban them from getting updates automatically, since that data isn't strictly needed for the auto update function.

That being said, I don't know how it is if we aggregate the data directly after receiving it, so we can't see which specific person uses synth x. Since GDPR is a big issue in Europe right now it would be good to investigate if NVDA is GDPR compliant. That's outside the scope of this PR of course, but sooner or later European companies willing to use NVDA will request GDPR compliance.

@michaelDCurran
Copy link
Member Author

michaelDCurran commented May 1, 2018 via email

@Brian1Gaff
Copy link

Brian1Gaff commented May 2, 2018 via email

…hering, append either core, external or addon:addonName to better differentiate between the drivers.
…lso allow this to be configured from the General Settings tab. Document what data is sent in the userGuide. Also collect outputBrailleTable.
@@ -32,6 +32,16 @@
RPC_E_DISCONNECTED = -2147417848
LOAD_WITH_ALTERED_SEARCH_PATH=0x8

def isPathExternalToNVDA(path):
""" Checks if the given path is external to NVDA (I.e. not pointing to build-in code). """
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: build-in should be built-in

"installed": config.isInstalledCopy(),
"synthDriver":getQualifiedDriverClassNameForStats(synthDriverClass) if synthDriverClass else None,
"brailleDisplay":getQualifiedDriverClassNameForStats(brailleDisplayClass) if brailleDisplayClass else None,
"outputBrailleTable":config.conf['braille']['translationTable'] if brailleDisplayClass else None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth while adding a comment here to say that any additions must be added to the userguide.

==== Allow the NVDA project to gather NVDA usage statistics ====[GeneralSettingsGatherUsageStats]
If this is enabled, NV Access will use the information from update checks in order to track the number of NVDA users including particular demographics such as Operating system and country of origin.
Note that although your IP address will be used to calculate your country during the update check, the IP address is never kept.
A part from the mandatory information required to check for updates, the following extra infomation is also currently sent:
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: A part should be Apart

- Whether this copy of NVDA is portable or installed
- Name of the current speech synthesizer in use (including the name of the add-on the driver comes from)
- Name of the current Braille display in use (including the name of the add-on the driver comes from)
- The current output braille table (if Braille is in use)
Copy link
Contributor

Choose a reason for hiding this comment

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

Capital letter b for first usage of Braille

- The current output braille table (if Braille is in use)


This information greatly aides NV Access to prioritize future development of NVDA.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: too many spaces between greatly and aides.

==== Allow the NVDA project to gather NVDA usage statistics ====[GeneralSettingsGatherUsageStats]
If this is enabled, NV Access will use the information from update checks in order to track the number of NVDA users including particular demographics such as Operating system and country of origin.
Note that although your IP address will be used to calculate your country during the update check, the IP address is never kept.
Apart from the mandatory information required to check for updates, the following extra infomation is also currently sent:
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: infomation should be information

@michaelDCurran
Copy link
Member Author

I'm wondering about the yes no dialog on start-up. Is it too easy to dismiss in the affirmative? Perhaps the focus should default to the No button? But then I feel that can heavily bias the question. I'd really prefer that the focus be on neither... like... a cancel button? Then they'd be asked again the next time?

@feerrenrut
Copy link
Contributor

I'd really prefer that the focus be on neither... like... a cancel button? Then they'd be asked again the next time?

Yes, but perhaps "Ask later" or similar, rather than "cancel".

I am also a little worried about introducing multiple dialogs on startup. It could complicate things, but perhaps combining this with the welcome dialog when the welcome dialog is going to be displayed, and showing this prompt independently when the welcome dialog is not shown?

@LeonarddeR
Copy link
Collaborator

@feerrenrut commented on 7 mei 2018 08:45 CEST:

I am also a little worried about introducing multiple dialogs on startup. It could complicate things, but perhaps combining this with the welcome dialog when the welcome dialog is going to be displayed, and showing this prompt independently when the welcome dialog is not shown?

Hmm, I agree with having multiple prompts introducing more complexity, so I'd just add the prompt to the welcome dialog. We could use a config profile upgrade step to enforce the welcome dialog showing up again.

@michaelDCurran
Copy link
Member Author

michaelDCurran commented May 7, 2018 via email

@LeonarddeR
Copy link
Collaborator

Hmm, I think you have a valid point here.

Now, there are more things I could think of that at some point could end up in the welcome dialog, such as the possibility to enable or disable braille display auto detection. This might require the welcome dialog to become a multi step dialog and behave more like a wizard.

@feerrenrut
Copy link
Contributor

Trying to introduce this question into the welcome dialog also brings the danger that the question is hidden from the user. A user is unlikely to carefully inspect all options in the welcome dialog.

Mick, after considering this, I agree we should leave it as is.

@michaelDCurran
Copy link
Member Author

michaelDCurran commented May 7, 2018 via email

@feerrenrut
Copy link
Contributor

Ahhh. Yes. Sorry, I must have been tired when I wrote that. I think it's important to be able to dismiss the dialog without making an explicit decision.

@@ -924,3 +924,64 @@ def shouldConfigProfileTriggersBeSuspended():

def _isDebug():
return config.conf["debugLog"]["gui"]

class AskAllowUsageStatsDialog(wx.Dialog):
"""A dialog asking if the user whishes to allow NVDA usage stats to be collected by NV Access."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: 'wishes`


@classmethod
def run(self):
d=self(mainFrame)
Copy link
Contributor

Choose a reason for hiding this comment

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

You might consider using def runScriptModalDialog in source/gui/__init__ See #6355 (comment)

def run(self):
d=self(mainFrame)
mainFrame.prePopup()
d.Show()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect this to a be a modal dialog, especially considering there is the option to dismiss it and be reminded later.


remindMeButtonID=wx.NewId()
# Translators: The label of a button to remind the user later about performing some action.
remindMeButton = bHelper.addButton(self, remindMeButtonID, label=_("Remind me &later"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect the "remind me later" button to come last, in place of "cancel". Eg "Yes, No, Cancel" -> "Yes, No, Remind me later". Its fine for it to have focus.

To make it last, make it the last call to addButton

sHelper = guiHelper.BoxSizerHelper(self, orientation=wx.VERTICAL)

# Translators: A message asking the user if they want to allow usage stats gathering
message=_("In order to improve NVDA in the future, NV Access wishes to collect usage data from running copies of NVDA. "
Copy link
Contributor

Choose a reason for hiding this comment

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

For better formatting, can you add \n\n to the end of this line.

# Translators: A message asking the user if they want to allow usage stats gathering
message=_("In order to improve NVDA in the future, NV Access wishes to collect usage data from running copies of NVDA. "
"Data includes Operating System version, NVDA version, language, country of origin, plus certain NVDA configuration such as current synthesizer, braille display and braille table. "
"No spoken or braille content will be ever sent to NV Access. Please refer to the User Guide for a current list of all data collected.\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add a blank line here, so \n\n

"Data includes Operating System version, NVDA version, language, country of origin, plus certain NVDA configuration such as current synthesizer, braille display and braille table. "
"No spoken or braille content will be ever sent to NV Access. Please refer to the User Guide for a current list of all data collected.\n"
"Do you wish to allow NV Access to periodically collect this data in order to improve NVDA?")
sHelper.addItem(wx.StaticText(self, label=message))
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently this text causes the dialog to expand to the full width of the screen, I would suggest using something like the following so that the lines are wrapped when they hit some max length:

        sText = sHelper.addItem(wx.StaticText(self, label=message))
        # the wx.Window must be constructed before we can get the handle.
        import windowUtils
        self.scaleFactor = windowUtils.getWindowScalingFactor(self.GetHandle())
        sText.Wrap(self.scaleFactor*600) # 600 was fairly arbitrarily chosen by a visual user to look acceptable on their machine.

source/core.py Outdated
@@ -61,6 +61,8 @@ def doStartupDialogs():
gui.messageBox(_("Your gesture map file contains errors.\n"
"More details about the errors can be found in the log file."),
_("gesture map File Error"), wx.OK|wx.ICON_EXCLAMATION)
if not config.conf['update']['askedAllowUsageStats']:
gui.AskAllowUsageStatsDialog.run()
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably only ask if updating is turned on? The check box is hidden in that case already right?

In fact, this should probably not be shown unless updating is possible. For instance, when run from the launcher or portable copies.


def onNoButton(self,evt):
log.debug("Usage stats gathering has been disallowed")
config.conf['update']['askedAllowUsageStats']=True
config.conf['update']['allowUsageStats']=False
self.Destroy()
self.EndModal(wx.ID_NO)

def onLaterButton(self,evt):
log.debug("Usage stats gathering question has been deferred")
evt.Skip()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised that you don't need to call EndModal here? Could it be because of the evt.Skip()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you calling Skip() on this?

@michaelDCurran
Copy link
Member Author

michaelDCurran commented May 14, 2018 via email

@feerrenrut
Copy link
Contributor

Ah yes, I see. I missed the id change for that button. Could you add comment to that effect to that line, just to make it clearer why there is inconsistency with the others. Something like: "evt.Skip() is called since wx.ID_CANCEL is used as the ID for the Ask Later button, wx automatically ends the modal itself."

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Looks good

michaelDCurran added a commit that referenced this pull request May 15, 2018
@@ -633,6 +633,12 @@ def makeSettings(self, settingsSizer):
if globalVars.appArgs.secure:
item.Disable()
settingsSizerHelper.addItem(item)
# Translators: The label of a checkbox in general settings to toggle allowing of usage stats gathering
item=self.allowUsageStatsCheckBox=wx.CheckBox(self,label=_("Allow the NVDA project to gather NVDA usage statistics"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually think it makes sense to reposition this checkbox after the notifyForPendingUpdateCheckBox. The current order of check boxes is a bit arbitrary now.

@netblue44
Copy link

netblue44 commented May 18, 2018 via email

@bramd
Copy link
Contributor

bramd commented May 18, 2018 via email

@netblue44
Copy link

netblue44 commented May 18, 2018 via email

LeonarddeR pushed a commit that referenced this pull request May 21, 2018
Merge remote-tracking branch 'origin/pr/8217' into next
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

8 participants