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

michaelDCurran · 2018-05-01T06:24:44Z

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.

…r for stats gathering.

michaelDCurran · 2018-05-01T06:35:07Z

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.

LeonarddeR · 2018-05-01T06:43:26Z

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 · 2018-05-01T06:47:15Z

@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 · 2018-05-01T11:24:26Z

Does this depth of data gathering contravene the GDPR in Europe, ie all data collected needs to be opt in. Just thought I'd mention it as its creating all kinds of problems for small charities who deal with blind people over here. 25th may is the start date. You don't want to include anything in nvda which might make a government decide not to use it. Brian bglists

Brian1Gaff · 2018-05-01T11:29:11Z

I think all data sent needs to be opt in to conform with the Eus GDPR in future in any case though as I said in my last post here. be very careful with data collection as its a dodgy thing at the moment as you will be well aware. It may well be annon, but if enough data is collected it could be used to find out who a person is. OK I think this is probably paranoid, but they do seem to be serious about controlling this as an opt in action if all the emails from companies and organisation from all over the globe I'm getting at the moment are anything to go by!

PratikP1 · 2018-05-01T12:48:28Z

GDPR could certainly be an issue. It can be resolved by having a popup alert when the user checks the automatic update checkbox. Collection practices should certainly be documented.

bramd · 2018-05-01T19:31:06Z

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 · 2018-05-01T20:26:49Z

We will certainly look into some kind of opt-in system before this gets to Master.

Brian1Gaff · 2018-05-02T16:36:27Z

Cheers, i have no objection myself you unnderstand. however people with muliltiple synths some configured for say all will they show up? At present I'm using speech player in espeak but with quincy as it has some bettterEnglish handling than the current one native to nvda. Brian

…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.

feerrenrut · 2018-05-07T00:50:18Z

source/logHandler.py

@@ -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). """


Typo: build-in should be built-in

feerrenrut · 2018-05-07T00:51:55Z

source/updateCheck.py

+			"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,


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

feerrenrut · 2018-05-07T00:53:36Z

user_docs/en/userGuide.t2t

+==== 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:


Typo: A part should be Apart

feerrenrut · 2018-05-07T00:54:15Z

user_docs/en/userGuide.t2t

+- 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)


Capital letter b for first usage of Braille

feerrenrut · 2018-05-07T00:55:22Z

user_docs/en/userGuide.t2t

+- The current output braille table (if Braille is in use)
+
+
+This information greatly   aides NV Access to prioritize future development of NVDA.


Typo: too many spaces between greatly and aides.

LeonarddeR · 2018-05-07T04:02:06Z

user_docs/en/userGuide.t2t

+==== 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:


typo: infomation should be information

michaelDCurran · 2018-05-07T04:07:17Z

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 · 2018-05-07T06:45:00Z

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 · 2018-05-07T06:58:52Z

@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 · 2018-05-07T07:06:35Z

I am very worried that we won't be able to provide the required text within the Welcome dialog for that specific option. I really feel if it is going to be asked at all that it should be in its own dialog. Also, the fact that the Welcome dialog reappears for all users in a future release I think will be more confusing than an extra dialog.

LeonarddeR · 2018-05-07T07:09:43Z

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 · 2018-05-07T07:10:21Z

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 · 2018-05-07T07:17:21Z

Should we still change the dialog buttons to Yes, No, Ask later?

feerrenrut · 2018-05-07T23:16:58Z

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.

…ons, with Ask later being the default.

feerrenrut · 2018-05-14T01:07:35Z

source/gui/__init__.py

@@ -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."""


Typo: 'wishes`

feerrenrut · 2018-05-14T01:18:46Z

source/gui/__init__.py

+
+	@classmethod
+	def run(self):
+		d=self(mainFrame)


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

feerrenrut · 2018-05-14T01:19:48Z

source/gui/__init__.py

+	def run(self):
+		d=self(mainFrame)
+		mainFrame.prePopup()
+		d.Show()


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

feerrenrut · 2018-05-14T01:26:37Z

source/gui/__init__.py

+
+		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"))


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

feerrenrut · 2018-05-14T01:36:35Z

source/gui/__init__.py

+		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. "


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

feerrenrut · 2018-05-14T01:37:26Z

source/gui/__init__.py

+		# 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"


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

feerrenrut · 2018-05-14T01:48:21Z

source/gui/__init__.py

+			"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))


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.

feerrenrut · 2018-05-14T01:52:33Z

source/core.py

@@ -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()


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.

feerrenrut · 2018-05-14T07:54:59Z

source/gui/__init__.py


 	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()


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

Why are you calling Skip() on this?

michaelDCurran · 2018-05-14T09:04:09Z

evt.Skip causes the default behaviour for this event to occur. As I have used wx.ID_CANCEL as the ID for the Ask Later button, wx automatically ends the modal itself.

feerrenrut · 2018-05-14T23:39:24Z

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."

feerrenrut

Looks good

LeonarddeR · 2018-05-16T17:44:01Z

source/gui/settingsDialogs.py

@@ -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"))


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 · 2018-05-18T15:19:48Z

Wait! when restarting NVDA the stats dialog pops up regardless of what option is selected. This happens even when it is configured from the settings dialog. The stats dialog shud not pop up when selecting yes or no from the settings dialog nore when selecting from the dialog that pops up when NVDA is restarded.On 18/05/2018 01:13 a. m., Leonard de Ruijter wrote:

…

***@***.**** approved this pull request. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#8217 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/API6eVGMpwppYpxDmvRjUVEmK0aWqtGbks5tznSngaJpZM4Ttn7I>.

bramd · 2018-05-18T17:34:33Z

I had the same issue. I don't save config on exit and had to save the config manually. This is kind of logical when you think of it, but somehow I wasn't triggered to save my config after answering this prompt.

netblue44 · 2018-05-18T20:07:57Z

This does not work fore me no matter what i do.

…

On 18/05/2018 11:34 a. m., Bram Duvigneau wrote: I had the same issue. I don't save config on exit and had to save the config manually. This is kind of logical when you think of it, but somehow I wasn't triggered to save my config after answering this prompt. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8217 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/API6eUk9LmdGfdKD5QVEPW4k4wncSXDnks5tzwYvgaJpZM4Ttn7I>.

Merge remote-tracking branch 'origin/pr/8217' into next

Send current synthDriver name and brailleDisplay name to update serve…

06a93fb

…r for stats gathering.

michaelDCurran requested a review from feerrenrut May 1, 2018 06:24

feerrenrut approved these changes May 1, 2018

View reviewed changes

michaelDCurran added a commit that referenced this pull request May 1, 2018

Incubates #8217

7876b98

nvaccessAuto added the incubating label May 1, 2018

nvaccessAuto assigned michaelDCurran May 1, 2018

michaelDCurran added 2 commits May 3, 2018 03:35

When sending synth / braille driver names to the server for stats gat…

294bb1b

…hering, append either core, external or addon:addonName to better differentiate between the drivers.

Ask the user if they wish to allow sending usage data to NV Access. A…

f303ca6

…lso allow this to be configured from the General Settings tab. Document what data is sent in the userGuide. Also collect outputBrailleTable.

feerrenrut suggested changes May 7, 2018

View reviewed changes

Address review actions

f8d0c48

LeonarddeR reviewed May 7, 2018

View reviewed changes

Fix typo

c40db2e

feerrenrut approved these changes May 7, 2018

View reviewed changes

Implement new AllowUsageStats dialog with yes, no, and ask later butt…

65dabee

…ons, with Ask later being the default.

michaelDCurran requested a review from feerrenrut May 10, 2018 20:15

feerrenrut suggested changes May 14, 2018

View reviewed changes

Address review actions.

da91a7e

feerrenrut reviewed May 14, 2018

View reviewed changes

Add comment

50beb08

feerrenrut approved these changes May 15, 2018

View reviewed changes

michaelDCurran added a commit that referenced this pull request May 15, 2018

Incubates #8217

e910626

LeonarddeR reviewed May 16, 2018

View reviewed changes

Move allow usage stats checkbox to the end of General Settings.

8d78b01

michaelDCurran requested a review from LeonarddeR May 16, 2018 23:42

LeonarddeR approved these changes May 18, 2018

View reviewed changes

LeonarddeR pushed a commit that referenced this pull request May 21, 2018

Incubates #8217.

d1d1307

Merge remote-tracking branch 'origin/pr/8217' into next

nvaccessAuto assigned LeonarddeR and unassigned michaelDCurran May 21, 2018

Merge branch 'master' into synthBrailleStats

3dc51c1

michaelDCurran merged commit e791c8d into master Jun 17, 2018

nvaccessAuto removed the incubating label Jun 17, 2018

nvaccessAuto added this to the 2018.3 milestone Jun 17, 2018

DrSooom mentioned this pull request Jul 25, 2018

Replace Welcome Dialog with a 6 steps Welcome Wizard #8555

Open

josephsl mentioned this pull request Aug 11, 2018

What's new: beta-level entry changes and clarifications #8618

Merged

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


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

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

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

Conversation

michaelDCurran commented May 1, 2018

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

michaelDCurran commented May 1, 2018

LeonarddeR commented May 1, 2018

LeonarddeR commented May 1, 2018

Brian1Gaff commented May 1, 2018 via email

Brian1Gaff commented May 1, 2018 via email

PratikP1 commented May 1, 2018 via email

bramd commented May 1, 2018

michaelDCurran commented May 1, 2018 via email

Brian1Gaff commented May 2, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelDCurran commented May 7, 2018

feerrenrut commented May 7, 2018

LeonarddeR commented May 7, 2018

michaelDCurran commented May 7, 2018 via email

LeonarddeR commented May 7, 2018

feerrenrut commented May 7, 2018

michaelDCurran commented May 7, 2018 via email

feerrenrut commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelDCurran commented May 14, 2018 via email

feerrenrut commented May 14, 2018

feerrenrut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netblue44 commented May 18, 2018 via email

bramd commented May 18, 2018 via email

netblue44 commented May 18, 2018 via email