Review actions for #6287

feerrenrut · Aug 19, 2016 · Aug 19, 2016 · Aug 19, 2016 · Aug 19, 2016 · Aug 19, 2016
commit ac401a811a69bc7a286494e4563c0dbb1292f8c6
@@ -47,7 +47,7 @@ def __init__(self, parent):
 		item.Selection = self.profileNames.index(config.conf.profiles[-1].name)
 		changeProfilesSizer.Add(item, proportion=1.0)
 
-		changeProfilesSizer.AddSpacer(guiHelper.SPACE_BETWEEN_BUTTONS_VERTICALLY)
+		changeProfilesSizer.AddSpacer(guiHelper.SPACE_BETWEEN_BUTTONS_VERTICAL)
 
 		self.changeStateButton = wx.Button(self)
 		self.changeStateButton.Bind(wx.EVT_BUTTON, self.onChangeState)
@@ -371,29 +371,28 @@ def __init__(self, parent):
 		# Translators: The title of the dialog to create a new configuration profile.
 		super(NewProfileDialog, self).__init__(parent, title=_("New Profile"))
 		mainSizer = wx.BoxSizer(wx.VERTICAL)
+		sHelper = guiHelper.BoxSizerHelper(self, orientation=wx.VERTICAL)
 
 		sizer = wx.BoxSizer(wx.HORIZONTAL)
 		# Translators: The label of a field to enter the name of a new configuration profile.
-		sizer.Add(wx.StaticText(self, label=_("Profile name:")), flag=wx.ALIGN_CENTER_VERTICAL)
-		sizer.AddSpacer(5)
-		item = self.profileName = wx.TextCtrl(self)
-		sizer.Add(item)
-		mainSizer.Add(sizer, border=10, flag=wx.ALL)
+		profileNameText = _("Profile name:")
+		self.profileName = sHelper.addLabeledControl(profileNameText, wx.TextCtrl)
 
 		# Translators: The label of a radio button to specify that a profile will be used for manual activation
 		# in the new configuration profile dialog.
 		self.triggers = triggers = [(None, _("Manual activation"), True)]
 		triggers.extend(parent.getSimpleTriggers())
-		item = self.triggerChoice = wx.RadioBox(self, label=_("Use this profile for:"),
-			choices=[trig[1] for trig in triggers])
-		item.Bind(wx.EVT_RADIOBOX, self.onTriggerChoice)
+		self.triggerChoice = sHelper.addItem(wx.RadioBox(self, label=_("Use this profile for:"),
+			choices=[trig[1] for trig in triggers]))
+		self.triggerChoice.Bind(wx.EVT_RADIOBOX, self.onTriggerChoice)
 		self.autoProfileName = ""
 		self.onTriggerChoice(None)
-		mainSizer.Add(item, border=5, flag=wx.ALL)
 
-		mainSizer.Add(self.CreateButtonSizer(wx.OK | wx.CANCEL), border=5, flag=wx.ALL)
+		sHelper.addItem(self.CreateButtonSizer(wx.OK | wx.CANCEL))
 		self.Bind(wx.EVT_BUTTON, self.onOk, id=wx.ID_OK)
 		self.Bind(wx.EVT_BUTTON, self.onCancel, id=wx.ID_CANCEL)
+
+		mainSizer.Add(sHelper.sizer, border = guiHelper.BORDER_FOR_DIALOGS, flag=wx.ALL)
 		mainSizer.Fit(self)
 		self.Sizer = mainSizer
 		self.profileName.SetFocus()

@@ -1,18 +1,17 @@
 # -*- coding: UTF-8 -*-
 #guiHelper.py
 #A part of NonVisual Desktop Access (NVDA)
-#Copyright (C) 2006-2015 NV Access Limited
+#Copyright (C) 2016 NV Access Limited
 #This file is covered by the GNU General Public License.
 #See the file COPYING for more details.
 
-import wx
-
 """ Example usage
+Utilities to simplify the creation of wx GUIs, including automatic management of spacing.
 
 class myDialog(class wx.Dialog):
 
 	def __init__(self,parent):
-		super(SettingsDialog, self).__init__(parent, wx.ID_ANY, self.title)
+		super(SettingsDialog, self).__init__(parent, title=self.title)
 		dialog = self
 
 		mainSizer=wx.BoxSizer(wx.VERTICAL)
@@ -42,38 +41,40 @@ def __init__(self,parent):
 		self.SetSizer(mainSizer)
 	...
 """
-# border space to be used around all controls in dialogs
+
+import wx
+
+#: border space to be used around all controls in dialogs
 BORDER_FOR_DIALOGS=10
 
-# when dialog items are laid out vertically use this much space between them
+#: when dialog items are laid out vertically use this much space between them
 SPACE_BETWEEN_VERTICAL_DIALOG_ITEMS = 10
 
-# put this much space between buttons  next to each other horizontally.
+#: put this much space between buttons  next to each other horizontally.
 SPACE_BETWEEN_BUTTONS_HORIZONTAL = 7
 
-# put this much space between buttons next to each other vertically
-SPACE_BETWEEN_BUTTONS_VERTICALLY = 5
+#: put this much space between buttons next to each other vertically
+SPACE_BETWEEN_BUTTONS_VERTICAL = 5
 
-# put this much space between two horizontally associated elements (such as a wx.StaticText and a wx.Choice or wx.TextCtrl)
+#: put this much space between two horizontally associated elements (such as a wx.StaticText and a wx.Choice or wx.TextCtrl)
 SPACE_BETWEEN_ASSOCIATED_CONTROL_HORIZONTAL = 10
 
-# put this much space between two vertically associated elements (such as a wx.StaticText and a wx.Choice or wx.TextCtrl)
+#: put this much space between two vertically associated elements (such as a wx.StaticText and a wx.Choice or wx.TextCtrl)
 SPACE_BETWEEN_ASSOCIATED_CONTROL_VERTICAL = 3
 
 class ButtonHelper(object):
 	""" Class used to ensure that the appropriate space is added between each button, whether in horizontal or vertical
-	arrangement. This class should be used for groups of buttons. While it wont cause problems to use this class with a
+	arrangement. This class should be used for groups of buttons. While it won't cause problems to use this class with a
 	single button there is little benefit. Individual buttons can be added directly to a sizer / sizer helper.
 	"""
 	def __init__(self, orientation):
 		"""
 		@param orientation: the orientation for the buttons, either wx.HORIZONTAL or wx.VERTICAL
-		@type itemType: wx.HORIZONTAL or wx.VERTICAL
 		"""
 		object.__init__(self)
 		self._firstButton = True
 		self._sizer = wx.BoxSizer(orientation)
-		self._space = SPACE_BETWEEN_BUTTONS_HORIZONTAL if orientation is wx.HORIZONTAL else SPACE_BETWEEN_BUTTONS_VERTICALLY
+		self._space = SPACE_BETWEEN_BUTTONS_HORIZONTAL if orientation is wx.HORIZONTAL else SPACE_BETWEEN_BUTTONS_VERTICAL
 
 	@property
 	def sizer(self):
@@ -112,9 +113,7 @@ def associateElements( firstElement, secondElement):
 	# staticText and (choice or textCtrl)
 	if isinstance(firstElement, wx.StaticText) and isinstance(secondElement, (wx.Choice, wx.TextCtrl)):
 		sizer = wx.BoxSizer(wx.HORIZONTAL)
-		kwargs = {}
-		kwargs.update( {'flag':wx.ALIGN_CENTER_VERTICAL} )
-		sizer.Add(firstElement, **kwargs)
+		sizer.Add(firstElement, flag=wx.ALIGN_CENTER_VERTICAL)
 		sizer.AddSpacer(SPACE_BETWEEN_ASSOCIATED_CONTROL_HORIZONTAL)
 		sizer.Add(secondElement)
 	# staticText and (ListCtrl, ListBox or TreeCtrl)
@@ -146,11 +145,11 @@ class LabeledControlHelper(object):
 	Relies on guiHelper.associateElements(), any limitations in guiHelper.associateElements() also apply here.
 	"""
 	def __init__(self, parent, labelText, wxCtrlClass, **kwargs):
-		""" @param parent - An instance of the parent wx window. EG wx.Dialog
-			@param labelText - The text to associate with a wx control.
-			@type labelText - string
-			@param wxCtrlClass - The class to associate with the label, eg: wx.TextCtrl
-			@param kwargs - The keyword arguments used to instantiate the wxCtrlClass
+		""" @param parent: An instance of the parent wx window. EG wx.Dialog
+			@param labelText: The text to associate with a wx control.
+			@type labelText: string
+			@param wxCtrlClass: The class to associate with the label, eg: wx.TextCtrl
+			@param kwargs: The keyword arguments used to instantiate the wxCtrlClass
 		"""
 		object.__init__(self)
 		self._label = wx.StaticText(parent, label=labelText)
@@ -168,14 +167,15 @@ def sizer(self):
 class PathSelectionHelper(object):
 	"""
 	Abstracts away details for creating a path selection helper. The path selection helper is a textCtrl with a
-	button in horizontal layout. The Button launches a directory explorer. Its recommended that the 
+	button in horizontal layout. The Button launches a directory explorer. To get the path selected by the user, use the
+	`pathControl` property which exposes a wx.TextCtrl.
 	"""
 	def __init__(self, parent, buttonText, browseForDirectoryTitle):
-		""" @param parent - An instance of the parent wx window. EG wx.Dialog
-			@param buttonText - The text for the button to launch a directory dialog (wx.DirDialog). This is typically 'Browse'
-			@type buttonText - string
-			@param browseForDirectoryTitle - The text for the title of the directory dialog (wx.DirDialog)
-			@type browseForDirectoryTitle - string
+		""" @param parent: An instance of the parent wx window. EG wx.Dialog
+			@param buttonText: The text for the button to launch a directory dialog (wx.DirDialog). This is typically 'Browse'
+			@type buttonText: string
+			@param browseForDirectoryTitle: The text for the title of the directory dialog (wx.DirDialog)
+			@type browseForDirectoryTitle: string
 		"""
 		object.__init__(self)
 		self._textCtrl = wx.TextCtrl(parent)
@@ -193,13 +193,11 @@ def pathControl(self):
 	def sizer(self):
 		return self._sizer
 
-	def getDefulatBrowseForDirectoryPath(self):
+	def getDefaultBrowseForDirectoryPath(self):
 		return self._textCtrl.Value or "c:\\"
 
 	def onBrowseForDirectory(self, evt):
-		# Translators: The title of the dialog presented when browsing for the
-		# destination directory when creating a portable copy of NVDA.
-		startPath = self.getDefulatBrowseForDirectoryPath()
+		startPath = self.getDefaultBrowseForDirectoryPath()
 		with wx.DirDialog(self._parent, self._browseForDirectoryTitle, defaultPath=startPath) as d:
 			if d.ShowModal() == wx.ID_OK:
 				self._textCtrl.Value = d.Path
@@ -209,7 +207,7 @@ class BoxSizerHelper(object):
 	"""
 	def __init__(self, parent, orientation=None, sizer=None):
 		""" Init. Pass in either orientation OR sizer.
-			@param parent - An instance of the parent wx window. EG wx.Dialog
+			@param parent: An instance of the parent wx window. EG wx.Dialog
 			@param orientation: the orientation to use when constructing the sizer, either wx.HORIZONTAL or wx.VERTICAL
 			@type itemType: wx.HORIZONTAL or wx.VERTICAL
 			@param sizer: the sizer to use rather than constructing one.
@@ -225,11 +223,11 @@ def __init__(self, parent, orientation=None, sizer=None):
 		elif sizer and isinstance(sizer, wx.BoxSizer):
 			self.sizer = sizer
 		else:
-			ValueError("Orientation OR Sizer must be supplied.")
+			raise ValueError("Orientation OR Sizer must be supplied.")
 
 	def addItem(self, item):
 		""" Adds an item with space between it and the previous item.
-			Does not handle adding LabledControlHelper, use the convenience method instead.
+			Does not handle adding LabledControlHelper; use L{addlabelledControl} instead.
 		"""
 		toAdd = item
 		keywordArgs = {}
@@ -240,22 +238,20 @@ def addItem(self, item):
 			buttonBorderAmount = 5
 			keywordArgs.update({'border':buttonBorderAmount, 'flag':wx.ALL})
 			shouldAddSpacer = False # no need to add a spacer, since the button border has been added.
-
-		if isinstance(item, BoxSizerHelper):
+		elif isinstance(item, BoxSizerHelper):
 			toAdd = item.sizer
-
-		if isinstance(item, PathSelectionHelper):
+		elif isinstance(item, PathSelectionHelper):
 			toAdd = item.sizer
 			if self.sizer.GetOrientation() == wx.VERTICAL:
-				keywordArgs.update({'flag':wx.EXPAND,})
+				keywordArgs['flag'] = wx.EXPAND
 			else:
 				raise NotImplementedError("Adding PathSelectionHelper to a horizontal BoxSizerHelper is not implemented")
-
-		if isinstance(item, LabeledControlHelper):
+		elif isinstance(item, LabeledControlHelper):
 			raise NotImplementedError("Use addLabeledControl instead")
 
+		# a boxSizerHelper could contain a wx.StaticBoxSizer
 		if isinstance(toAdd, wx.StaticBoxSizer):
-			keywordArgs.update({'flag':wx.EXPAND,})
+			keywordArgs['flag'] = wx.EXPAND
 
 		if shouldAddSpacer:
 			self.sizer.AddSpacer(SPACE_BETWEEN_VERTICAL_DIALOG_ITEMS)
@@ -265,11 +261,11 @@ def addItem(self, item):
 
 	def addLabeledControl(self, labelText, wxCtrlClass, **kwargs):
 		""" Convenience method to create a labeled control
-			@param labelText - Text to use when constructing the wx.StaticText to label the control.
-			@type LabelText - String
-			@param wxCtrlClass - Control class to construct and associate with the label
-			@type wxCtrlClass - Some wx control type EG wx.TextCtrl
-			@param kwargs - keyword arguments used to construct the wxCtrlClass. As taken by guiHelper.LabeledControlHelper
+			@param labelText: Text to use when constructing the wx.StaticText to label the control.
+			@type LabelText: String
+			@param wxCtrlClass: Control class to construct and associate with the label
+			@type wxCtrlClass: Some wx control type EG wx.TextCtrl
+			@param kwargs: keyword arguments used to construct the wxCtrlClass. As taken by guiHelper.LabeledControlHelper
 
 			Relies on guiHelper.LabeledControlHelper and thus guiHelper.associateElements, and therefore inherits any
 			limitations from there.

@@ -1812,21 +1812,22 @@ def makeSettings(self, settingsSizer):
 
 		settingsSizer.AddSpacer(guiHelper.SPACE_BETWEEN_ASSOCIATED_CONTROL_VERTICAL)
 
-		buttonSizer = wx.BoxSizer(wx.HORIZONTAL)
+		bHelper = guiHelper.ButtonHelper(wx.HORIZONTAL)
+
 		# Translators: The label of a button to add a gesture in the Input Gestures dialog.
-		item = self.addButton = wx.Button(self, label=_("&Add"))
-		item.Bind(wx.EVT_BUTTON, self.onAdd)
-		item.Disable()
-		buttonSizer.Add(item)
-		buttonSizer.AddSpacer(guiHelper.SPACE_BETWEEN_BUTTONS_HORIZONTAL)
+		self.addButton = bHelper.addButton(self, label=_("&Add"))
+		self.addButton.Bind(wx.EVT_BUTTON, self.onAdd)
+		self.addButton.Disable()
+
 		# Translators: The label of a button to remove a gesture in the Input Gestures dialog.
-		item = self.removeButton = wx.Button(self, label=_("&Remove"))
-		item.Bind(wx.EVT_BUTTON, self.onRemove)
-		item.Disable()
+		self.removeButton = bHelper.addButton(self, label=_("&Remove"))
+		self.removeButton.Bind(wx.EVT_BUTTON, self.onRemove)
+		self.removeButton.Disable()
+
 		self.pendingAdds = set()
 		self.pendingRemoves = set()
-		buttonSizer.Add(item)
-		settingsSizer.Add(buttonSizer)
+
+		settingsSizer.Add(bHelper.sizer)
 
 	def postInit(self):
 		self.tree.SetFocus()