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

Redesign main frame #365

Merged
merged 2 commits into from Feb 22, 2016
Merged

Redesign main frame #365

merged 2 commits into from Feb 22, 2016

Conversation

morinted
Copy link
Member

  • Remove bitmaps for status indicator, as part of the effort for Plover could use more art #263.
  • Windows has a grey color for the main frame because that is Windows' default color. To fix this, you place a top-level panel in the frame.
  • Add a disabled state for output status when a machine is disconnected.
  • Remove status from title-bar.
  • Section off the user interface for clarity, needed especially because of the status indicator.
  • Show status with radio buttons.

Here is the redesign:

screen shot 2016-02-20 at 6 08 22 pm

screen shot 2016-02-20 at 6 08 25 pm

screen shot 2016-02-20 at 6 08 30 pm

@morinted
Copy link
Member Author

The branch check failed on AppVeyor while uploading the artifact, but the second build succeeded, so ignore the failed build please.

@@ -91,19 +92,20 @@ def __init__(self, config):
self.config = config

# Note: don't set position from config, since it's not yet loaded.
wx.Frame.__init__(self, None, title=self.TITLE,
mainframe = wx.Frame.__init__(self, None, title=self.TITLE,
Copy link
Member

Choose a reason for hiding this comment

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

No need for this.

@morinted
Copy link
Member Author

Instead of "stopped" we could change to Mirabai's suggestion of "disabled". Alternatively, I've used "suspended" and "paused" to describe Plover's not-running state.

@benoit-pierre
Copy link
Member

The problem is: it is still running, listening to keys, if you use a machine, the port is still in use. I think: "output enabled"/"output disabled" is the better terminology.

@morinted
Copy link
Member Author

Yes, I can agree with that. I may also rename, in this PR, "error" to "disconnected", since that's when the error message is used.

@benoit-pierre
Copy link
Member

What about in case of initialization error?

@morinted
Copy link
Member Author

All right, I updated.

Removing the window title, it becomes clear that we'll need a secondary label. I'm all for folding in my checkbox change into this PR, with our updated terminology.

So, we could do like the checkbox in the art thread:

  • Enable Output
  • [-] Output Enabled

Or perhaps a label & button?

Output Enabled
[ ⏸Disable ]

----

Output Disabled
[ ▶️ Enable ]

Label and a button, short form?

Output Enabled [ ⏸ ]

----

Output Disabled [ ▶️ ]

For the icons, we should just render some bitmaps from some open icon repository, as Unicode support differs depending on the user's system font. The first two that come to mind are Google's Material Icons and Ionic's Ionicons.

@jeremy-w
Copy link
Member

Google's Material icons don't seem to have a standard play/pause, just stuff for snoozing notifications and pausing phone calls. The Ionicons look great. (We should do the notice & terms requirement of MIT code in the About dialog somewhere, probably.)

@stenoknight
Copy link
Member

This might seem arbitrary, but I have an aesthetic dislike of using
play/pause icons for anything other than actual audio playback. I just feel
like something accepting input and producing output isn't best represented
by a schema like play/pause. Just my opinion.
On Feb 16, 2016 9:18 AM, "Ted Morin" notifications@github.com wrote:

All right, I updated.

Removing the window title, it becomes clear that we'll need a secondary
label. I'm all for folding in my checkbox change into this PR, with our
updated terminology.

So, we could do like the checkbox in the art thread:

  • Enable Output
  • [*] Output Enabled

Or perhaps a label & button?

Output Enabled
[ ⏸Disable ]


Output Disabled
[ ▶️ Enable ]

Label and a button, short form?

Output Enabled [ ⏸ ]


Output Disabled [ ▶️ ]

For the icons, we should just render some bitmaps from some open icon
repository, as Unicode support differs depending on the user's system font.
The first two that come to mind are Google's Material Icons
https://design.google.com/icons/ and Ionic's Ionicons
http://ionicons.com/.


Reply to this email directly or view it on GitHub
#365 (comment)
.

@benoit-pierre
Copy link
Member

Damn, miss-click... Sorry about that.

How about something like this:

2016-02-16-164922_204x67_scrot

@benoit-pierre benoit-pierre reopened this Feb 16, 2016
@morinted
Copy link
Member Author

If we added a group, we'd have to group the other elements (machine status and configure/about buttons) as well. I'd like to try:

Output Enabled    [ Disable ]

Output Disabled   [ Enable ]

@jeremy-w
Copy link
Member

Could we split out the background color change from the "what buttons do we want" bit that's still under discussion? I'm all for getting the better background color in, that's an uncontroversial win AFAICT.

@morinted morinted force-pushed the windows-grey branch 2 times, most recently from 4ce4622 to ac4f1ee Compare February 18, 2016 04:05
@morinted morinted changed the title Fix Plover's grey color on Windows, reorganize status button Redesign main frame Feb 18, 2016
@morinted
Copy link
Member Author

@benoit-pierre @jeremy-w

I folded in my status button work into this PR. If I could get your feedback on functionality and code style, then we can get this done in time for the release.

@jeremy-w
Copy link
Member

I like the screenshots in the description. Far clearer than what we have now.

@Achim63
Copy link

Achim63 commented Feb 18, 2016

I beg to differ - I don't like that enabled-disable disabled-enable button at all. Since it's a simple switch, a button is simply the wrong UI element here. It should either be two mutually exclusive radio buttons, a checkbox, or (my preference) an on/off slider - something like this (found in Google pictures, so maybe copyrighted), maybe with the string "Output" next to it:
08-mobile-on-off-toggle-switch-psd

@morinted
Copy link
Member Author

@Achim63 you can read through the art exchange if you'd like. There are various reasons for why we ended up on this design and I'm afraid that ultimately not everyone is going to be happy.

  • The toggle buttons like you are showing, I've looked at them for hours, and they don't make sense if you can't drag them → which you can't with the bitmap buttons that wxPython has. So, knowing that you can't swipe, what's another toggle button?
  • The other issue with a bitmap is the need to manage retina display on a Mac, but let's ignore that hurdle for the sake of argument.
  • So, I started to look at toggle buttons that don't need sliding. Lots of classical "switches", with on/off on each end. That's where I got my two-button switch, where the active button gets "pushed-in". That one had some issues with general aesthetic, and it took up a lot of vertical space.
  • The other discussion was terminology. We discussed what sort of words can be used to describe Plover's output state. Previously it was "running" and "stopped", which isn't true because Plover is always running, listening for toggle strokes and such. Therefore, we found that "output enabled/disabled" most succinctly described what Plover was doing.
  • The title bar with the status in it was annoying on Windows because no matter what it was always cut off, so that was removed along the way in favor of a more verbose user interface in the main configuration dialogue.
  • That leaves us with the need to convey status as well as convey what clicking a button will do. While not super-pretty or revolutionary or what-have-you, the grouped solution allows us to clearly section off different pieces of Plover's main window. The label states Plover's output state, using unambiguous words (enabled, disabled, disconnected → all adjectives) and the button labels its own action (enable, disable → verbs).
  • We also tried a check box, but the problem is indicating what clicking the check box will do.

Finally, for the toggle button, I really recommend reading through this stack overflow response: http://ux.stackexchange.com/a/40954

As always, though, the code is iterative. If the feedback is overwhelming when we release a preview build, we can always change the user interface later.

@morinted
Copy link
Member Author

I haven't tried radio buttons, though, that might be worth a try.
On Feb 18, 2016 9:34 AM, "Achim" notifications@github.com wrote:

I beg to differ - I don't like that enabled-disable disabled-enable button
at all. Since it's a simple switch, a button is simply the wrong UI element
here. It should either be two mutually exclusive radio buttons, a checkbox,
or (my preference) an on/off slider - something like this (found in Google
pictures, so maybe copyrighted):
[image: 08-mobile-on-off-toggle-switch-psd]
https://cloud.githubusercontent.com/assets/10847639/13146217/f4fa0d3c-d654-11e5-8856-8794cfbfea8a.png


Reply to this email directly or view it on GitHub
#365 (comment)
.

@jeremy-w
Copy link
Member

I was in favor of giving radio buttons a shot, but I like the visual symmetry with all normal buttons now that I see that.

@Achim63 A lot of UI stuff is hamstrung by using the lowest-common denominator of what wxWidgets has to offer via wxPython. It's frustrating.

RUNNING_MESSAGE = "running"
STOPPED_MESSAGE = "stopped"
ERROR_MESSAGE = "error"
STATUS_DISCONNECTED, STATUS_OUTPUT_ENABLED, STATUS_OUTPUT_DISABLED = range(3)
Copy link
Member

Choose a reason for hiding this comment

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

I'd expect these to be disconnected => -1, disabled => 0, enabled => 1, but since this isn't exposed outside the app yet, no big deal.

@morinted
Copy link
Member Author

@benoit-pierre reports that this renders very badly on Ubuntu, so it will require additional work before we can merge it.

@jeremy-w
Copy link
Member

@benoit-pierre Could you attach a screenshot of this under Ubuntu?

@jeremy-w jeremy-w closed this Feb 19, 2016
@jeremy-w jeremy-w reopened this Feb 19, 2016
@jeremy-w
Copy link
Member

TFW you think "not the default button, must be Cancel" and instead close a PR and leave the comment you were trying to cancel.

@benoit-pierre
Copy link
Member

So the issue if not as clear cut as "it never looks good on Ubuntu", this only happen when I use a configuration with only one small dictionary (for faster startup). It otherwise looks fine, except for the radio buttons that are a bit cramped (vertically) with my theme on Arch Linux:

2016-02-19-205023_242x125_scrot

But this is a minor issue, so let's merge this.

@morinted
Copy link
Member Author

@benoit-pierre I rejigged the UI slightly to make it look good on Windows, I'll have to confirm Mac looks all right, does this also fix the crampedness on your machine?

- Windows has a grey color for the main frame because that is Windows' default color. To fix this, you place a top-level panel in the frame.
- Add a disabled state for the status button when a machine is disconnected.
- Remove title status
@benoit-pierre
Copy link
Member

@morinted: yes.

@benoit-pierre
Copy link
Member

With those additional changes, the layout is always correct, no matter the configuration, and whether I start with the keyboard and switch to another machine or vice versa:

diff --git i/plover/gui/main.py w/plover/gui/main.py
index cd968a2..4afb9bb 100644
--- i/plover/gui/main.py
+++ w/plover/gui/main.py
@@ -99,34 +99,34 @@ class MainFrame(wx.Frame):
               style=wx.DEFAULT_FRAME_STYLE & ~(wx.RESIZE_BORDER |
                                wx.RESIZE_BOX |
                                wx.MAXIMIZE_BOX))
-    self.root = wx.Panel(self, style=wx.DEFAULT_FRAME_STYLE)
+    root = wx.Panel(self, style=wx.DEFAULT_FRAME_STYLE)

     # Menu Bar
     MenuBar = wx.MenuBar()
     self.SetMenuBar(MenuBar)

     # Configure button.
-    self.configure_button = wx.Button(self.root,
+    self.configure_button = wx.Button(root,
                       label=self.CONFIGURE_BUTTON_LABEL)
     self.configure_button.Bind(wx.EVT_BUTTON, self._show_config_dialog)

     # About button.
-    self.about_button = wx.Button(self.root, label=self.ABOUT_BUTTON_LABEL)
+    self.about_button = wx.Button(root, label=self.ABOUT_BUTTON_LABEL)
     self.about_button.Bind(wx.EVT_BUTTON, self._show_about_dialog)

     # Status radio buttons.
-    self.radio_output_enable = wx.RadioButton(self.root,
+    self.radio_output_enable = wx.RadioButton(root,
                           label=self.ENABLE_OUTPUT_LABEL)
     self.radio_output_enable.Bind(wx.EVT_RADIOBUTTON,
                       lambda e: self.steno_engine.set_is_running(True))
-    self.radio_output_disable = wx.RadioButton(self.root,
+    self.radio_output_disable = wx.RadioButton(root,
                            label=self.DISABLE_OUTPUT_LABEL)
     self.radio_output_disable.Bind(wx.EVT_RADIOBUTTON,
                    lambda e: self.steno_engine.set_is_running(False))

     # Machine status.
     # TODO: Figure out why spinner has darker gray background.
-    self.spinner = wx.animate.GIFAnimationCtrl(self.root, -1, SPINNER_FILE)
+    self.spinner = wx.animate.GIFAnimationCtrl(root, -1, SPINNER_FILE)
     self.spinner.GetPlayer().UseBackgroundColour(True)
     self.spinner.Hide()

@@ -134,7 +134,7 @@ class MainFrame(wx.Frame):
                       wx.BITMAP_TYPE_PNG)
     self.disconnected_bitmap = wx.Bitmap(self.DISCONNECTED_IMAGE_FILE,
                          wx.BITMAP_TYPE_PNG)
-    self.connection_ctrl = wx.StaticBitmap(self.root,
+    self.connection_ctrl = wx.StaticBitmap(root,
                        bitmap=self.disconnected_bitmap)

     border_flag = wx.SizerFlags(1).Border(wx.ALL, self.BORDER)
@@ -147,30 +147,30 @@ class MainFrame(wx.Frame):
     settings_sizer.AddF(self.about_button, border_flag.Expand())

     # Create Output Status Box
-    box = wx.StaticBox(self.root, label=self.HEADER_OUTPUT)
+    box = wx.StaticBox(root, label=self.HEADER_OUTPUT)
     status_sizer = wx.StaticBoxSizer(box, wx.HORIZONTAL)

     status_sizer.AddF(self.radio_output_enable, border_flag)
     status_sizer.AddF(self.radio_output_disable, border_flag)

     # Create Machine Status Box
-    self.machine_sizer = wx.BoxSizer(wx.HORIZONTAL)
+    machine_sizer = wx.BoxSizer(wx.HORIZONTAL)
     center_flag =\
         wx.SizerFlags()\
           .Align(wx.ALIGN_CENTER_VERTICAL)\
           .Border(wx.LEFT | wx.RIGHT, self.BORDER)

-    self.machine_sizer.AddF(self.spinner, center_flag)
-    self.machine_sizer.AddF(self.connection_ctrl, center_flag)
+    machine_sizer.AddF(self.spinner, center_flag)
+    machine_sizer.AddF(self.connection_ctrl, center_flag)
     longest_machine = max(machine_registry.get_all_names(), key=len)
     longest_state = max((STATE_ERROR, STATE_INITIALIZING, STATE_RUNNING),
                 key=len)
     longest_machine_status = '%s: %s' % (longest_machine, longest_state)
-    self.machine_status_text = wx.StaticText(self.root, label=longest_machine_status)
-    self.machine_sizer.AddF(self.machine_status_text, center_flag)
-    refresh_bitmap = wx.Bitmap(self.REFRESH_IMAGE_FILE, wx.BITMAP_TYPE_PNG)
-    self.reconnect_button = wx.BitmapButton(self.root, bitmap=refresh_bitmap)
-    self.machine_sizer.AddF(self.reconnect_button, center_flag)
+    self.machine_status_text = wx.StaticText(root, label=longest_machine_status)
+    machine_sizer.AddF(self.machine_status_text, center_flag)
+    refresh_bitmap = wx.Bitmap(self.REFRESH_IMAGE_FILE, wx.BITMAP_TYPE_PNG)
+    self.reconnect_button = wx.BitmapButton(root, bitmap=refresh_bitmap)
+    machine_sizer.AddF(self.reconnect_button, center_flag)

     # Assemble main UI
     global_sizer = wx.GridBagSizer(vgap=self.BORDER, hgap=self.BORDER)
@@ -183,7 +183,7 @@ class MainFrame(wx.Frame):
              border=self.BORDER,
              pos=(1, 0),
              span=(1, 2))
-    global_sizer.Add(self.machine_sizer,
+    global_sizer.Add(machine_sizer,
              flag=wx.CENTER | wx.ALIGN_CENTER | wx.EXPAND | wx.LEFT,
              pos=(2, 0),
              border=self.BORDER,
@@ -193,8 +193,8 @@ class MainFrame(wx.Frame):
     border = wx.BoxSizer()
     border.AddF(global_sizer,
             wx.SizerFlags(1).Border(wx.ALL, self.BORDER).Expand())
-    border.Fit(self.root)
-    self.SetSizerAndFit(border)
+    root.SetSizerAndFit(border)
+    border.SetSizeHints(self)

     self.Bind(wx.EVT_CLOSE, self._quit)
     self.Bind(wx.EVT_MOVE, self.on_move)
@@ -297,7 +297,6 @@ class MainFrame(wx.Frame):
         self.connection_ctrl.SetBitmap(self.connected_bitmap)
         elif state == STATE_ERROR:
         self.connection_ctrl.SetBitmap(self.disconnected_bitmap)
-        self.machine_sizer.Layout()
     if not self.steno_engine.machine or self.steno_engine.machine.state is not STATE_RUNNING:
         status = self.STATUS_DISCONNECTED
     elif self.steno_engine.is_running:

The important change is the SetSizerAndFit/SetSizeHints part, the rest is just follow-up cleanups.

@benoit-pierre
Copy link
Member

And it works fine on Windows too, let's hope it does on Mac too...

- Remove on/off Plover image
- Set default focus to "Configuration..." instead of toggle
- Implement status indicator as radio buttons
- The machine refresh button is now statically placed on the right
@morinted
Copy link
Member Author

Yes, seems great, as usual. Thanks! It's been applied, works on Mac, and should be ready to submit.

benoit-pierre added a commit that referenced this pull request Feb 22, 2016
@benoit-pierre benoit-pierre merged commit 00046b6 into master Feb 22, 2016
@morinted morinted deleted the windows-grey branch February 29, 2016 06:03
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