Merge lp://qastaging/~mterry/update-manager/update-at-start into lp://qastaging/update-manager

Note that I also filed https://code.launchpad.net/~mterry/update-notifier/use-no-update/+merge/108377 to have update-notifier pass --no-update when launching update-manager.

When I run ./update-manager from your branch I get the following errors on the console:

% ./update-manager
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/gi/overrides/Gtk.py", line 354, in _full_callback
    raise AttributeError('Handler %s not found' % handler_name)
AttributeError: Handler on_button_reload_clicked not found
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/gi/overrides/Gtk.py", line 354, in _full_callback
    raise AttributeError('Handler %s not found' % handler_name)
AttributeError: Handler on_checkbutton_reminder_toggled not found

Probably some additional left over cruft that isn't removed in your branch?

Also, you mention a spec, but I'm not aware of it. Do you have a link?

Minor misspelling at the top of UpdateProgress.py

You might want to run pyflakes over UpdateProgress.py. Unused imports include Gtk, _, ngettext
Also, you can probably collapse the .Core.utils import onto one line
Spaces around the = sign when setting os.environ[]
4 space indents please! :)

Could you add some tests for the new functionality and files?

The spec is https://wiki.ubuntu.com/SoftwareUpdates

The AttributeError is likely because you need to point at the new glade files too (update-manager isn't smart enough to look in the local location). So either futz with XDG_DATA_DIRS or just do "sudo cp data/gtkbuilder/* /usr/share/update-manager/gtkbuilder/"

I'll update UpdateProgress.py and fix the nits. You're killing me with the 4-indents. Not that I care, but update-manager source is very inconsistent with 2 or 4, even inside a file, so I've tried to always stick with 2 in all my pending branches. :(

OK, I've updated to fix nits. I'm not going to do the 4-indent change, because that would cause massive headache with all my chain of pending merges. I'd rather just do a big flag-day indent fixup branch at the end, which will fix existing inconsistencies too.

As for tests, I'd like to talk to mvo first about what his plans were for tests going forward. (tests right now are very hodge-pogde; some rely on internal test-only flags for detecting if a dialog was shown, some just use direct imports of modules and poke around, I don't think any use a UI-driving framework) It's fine if that's a blocker, but it's not something I can fix right this second.

On Tue, Jun 19, 2012 at 09:56:18PM -0000, Michael Terry wrote:
> The spec is https://wiki.ubuntu.com/SoftwareUpdates
>
> The AttributeError is likely because you need to point at the new glade files too (update-manager isn't smart enough to look in the local location). So either futz with XDG_DATA_DIRS or just do "sudo cp data/gtkbuilder/* /usr/share/update-manager/gtkbuilder/"

Alternatively:
$ ./update-manager --data-dir ./data
should work too.

> I'll update UpdateProgress.py and fix the nits. You're killing me with the 4-indents. Not that I care, but update-manager source is very inconsistent with 2 or 4, even inside a file, so I've tried to always stick with 2 in all my pending branches. :(

Yeah, its a big pain, given the restructure that is going on, it
sounds like a good time to make it consistently 4 spaces, wdyt?

Cheers,
 Michael

On Tue, Jun 19, 2012 at 10:07:18PM -0000, Michael Terry wrote:
> OK, I've updated to fix nits. I'm not going to do the 4-indent change, because that would cause massive headache with all my chain of pending merges. I'd rather just do a big flag-day indent fixup branch at the end, which will fix existing inconsistencies too.

That sounds like a reasonable approach.

> As for tests, I'd like to talk to mvo first about what his plans were for tests going forward. (tests right now are very hodge-pogde; some rely on internal test-only flags for detecting if a dialog was shown, some just use direct imports of modules and poke around, I don't think any use a UI-driving framework) It's fine if that's a blocker, but it's not something I can fix right this second.

Indeed, there is no UI-driving framework, my experience with all of
them wasn't that great, so its importing and poking around at the
modules/classes. I'm happy about u-m moving towards a UI-driving
framework, suggestions for a good one welcome.

Cheers,
 Michael

<mterry> mvo, darn, I was hoping you had been hiding some amazing UI driving framework that you had been too lazy to use. :-/
<mterry> mvo, the best I've used is ldtp, but it's very rickety
<mvo> mterry: yeah, excatly, I wasn't overly impressed with that one :/
<mterry> mvo, well, I'm happy to write various tests, but since doing so might involve various code changes (for instrumentation) and since code is moving around a lot in my branches, would you mind if I did a big test branch after all these intermediate branches land? (ala the indentation one)
<mvo> mterry: that is fine with me (for the given reasons)

So I'll start working on an indentation branch and a testing branch after my branches land on trunk. I believe that means there's no outstanding issue with this particular branch. Could I get a re-review?

I got a very minor merge conflict for UpdateManager/UpdateManager.py

<<<<<<< TREE
          if self._get_last_apt_get_update_text() is not None:
              text_label_main = self._get_last_apt_get_update_text()
              ago_minutes = self._get_last_apt_get_update_minutes()
              if ago_minutes is not None and ago_minutes > self.NO_UPDATE_WARNING_DAYS*24*60:
                  text_header = _("Software updates may be available for your computer.")
          # add timer to ensure we update the information when the
          # last package count update was performed
          GObject.timeout_add_seconds(10, self.update_last_updated_text, None)
=======
>>>>>>> MERGE-SOURCE

I manually fixed the merge and tested. After applying updates I noticed the "Details of updates" expander doesn't get hidden. Perhaps that is expected at this point and fixed in another branch.

Still reviewing... more to come later.

On Jun 22, 2012, at 06:36 PM, Ken VanDine wrote:

>I got a very minor merge conflict for UpdateManager/UpdateManager.py
>
><<<<<<< TREE
> if self._get_last_apt_get_update_text() is not None:
> text_label_main = self._get_last_apt_get_update_text()
> ago_minutes = self._get_last_apt_get_update_minutes()
> if ago_minutes is not None and ago_minutes > self.NO_UPDATE_WARNING_DAYS*24*60:
> text_header = _("Software updates may be available for your computer.")
> # add timer to ensure we update the information when the
> # last package count update was performed
> GObject.timeout_add_seconds(10, self.update_last_updated_text, None)
>=======
>>>>>>>> MERGE-SOURCE

I get the same conflict against trunk. I suppose that chunk of code is
supposed to be removed?

On Jun 19, 2012, at 10:07 PM, Michael Terry wrote:

>OK, I've updated to fix nits. I'm not going to do the 4-indent change,
>because that would cause massive headache with all my chain of pending
>merges. I'd rather just do a big flag-day indent fixup branch at the end,
>which will fix existing inconsistencies too.

I think it's fine to defer the reindentation, and I agree the code base is
fairly inconsistent about indentation right now. It would be nice at some
point to do a thorough PEP-8-ification of the code.

>As for tests, I'd like to talk to mvo first about what his plans were for
>tests going forward. (tests right now are very hodge-pogde; some rely on
>internal test-only flags for detecting if a dialog was shown, some just use
>direct imports of modules and poke around, I don't think any use a UI-driving
>framework) It's fine if that's a blocker, but it's not something I can fix
>right this second.

I guess not. This is another thing I'd really like to do at some point,
i.e. refactor the code to be more testable and have better coverage.

Right now your branch is conflicting with trunk. Can you resolve that and
push an update?

On Fri, 2012-06-22 at 18:22 -0400, Barry Warsaw wrote:
> On Jun 22, 2012, at 06:36 PM, Ken VanDine wrote:
>
> >I got a very minor merge conflict for UpdateManager/UpdateManager.py
> >
> ><<<<<<< TREE
> > if self._get_last_apt_get_update_text() is not None:
> > text_label_main = self._get_last_apt_get_update_text()
> > ago_minutes = self._get_last_apt_get_update_minutes()
> > if ago_minutes is not None and ago_minutes > self.NO_UPDATE_WARNING_DAYS*24*60:
> > text_header = _("Software updates may be available for your computer.")
> > # add timer to ensure we update the information when the
> > # last package count update was performed
> > GObject.timeout_add_seconds(10, self.update_last_updated_text, None)
> >=======
> >>>>>>>> MERGE-SOURCE
>
> I get the same conflict against trunk. I suppose that chunk of code is
> supposed to be removed?
>

Looks like it to me.

--
Ken VanDine
Ubuntu Desktop Integration Engineer
Canonical, Ltd.

Yup, the conflicting code just needed to be removed. How about now?

Looks good now, thanks.

On Jun 25, 2012, at 03:06 PM, Michael Terry wrote:

>Yup, the conflicting code just needed to be removed. How about now?

Looks good, thanks. I've merged this into trunk, but didn't upload it, in
case there are other upcoming changes for u-m. Happy to upload it if you
prefer though.