Fix gui alignment issues by feerrenrut · Pull Request #6287 · nvaccess/nvda

feerrenrut · 2016-08-22T08:20:54Z

Fixed alignment and made small modifications to several nvda dialogs.

Also fixes:
#6317 (Gestures treeview does not look like a treeview)
#5548 (Text control to specify the path when creating a portable copy is quite narrow)
#6342 (Spacing between labels and combo boxes)
#6343 (Inconsistency with vertical spacing between checkbox/label options)
#6349 (input gestures dialog tree view too small)

feerrenrut · 2016-08-22T08:22:57Z

Images of the changed dialogs:

jcsteh · 2016-08-24T03:16:31Z

Thanks for the fixes! :) First, a few specific concerns:

Configuration Profiles dialog: I see you've "grouped" the list and the buttons for manipulating profiles using a StaticBoxSizer. I sort of follow this, since Triggers are separate from that list. The problem is that this gets reported as a grouping for screen reader users, which is IMO extraneous verbosity here. Groupings certainly make sense when it needs to be made clear that a set of options are in a group, but in this case, everything is really related to the same thing: profiles. Perhaps it might be helpful if you can explain how this "looks better" visually. Does it make something clear that wasn't clear before?
Input Gestures dialog: All of the controls except the OK and Cancel buttons are now inside a "Gestures" box (exposed to screen reader users as a grouping). Again, can you explain how this "looks" better visually? There's nothing else in the dialog, so I'm just trying to follow the need to group things here. Again, my concern is that there is some extraneous verbosity here for screen reader users.
dropDownLabelBorder and dropDownLabelFlags get defined in quite a few places. If these are the border/flags we need to use for drop-downs everywhere, I think these should be moved into the SettingsDialog class as class constants. That way, they can be reused.

Beyond this (and perhaps something that needs to be addressed separately), a major concern for me here is that as a blind developer, I'm struggling to understand the "rules" we need to use to make the GUI look decent. Obviously, we're always going to need some help to "pretty things up", but it'd be good if we can avoid making the same mistakes over and over. Perhaps we need to develop a set of guidelines as to what borders/spacing/flags/proportions to use and when to use them, etc. As another example, I don't follow when we should use a border and when we should add a "spacer". (I didn't even know about spaces before today! :)) I wonder how much of this we can somehow hide behind code. Perhaps we could create our own helper methods or sizer subclasses or the like.

Thoughts?

nishimotz · 2016-08-24T09:12:53Z

Regarding dropDownLabelBorder and dropDownLabelFlags:
When wx.StaticText and wx.Choice are used horizontally together like those cases, this work makes it nicer visually.
In such cases, the static text should be used as the label of the following combobox.
I am in favor of refactoring SettingDialog class regarding this combination, because it is semantically meaningful.

feerrenrut · 2016-08-30T09:26:41Z

@jcsteh As per our conversation I have pulled the common patterns out into a helper, and removed one of the group boxes. The other I have left in place (since it helps to visually associate the items) but removed the label. The configProfiles code is harder to make generic.

Could you please take another look when you get the chance?

feerrenrut · 2016-09-06T10:45:07Z

See #6342 (Spacing between labels and combo boxes)

feerrenrut · 2016-09-06T10:46:16Z

See #6343 (Inconsistency with vertical spacing between checkbox/label options)

feerrenrut · 2016-09-06T10:51:50Z

See #6349 (input gestures dialog tree view too small)

feerrenrut · 2016-09-06T10:53:50Z

Feedback on GUI, received via email, with respect to the create portable nvda dialog

this dialog seems to have no real padding around any of the items and the edges of the dialog. we need to make sure we have a consistent padding around the items in the dialog like with all other major dialogs. The issue of spacing exists here too in regards to the path edit field and the browse button. there needs to be more space between them like in the combo-box / text label issue already discussed. the continue/cancel also needs to be padded out (similar to the ok/cancel issue already discussed) and again, right aligned.

Also the text along the top does wrap in the dialog making the layout seem messy. maybe a re-wording would solve this.
e.g.:
Select a folder for the portable copy of NVDA
[then you have under this the edit field and browse button]
Use the current configuration for the portable copy (which is a label with a checkbox to its left as per normal)
Continue/Cancel (right aligned)

With respect to the "check for update dialog and resultant download dialog"

Like the create portable nvda dialog, the spacing is not correct. nothing seems to be spaced or padded here at all, and so it is not consistent with other dialogs. The download and install and close buttons are aligned one under the other, but really they should be placed with download and update to the left and Cancel to the right, and right aligned at the bottom of the dialog much like the ok/cancel paradigm that i have been discussing. (note i suggest cancel instead of close here).

Fix up the portable nvda dialog
Fix up the update dialog, and resultant download dialog
Look for other dialogs that may have been missed

michaelDCurran · 2016-09-15T03:18:47Z

 		# Translators: The list of languages for NVDA.
-		self.languageList=wx.Choice(self,languageListID,name=_("Language"),choices=[x[1] for x in self.languageNames])
+		languageCtrlName = _("Language")
+		self.languageList=settingsSizerHelper.addLabeledControl(languageLabelText, wx.Choice, name=languageCtrlName, choices=languageChoices)


the name=languageControlName for wx.Choice is not needed (assuming it is never shown visually).

michaelDCurran · 2016-09-15T03:21:53Z

-		self.logLevelList=wx.Choice(self,logLevelListID,name=_("Log level"),choices=[name for level, name in self.LOG_LEVELS])
+		logLevelChoicesName=_("Log level")
+		logLevelChoices = [name for level, name in self.LOG_LEVELS]
+		self.logLevelList = settingsSizerHelper.addLabeledControl(logLevelLabelText, wx.Choice, name=logLevelChoicesName, choices=logLevelChoices)


name is not needed.

michaelDCurran · 2016-09-15T03:26:18Z

 		setattr(self,"_%ss"%setting.name,getattr(synth,"available%ss"%setting.name.capitalize()).values())
 		l=getattr(self,"_%ss"%setting.name)###
-		lCombo=wx.Choice(self,wx.ID_ANY,name="%s:"%setting.i18nName,choices=[x.name for x in l])
+		labeledControl=guiHelper.LabeledControlHelper(self, labelText, wx.Choice, name="%s:"%setting.i18nName, choices=[x.name for x in l])


name can be removed

michaelDCurran · 2016-09-15T03:27:14Z

-		# Translators: This is the name of a combobox in the keyboard settings dialog.
-		self.kbdList=wx.Choice(self,kbdListID,name=_("Keyboard layout"),choices=[layouts[layout] for layout in self.kbdNames])
+		kbdChoices = [layouts[layout] for layout in self.kbdNames]
+		self.kbdList=sHelper.addLabeledControl(kbdLabelText, wx.Choice, name=kbdChoicesName, choices=kbdChoices)


name can be removed

michaelDCurran · 2016-09-15T03:27:50Z

-		# Translators: This is the name of a combobox in the mouse settings dialog.
-		self.textUnitComboBox=wx.Choice(self,wx.ID_ANY,name=_("text reporting unit"),choices=[textInfos.unitLabels[x] for x in self.textUnits])
+		textUnitsChoices = [textInfos.unitLabels[x] for x in self.textUnits]
+		self.textUnitComboBox=sHelper.addLabeledControl(textUnitLabelText, wx.Choice, name=textUnitsName, choices=textUnitsChoices)


name can be removed

michaelDCurran · 2016-09-15T03:28:26Z

-		self.progressList=wx.Choice(self,progressListID,name=_("Progress bar output"),choices=[name for setting, name in self.progressLabels])
+		progressName=_("Progress bar output")
+		progressChoices = [name for setting, name in self.progressLabels]
+		self.progressList=sHelper.addLabeledControl(progressLabelText, wx.Choice,name=progressName,choices=progressChoices)


name can be removed

To resolve visual ambiguity between a listview and a treeview, show plus/minus symbols

Some wx controls were being created using the 'name' parameter. When there is a label this is unnecessary, since the label will describe the controls. It may also be unnecessary when there is sufficient context provided by the dialog itself.

Label for addons disabled was missed in clean up. This was not easy to notice since it is conditionally shown.

Alignments fixed on symbol/pronunciation dialog. Fixed alignment on 'make portable copy' dialog Groups of controls (wx.StaticBoxSizer) added to a VERTICAL boxSizerHelper are now automatically expanded to the full width. I think this is a more common pattern. Directory entry concept has been abstracted.

Fixed up the: - launcher - installer dialog - update check result

Updates to the user guide, and a the symbol/pronunciation add symbol dialog.

feerrenrut · 2016-09-23T06:06:48Z

Consider also closing: #5548

jcsteh · 2016-09-26T00:09:59Z

nit: There are quite a few added blank lines (particularly in gui/settingsDialogs.py) that have tabs on them. I'm not too worried about this, but if there's an easy way for you to remove the tabs, please do. A regular expression like ^\w+$ should catch this.

jcsteh

Looks great overall! Thanks!

jcsteh · 2016-09-23T06:42:10Z

 			triggers.append(TriggerInfo(spec, disp, profile))

-		sizer = wx.BoxSizer(wx.HORIZONTAL)
+


nit: Extraneous blank line.

jcsteh · 2016-09-23T06:48:02Z

 		item = self.profileName = wx.TextCtrl(self)
 		sizer.Add(item)
-		mainSizer.Add(sizer)
+		mainSizer.Add(sizer, border=10, flag=wx.ALL)


Can you explain why this particular case isn't like a labelled control in most other places? I do see it's different; just curious as to why.

It looks like I missed this one when converting to the guiHelper. I have updated this.

jcsteh · 2016-09-23T06:49:20Z

+# -*- coding: UTF-8 -*-
+#guiHelper.py
+#A part of NonVisual Desktop Access (NVDA)
+#Copyright (C) 2006-2015 NV Access Limited


This can just say 2016 (rather than 2006-2015).

jcsteh · 2016-09-23T06:50:12Z

+
+import wx
+
+""" Example usage


The docstring needs to go above any imports. Otherwise, it won't be treated as a module docstring. Unfortunately, we have this problem in a few of our other modules too. :(

jcsteh · 2016-09-23T06:51:57Z

+
+import wx
+
+""" Example usage


Maybe we could have a brief sentence at the top here explaining what this is for? Something like:

Utilities to simplify the creation of wx GUIs, including automatic management of spacing.

jcsteh · 2016-09-23T11:40:11Z

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


Rather than saying "the convenience method", I think it'd be better to use the method's name. In fact, epydoc let's you link to an identifier like so:

Does not handle adding LabledControlHelper; use L{addlabelledControl} instead.

jcsteh · 2016-09-23T11:40:47Z

+			raise NotImplementedError("Use addLabeledControl instead")
+
+		if isinstance(toAdd, wx.StaticBoxSizer):
+			keywordArgs.update({'flag':wx.EXPAND,})


Same as above re key assignment.

jcsteh · 2016-09-23T11:41:26Z

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


: instead of -

jcsteh · 2016-09-25T23:25:07Z

-		sizer.Add(item)
-		settingsSizer.Add(sizer)
+		buttonSizer.Add(item)
+		settingsSizer.Add(buttonSizer)


I'm almost certainly missing something, but... why not use a Buttonhelper here?

jcsteh · 2016-09-26T00:41:28Z

-		settingsSizer.Add(item, border=10, flag=wx.BOTTOM)
+		readByParagraphText = _("Read by &paragraph")
+		self.readByParagraphCheckBox = sHelper.addItem(wx.CheckBox(self, label=readByParagraphText))
+		self.readByParagraphCheckBox.Value = config.conf["braille"]["readByParagraph"]


I'm totally cool with just accepting these as stylistic differences, but I'm curious nevertheless:

You've changed most places to assign label text to a variable, rather than just specifying the label in the call. Why? I guess this makes the lines shorter, but you could wrap to a new line. This approach seems more "indirect" to me. It also creates a lot more local variables, but that's inconsequential; premature optimisation and all. :)

Here (and in quite a few other places), I had a variable like item which I just assign temporarily. The reason is largely to make the code less verbose and thus easier to type. You've changed these to either use a new local or, if there was an attribute on self, to use that. Why? Was the use of item confusing to you; i.e. did you feel you weren't sure which control I was referring to despite the grouping of the code? One other reason I did this was to avoid hash lookups on self, but again, inconsequential; premature optimisation and all. :)

Again, not asking for changes here; just curious on your reasoning.

For 1: yes, to make the lines shorter. But also to try to better pair the text with the translator comment.

For 2: Using the named variable rather than item makes the code easier to read and understand. When item is used, and then a few lines down something is being done to it, you have to remember (or go look for) the context. With the named variable, (hopefully) it is fairly self explanatory what you are dealing with.

jcsteh

Looks great!

jcsteh · 2016-09-27T06:38:08Z

-import wx
-
 """ Example usage
+Utilities to simplify the creation of wx GUIs, including automatic management of spacing.


These two lines should probably be reversed; i.e. so "Example usage" comes below the summary.

For issues: #6317 (Gestures treeview does not look like a treeview) #5548 (Text control to specify the path when creating a portable copy is quite narrow) #6342 (Spacing between labels and combo boxes) #6343 (Inconsistency with vertical spacing between checkbox/label options) #6349 (input gestures dialog tree view too small) Merge branch 'fixGuiAlignmentIssues' into next Conflicts: source/gui/settingsDialogs.py - voice settings (speak numbers as) - SpeechSymbolsDialog (mouse interaction)

Merges in branches - 'i6101_SymbolsListCtrl' - 'i6348-documentFormattingDialog' - 'fixGuiAlignmentIssues' Adjust the symbols list. See PR #6339 Put the 'symbol' label above the list and have the list itself take up the full width of the dialog. Columns are spaced automatically, with one nominated to take up any extra available room. The first column was selected to expand more, as the information in the other columns can be viewed in the "change symbol" group. Fixes #6101 Restructure the document formatting dialog. See PR #6402 The document formatting settings dialog is restructured so that items can no longer "disappear" off the screen. The options are now in a scrolling section, grouped for clarity (paritcularly for new users / sighted users). A dialog description has been added, for further clarity. Fixes #6348 Fix Gui Alignment issues. See PR #6287 Fixes up various alignment and padding problems in the GUI. Introduces a new helper to abstract some of these details. Fixes #6317 (Gestures treeview does not look like a treeview) Fixes #5548 (Text control to specify the path when creating a portable copy is quite narrow) Fixes #6342 (Spacing between labels and combo boxes) Fixes #6343 (Inconsistency with vertical spacing between checkbox/label options) Fixes #6349 (input gestures dialog tree view too small)

feerrenrut · 2016-10-13T07:18:45Z

This has been merged into master with commit: 87ed4d1

For PR #6287 - Various padding and alignment issues have been resolved. (#6317, #5548, #6342, #6343, #6349) For PR #6402 - The document formatting dialog has been adjusted so that the contents scrolls. (#6348) For PR #6339 - Adjusted the layout of the symbols pronunciation dialog so the full width of the dialog is used for the symbols list. (#6101)

feerrenrut assigned feerrenrut and jcsteh Aug 22, 2016

feerrenrut added the component/NVDA-GUI label Sep 6, 2016

feerrenrut force-pushed the fixGuiAlignmentIssues branch from de4854a to 2af53e4 Compare September 14, 2016 07:25

michaelDCurran requested changes Sep 15, 2016

View reviewed changes

michaelDCurran approved these changes Sep 19, 2016

View reviewed changes

feerrenrut added 16 commits September 19, 2016 13:01

fix alignment of the exit dialog

1e232cf

fix general settings alignments

825864f

fixed alignment in the synthesizer dialog

e13351d

fix alignment on voice settings dialog

81cd171

fix alignment on keyboard and mouse settings dialogs

c3f2a35

fix alignment on braille settings dialogs

fa3384c

fix alignment on the object presentation settings dialogs

82b7dbf

fix alignment on the dictionary dialog

1ba26ac

fix alignment on the input gestures dialog

dfacc70

fix alignment on the configuration profiles dialog

e710892

fix alignment on the new config profile dialog

c64f213

fix alignment on the profile triggers dialog

173879f

Pull out common gui patterns into a helper module.

531aaa9

Show symbols on tree view

0cb617b

To resolve visual ambiguity between a listview and a treeview, show plus/minus symbols

fixed up alignment and spacing on the elements list dialog

b3a05c4

WIP

4b8ab58

feerrenrut added 7 commits September 19, 2016 13:04

Moved more of the code over to the class based approach

899358e

Updated comments for the guiHelper

20c88e0

Fix some errors and remove 'name' parameter

108ee95

Some wx controls were being created using the 'name' parameter. When there is a label this is unnecessary, since the label will describe the controls. It may also be unnecessary when there is sufficient context provided by the dialog itself.

Fixed up missed part of gui

8e93c71

Label for addons disabled was missed in clean up. This was not easy to notice since it is conditionally shown.

Update some remainig dialogs

4b52d5d

Fixed up the: - launcher - installer dialog - update check result

minor fixes

86548ae

Updates to the user guide, and a the symbol/pronunciation add symbol dialog.

feerrenrut force-pushed the fixGuiAlignmentIssues branch from 2e413fd to 86548ae Compare September 19, 2016 05:06

jcsteh requested changes Sep 26, 2016

View reviewed changes

jcsteh mentioned this pull request Sep 26, 2016

Document formatting dialog (issue #6348) #6402

Closed

Review actions for #6287

ac401a8

jcsteh approved these changes Sep 27, 2016

View reviewed changes

Fixed up an out of order comment for #6287

a262092

nvaccessAuto added the incubating label Sep 29, 2016

nvaccessAuto unassigned jcsteh Sep 29, 2016

feerrenrut closed this Oct 13, 2016

feerrenrut added this to the 2016.4 milestone Oct 13, 2016

JulienCochuyt added a commit to accessolutions/nvda that referenced this pull request Sep 11, 2019

Fix up of: Fix gui alignment issues (nvaccess#6287)

428dfc1

JulienCochuyt mentioned this pull request Sep 11, 2019

Fix up of: Fix gui alignment issues (#6287) - typo #10199

Merged

JulienCochuyt added a commit to accessolutions/nvda that referenced this pull request Sep 12, 2019

Fix up of: Fix gui alignment issues (nvaccess#6287)

0d2f1ff

JulienCochuyt mentioned this pull request Sep 12, 2019

Fix up of: Fix gui alignment issues (#6287) - addItem keywords #10205

Merged

michaelDCurran pushed a commit that referenced this pull request Sep 13, 2019

Fix up of: Fix gui alignment issues (#6287) (#10205)

1e906fb

michaelDCurran pushed a commit that referenced this pull request Sep 13, 2019

Fix up of: Fix gui alignment issues (#6287) (#10199)

b9c81a5

feerrenrut deleted the fixGuiAlignmentIssues branch January 20, 2020 09:19

		triggers.append(TriggerInfo(spec, disp, profile))

		sizer = wx.BoxSizer(wx.HORIZONTAL)

Uh oh!

Conversation

feerrenrut commented Aug 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feerrenrut commented Aug 22, 2016

Uh oh!

jcsteh commented Aug 24, 2016

Uh oh!

nishimotz commented Aug 24, 2016

Uh oh!

feerrenrut commented Aug 30, 2016

Uh oh!

feerrenrut commented Sep 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feerrenrut commented Sep 6, 2016

Uh oh!

feerrenrut commented Sep 6, 2016

Uh oh!

feerrenrut commented Sep 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feerrenrut commented Sep 23, 2016

Uh oh!

jcsteh commented Sep 26, 2016

Uh oh!

jcsteh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcsteh Sep 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcsteh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feerrenrut commented Oct 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

feerrenrut commented Aug 22, 2016 •

edited

Loading

feerrenrut commented Sep 6, 2016 •

edited

Loading

feerrenrut commented Sep 6, 2016 •

edited

Loading

jcsteh Sep 23, 2016 •

edited

Loading