>>>>> "Jelmer" == Jelmer Vernooij <[email protected]> writes:
Jelmer> Hi Vincent,
Jelmer> Vincent Ladeuil wrote:
>> This patch uses the new progress reporting API available in bzr
>> since 1.12 :-P
>>
>> The nice thing is that bzr viz now shows a lot more than before
>> (or at least that what I remember :-)
>>
Jelmer> As far as I can tell nothing has changed.
Ok, I was under the impression that a bit more specialized messages were
shown, but I may be wrong.
>> I'm pretty sure the patch is conservative but I'd appreciate
>> feedback on any edge case (I mostly tested viz).
>>
Jelmer> Thanks for working on this, it was indeed very much overdue.
Jelmer> There seem to be a lot of unrelated whitespace fixes, they make the
Jelmer> patch a bit harder to read.
Sorry about that, I generally revert them before submitting, but the
ones I left are in modified areas anyway (and AFAICS they are spaces at
end of line which are hard to undo when located in hunks of other
modification(s)).
Jelmer> The patch seems ok, in general but there's some FIXME's that don't
seem
Jelmer> right:
Good, I put them there as hints for the review.
>> + # FIXME: Why is the following needed ? Are there really cases
where we
>> + # use a TreeView without installing our own ui ? --vila 20090610
>> + if getattr(ui.ui_factory, "set_progress_bar_widget", None) is
not None:
>> + ui.ui_factory.set_progress_bar_widget(loading_msg_widget)
Jelmer> This can happen if somebody else imports bzr-gtk widgets but doesn't
Jelmer> change the UI.
Hmm, that was my fear but I couldn't find any example in my limited
research.
Besides, at that place, a progress widget *is* defined. I'm not sure
there are cases where someone want to reuse the TreeView without the
embedded progress widget. Or may be that progress widget should be
defined *outside* the tree view ?
>> + # FIXME: The following seems to be there to provide a default
for cases
>> + # where set_progress_bar_widget() is not called explicitely. It
will be
>> + # better to call it explicitely and get rid of that default.
(I'm not
>> + # even sure it really needed now :-/ -- vila 20090610.
Jelmer> We rely on this in a lot of places, and we can't get rid of it in
the
Jelmer> near future, and perhaps never. Progress bars can be created by
pretty
Jelmer> much any call to bzrlib.
I will tend to answer to that by:
if we have a progress widget:
use it
else:
tell bzr to *not* report progress
And try to add progress widgets anywhere it's needed. This can be done
progressively driven by demand (bzr doesn't report progress everywhere it
should either).
Jelmer> If you for example run "bzr gannotate" in a subversion
Jelmer> working copy you get a window-based progress bar.
That also means that if you *don't* need a window-based progress bar,
you can't avoid using one either...
And then there is the question of destroying that window when you don't
need it anymore which we have no way to detect :-/
>> def clear(self):
>> self.pb.clear()
>> + # FIXME: destroy() ? Really ? -- vila 20090610
>> self.destroy()
Jelmer> This is necessary because otherwise the progress bar window stays
Jelmer> around, visible to the user.
But who calls clear() and when ?
Should we use rely on ui.clear_term() ? It's at least an indication from
brzlib that progress reporting should be stopped, but it can also means
that the progress reporting should just be suspended and will be resumed
later.
So unless bzr-gtk knows when bzrlib is invoked and when the operations
are finished, we have no way to safely close that report *window*.
Jelmer> Some ad-hoc testing shows me that the progress windows now
Jelmer> stay around frozen rather than being hidden properly.
Lacking automating test sucks :-)
But describing ad-hoc tests rocks ! How did you test ? :-)
Jelmer> There's some bits of the network activity still being
Jelmer> printed to stdout. It would be nice to show this
Jelmer> information in the progress bars, or alternatively we could
Jelmer> just hide it for now.
I can hide it easily and wait for another submission to do it better,
but again how did you test that ?
>> The intent is to land that patch ASAP as one can't use bzr.dev
>> and bzr-gtk trunk anymore.
I don't mind continuing working on the subject (usual lack of time still
applying though), but the main intent is to address the above.
So let's try to make it works again before making it right, ok ? :)
>>
>> bzr-1.16 is pretty close, it would be nice to release
>> bzr-gtk-0.9.6 soon (hint [1] :)
>>
>>
>> [1]: Looks like we can settle on bug
>> https://bugs.edge.launchpad.net/bzr-gtk/+bug/377476 now, yes ? :-D
>>
Jelmer> IMHO that bug report should be closed as invalid for bzr-gtk
Jelmer> as it is a problem in seahorse. What do you think?
Full agreement. My understanding of the issue is that you objected to
Silvester work-around because the real problem was unknown and masking
it wasn't a good solution. Now that we know the bug is known and
addressed upstream, I think the work-around makes sense.
That being said, thanks for the insightful review, I'll submit a new
patch ASAP.
Vincent
--
bzr-gtk mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.canonical.com/mailman/listinfo/bzr-gtk
