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
base: master
from

Conversation

Projects
None yet
5 participants
@feerrenrut
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Aug 22, 2016

Contributor

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

Contributor

feerrenrut commented Aug 22, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Aug 24, 2016

Contributor

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?

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

This comment has been minimized.

Show comment
Hide comment
@nishimotz

nishimotz Aug 24, 2016

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.

Contributor

nishimotz commented Aug 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Aug 30, 2016

Contributor

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

Contributor

feerrenrut commented Aug 30, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Sep 6, 2016

Contributor

See #6342 (Spacing between labels and combo boxes)

Contributor

feerrenrut commented Sep 6, 2016

See #6342 (Spacing between labels and combo boxes)

@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Sep 6, 2016

Contributor

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

Contributor

feerrenrut commented Sep 6, 2016

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

@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Sep 6, 2016

Contributor

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

Contributor

feerrenrut commented Sep 6, 2016

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

@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Sep 6, 2016

Contributor

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
Contributor

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

@feerrenrut feerrenrut added the NVDA GUI label Sep 6, 2016

Show outdated Hide outdated source/gui/settingsDialogs.py Outdated
Show outdated Hide outdated source/gui/settingsDialogs.py Outdated
Show outdated Hide outdated source/gui/settingsDialogs.py Outdated
Show outdated Hide outdated source/gui/settingsDialogs.py Outdated
Show outdated Hide outdated source/gui/settingsDialogs.py Outdated
Show outdated Hide outdated source/gui/settingsDialogs.py Outdated

feerrenrut added some commits Sep 14, 2016

Fix some errors and remove 'name' parameter
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
Label for addons disabled was missed in clean up. This was not easy to
notice since it is conditionally shown.
fix alignment on several dialogs
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.
Update some remainig dialogs
Fixed up the:
- launcher
- installer dialog
- update check result
minor fixes
Updates to the user guide, and a the symbol/pronunciation add symbol
dialog.
@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Sep 23, 2016

Contributor

Consider also closing: #5548

Contributor

feerrenrut commented Sep 23, 2016

Consider also closing: #5548

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Sep 26, 2016

Contributor

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.

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.

@jcsteh

Looks great overall! Thanks!

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

This comment has been minimized.

@jcsteh

jcsteh Sep 26, 2016

Contributor

nit: Extraneous blank line.

@jcsteh

jcsteh Sep 26, 2016

Contributor

nit: Extraneous blank line.

Show outdated Hide outdated source/gui/configProfiles.py Outdated
Show outdated Hide outdated source/gui/guiHelper.py Outdated
Show outdated Hide outdated source/gui/guiHelper.py Outdated
Show outdated Hide outdated source/gui/guiHelper.py Outdated
Show outdated Hide outdated source/gui/guiHelper.py Outdated
Show outdated Hide outdated source/gui/guiHelper.py Outdated
Show outdated Hide outdated source/gui/guiHelper.py Outdated
Show outdated Hide outdated source/gui/settingsDialogs.py Outdated
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"]

This comment has been minimized.

@jcsteh

jcsteh Sep 26, 2016

Contributor

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.

@jcsteh

jcsteh Sep 26, 2016

Contributor

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.

This comment has been minimized.

@feerrenrut

feerrenrut Sep 27, 2016

Contributor

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.

@feerrenrut

feerrenrut Sep 27, 2016

Contributor

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

jcsteh approved these changes Sep 27, 2016

Looks great!

Show outdated Hide outdated source/gui/guiHelper.py Outdated

feerrenrut added a commit that referenced this pull request Sep 29, 2016

incubates #6287
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

Delivers a set of gui changes
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

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Oct 13, 2016

Contributor

This has been merged into master with commit: 87ed4d1

Contributor

feerrenrut commented Oct 13, 2016

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

Update changes file for PR #6287, #6402, #6339
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment