Merge lp://staging/~sinzui/bzr-gtk/ui-factory into lp://staging/bzr-gtk

Proposed by Curtis Hovey
Status: Merged
Approved by: Jelmer Vernooij
Approved revision: 781
Merged at revision: 780
Proposed branch: lp://staging/~sinzui/bzr-gtk/ui-factory
Merge into: lp://staging/bzr-gtk
Diff against target: 873 lines (+582/-110)
4 files modified
setup.py (+41/-24)
tests/__init__.py (+51/-19)
tests/test_ui.py (+375/-0)
ui.py (+115/-67)
To merge this branch: bzr merge lp://staging/~sinzui/bzr-gtk/ui-factory
Reviewer Review Type Date Requested Status
Jelmer Vernooij (community) Approve
Review via email: mp+94866@code.staging.launchpad.net

Description of the change

Add GtkUIFactory implementations and missing tests.

This branch was extracted from my effort to guarantee that the progress bar
shows when I use gpush.

--------------------------------------------------------------------

RULES

    * Add tests for all the implemented classes and specifically GtkUIFactory.
    * Add ._progress_updated() and .report_transport_activity() that are
      needed to update the progress widget.
    * Add the message, warning, and error dialogs.
      * Maybe update the Confirmation dialog to extend MessageDialog?
    * ADDENDUM: Let me run just one test module!

LINT

    added:
      tests/test_ui.py
    modified:
      setup.py
      tests/__init__.py
      ui.py

TEST

    ./setup.py check
    or
    ./setup.py check -m test_ui

IMPLEMENTATION

I was frustrated running the whole suite which takes 18 seconds on my
computer. I updated the test runner code to use existing 'discover'
option to run the whole suite, or accept a single module as an arg. The
ui tests complete in 0.107s on my computer.
    setup.py
    tests/__init__.py

I First added tests for the existing classes. I have never used this
many mocks in a test suite before, they definitely make the tests fast,
though I thought I might have used too many. I tried removing a few but
the tests could become may times slower or just never complete without
forcing the Gtk.main_loop to run.
    tests/test_ui.py

I then added test for new methods and refactored some of the code.
* I extracted the main_iteration code in one method to a decorator
  function that I used on several progress bar methods to ensure
  the UI updates immediately.
* I added the Info, Warning, and Error dialogs and refactored PromptDialog
  to extend Gtk.MessageDialog. This provides an icon with the text
  message, as well a guarantees that the spacing and layout conforms
  to Gnome HIG.
* After I extracted the main_loop iteration rules from update(), changed
  self.fraction to fraction because nothing used it. I changed the error
  to a ValueError because it can only be caused by insane inputs.
* I extracted the common progress window and panel methods to
  ProgressContainerMixin which was easy since I had already written
  tests to that used a similar mixin to verify their function.
* I updated ProgressPanel to GtkBox since GtkHBox is deprecated.
* I added show_user_warning, which is almost identical to the TextUI
  implementation.
* I added ._progress_updated() and .report_transport_activity() which made
  the gpush progress bar show sooner and update more often.
    ui.py
    tests/test_ui.py

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

This is *really* nice. Thanks, Curtis!

review: Approve
779. By Curtis Hovey

Merged GtkUIFactory additions and tweaks.

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

This seems to break 'bzr selftest -s bp.gtk'.

review: Needs Fixing
Revision history for this message
Curtis Hovey (sinzui) wrote :

I suck. I will fix this right away.

780. By Curtis Hovey

Support selftest, check, and check -m

781. By Curtis Hovey

DRY.

Revision history for this message
Curtis Hovey (sinzui) wrote :

I merged this branch into trunk a few hours before you discovered my badness. I have a branch that fixes this issue: https://code.launchpad.net/~sinzui/bzr-gtk/setup-fix/+merge/95013

Revision history for this message
Curtis Hovey (sinzui) wrote :

Oh never mind. either I never pushed my branch or you reverted. I have pushed my changes to this branch and these are my changes I reported in the other MP that I am deleting.

RULES

    * Consider reverting the changes if the fix cannot be made in a few
      hours
    * Setup.py can set the module to None, self tests passes a module
      object, and the user -m arg can be a string
      * Only filter the discovered tests if the module is a basestring.
      * ADDENDUM: It is trick using the native discover feature because
        it builds a module name in a different name space; not
        bzrlib.plugins.gtk. A listing of the directory using the same
        rules as discover might work to construct a module name that
        always works.

TEST

    ./setup.py check
    ./setup.py check -m ui
    BZR_PLUGINS_AT=gtk@./ bzr selftest -s bp.gtk

IMPLEMENTATION

I changed the default module in setup.py to None so that it was easy
to detect a basestring to select a test. I wrote my own function to
find tests module names because discover was not construction the module
name of bzrlib.plugins.gtk when run under selftest. The function uses
the same file matching rules as discover, but always uses the current
module name to construct the test module name. There are still few lines
of code to load tests then there were before.

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

Thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: