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

Fix gui alignment issues #6287

Closed
wants to merge 26 commits into from
Closed

Fix gui alignment issues #6287

wants to merge 26 commits into from

Conversation

feerrenrut
Copy link
Contributor

@feerrenrut feerrenrut commented Aug 22, 2016

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
Copy link
Contributor Author

Images of the changed dialogs:
profile triggers
synthesizer settings
voice settings
braille settings
configurations
dictionary entries
exit
general settings
input gestures
keyboard settings
mouse settings
new profile
object presentation

@jcsteh
Copy link
Contributor

jcsteh commented Aug 24, 2016

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

  1. 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?
  2. 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.
  3. 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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

feerrenrut commented Sep 6, 2016

See #6342 (Spacing between labels and combo boxes)

@feerrenrut
Copy link
Contributor Author

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

@feerrenrut
Copy link
Contributor Author

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

@feerrenrut
Copy link
Contributor Author

feerrenrut commented Sep 6, 2016

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

# 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)
Copy link
Member

Choose a reason for hiding this comment

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

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

name is not needed.

synth=getSynth()
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])
Copy link
Member

Choose a reason for hiding this comment

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

name can be removed

# 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)
Copy link
Member

Choose a reason for hiding this comment

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

name can be removed

# 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)
Copy link
Member

Choose a reason for hiding this comment

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

name can be removed

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)
Copy link
Member

Choose a reason for hiding this comment

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

name can be removed

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
Copy link
Contributor Author

Consider also closing: #5548

@jcsteh
Copy link
Contributor

jcsteh commented Sep 26, 2016

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.

Copy link
Contributor

@jcsteh jcsteh left a comment

Choose a reason for hiding this comment

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

Looks great overall! Thanks!

@@ -298,33 +309,29 @@ def __init__(self, parent):
continue
triggers.append(TriggerInfo(spec, disp, profile))

sizer = wx.BoxSizer(wx.HORIZONTAL)

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Extraneous blank line.

item = self.profileName = wx.TextCtrl(self)
sizer.Add(item)
mainSizer.Add(sizer)
mainSizer.Add(sizer, border=10, flag=wx.ALL)
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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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


import wx

""" Example usage
Copy link
Contributor

Choose a reason for hiding this comment

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

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. :(


import wx

""" Example usage
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

raise NotImplementedError("Use addLabeledControl instead")

if isinstance(toAdd, wx.StaticBoxSizer):
keywordArgs.update({'flag':wx.EXPAND,})
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above re key assignment.

@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
Copy link
Contributor

@jcsteh jcsteh Sep 23, 2016

Choose a reason for hiding this comment

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

: instead of -

sizer.Add(item)
settingsSizer.Add(sizer)
buttonSizer.Add(item)
settingsSizer.Add(buttonSizer)
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 almost certainly missing something, but... why not use a Buttonhelper here?

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"]
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 totally cool with just accepting these as stylistic differences, but I'm curious nevertheless:

  1. 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. :)
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jcsteh jcsteh left a comment

Choose a reason for hiding this comment

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

Looks great!

""" Example usage
Utilities to simplify the creation of wx GUIs, including automatic management of spacing.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

feerrenrut added a commit that referenced this pull request Sep 29, 2016
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)
feerrenrut added a commit that referenced this pull request Oct 13, 2016
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
Copy link
Contributor Author

This has been merged into master with commit: 87ed4d1

@feerrenrut feerrenrut closed this Oct 13, 2016
feerrenrut added a commit that referenced this pull request Oct 13, 2016
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 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
JulienCochuyt added a commit to accessolutions/nvda that referenced this pull request Sep 12, 2019
@feerrenrut feerrenrut deleted the fixGuiAlignmentIssues branch January 20, 2020 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants