Merge lp:~agateau/checkbox/progress-bar into lp:checkbox

Proposed by Aurélien Gâteau on 2012-02-09
Status: Merged
Merge reported by: Daniel Manrique
Merged at revision: not available
Proposed branch: lp:~agateau/checkbox/progress-bar
Merge into: lp:checkbox
Diff against target: 69 lines (+26/-3)
3 files modified
checkbox_gtk/gtk_interface.py (+6/-0)
gtk/checkbox-gtk.ui (+16/-3)
plugins/jobs_prompt.py (+4/-0)
To merge this branch: bzr merge lp:~agateau/checkbox/progress-bar
Reviewer Review Type Date Requested Status
Daniel Manrique Approve on 2012-03-01
Brendan Donegan (community) 2012-02-09 Needs Fixing on 2012-02-27
Review via email: mp+92304@code.launchpad.net

Description of the change

This branch adds a progress bar to the main test window, showing how many tests remain to be run. The changes in the .ui looks heavy but it's just glade rewriting things: it just replace the GtkButtonBox holding the Test button with a GtkBox and inserts a GtkProgressBar to the left of the Test button.

To post a comment you must log in.
Daniel Manrique (roadmr) wrote :

Thanks, this is awesome! It's easily one of the top five features requested in Checkbox.

Checkbox rev 1109 also had some rearranging of components in Glade, but Glade really made a mess of the XML files for what was meant to be a simple change. What I did was make the changes manually in the XML file and the patch came out much cleaner. If you have a moment, could you see if it's possible to do this here? If not, don't worry, I'll probably look into doing it manually myself and then test that all works well.

Thanks again, much appreciated.

Brendan Donegan (brendan-donegan) wrote :

Aurelien, thanks for the code! Do you agree with what Daniel said about changing the ui file manually? If so can you take care of it and resubmit, otherwise if you don't have the time then as Daniel mentioned please let us know and we may take care of it ourselves (giving you due credit of course!)

review: Needs Fixing
Aurélien Gâteau (agateau) wrote :

Sure, I am going to look into reworking the .ui change to something easier to read.

lp:~agateau/checkbox/progress-bar updated on 2012-02-28
1200. By Aurélien Gâteau on 2012-02-28

Redone changes to the Glade file by hand.

Should be easier to review now.

Daniel Manrique (roadmr) wrote :

Hi Aurélien!

Thanks for updating the glade file, I hope you're OK with the changes and it still fits your purpose.

One last thing before we move forward. I notice you're still carrying the progress information in the job itself. As I mentioned before, this feels like a forced use of the job to shuttle information to the frontend. I'm wondering if you have a specific reason for preferring this implementation; maybe it's something you are using elsewhere, or have future plans for it.

In any case, the mechanism I suggested (where an event gets fired which then is caught by the user_interface plugin which in turn sets a value in the UserInterface itself) is an alternative, although it's more complicated it avoids using the job for this purpose.

Please let me know if this is something you definitely want to be as you propose, or you'd be open to considering the other alternative (the progress bar will still work the same way).

Thanks!

review: Needs Information
Aurélien Gâteau (agateau) wrote :

I didn't have any specific idea in mind. It just felt like the easiest way to go, without having to patch all frontends. I am still struggling with checkbox codebase, so I don't always figure out the checkbox-way to do things.

Will have a look at this on monday.

Daniel Manrique (roadmr) wrote :

Hi Aurelien,

I'll merge this, based on your latest comment I took the liberty of replacing the mechanism to convey the progress information to the one I suggested, which is also generic enough to be used (or ignored, as needed) by all frontends.

Incidentally, the mechanism used to carry a variable from the core to the frontend is also being used for carrying a report_url variable, so it seems generic enough :)

Thanks!

review: Approve
Aurélien Gâteau (agateau) wrote :

Sounds good, thanks!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'checkbox_gtk/gtk_interface.py'
2--- checkbox_gtk/gtk_interface.py 2012-02-22 17:16:30 +0000
3+++ checkbox_gtk/gtk_interface.py 2012-02-28 14:37:18 +0000
4@@ -519,6 +519,12 @@
5 self._set_hyper_text_view("hyper_text_view_test",
6 test["description"])
7
8+ # Update progress bar
9+ bar = self._get_widget("progressbar_test")
10+ done, total = test["progress"]
11+ bar.set_text("%d/%d" % (done, total))
12+ bar.set_fraction(float(done) / total)
13+
14 # Set buttons
15 if "command" in test:
16 self._set_sensitive("button_test", True)
17
18=== modified file 'gtk/checkbox-gtk.ui'
19--- gtk/checkbox-gtk.ui 2011-11-04 20:32:54 +0000
20+++ gtk/checkbox-gtk.ui 2012-02-28 14:37:18 +0000
21@@ -165,9 +165,22 @@
22 </packing>
23 </child>
24 <child>
25- <object class="GtkHButtonBox" id="hbuttonbox_test">
26+ <object class="GtkBox" id="box_test">
27 <property name="visible">True</property>
28- <property name="layout_style">end</property>
29+ <property name="can_focus">False</property>
30+ <property name="spacing">6</property>
31+ <child>
32+ <object class="GtkProgressBar" id="progressbar_test">
33+ <property name="visible">True</property>
34+ <property name="can_focus">False</property>
35+ <property name="show_text">True</property>
36+ </object>
37+ <packing>
38+ <property name="expand">True</property>
39+ <property name="fill">True</property>
40+ <property name="position">0</property>
41+ </packing>
42+ </child>
43 <child>
44 <object class="GtkButton" id="button_test">
45 <property name="label" translatable="yes">_Test</property>
46@@ -179,7 +192,7 @@
47 <packing>
48 <property name="expand">False</property>
49 <property name="fill">False</property>
50- <property name="position">0</property>
51+ <property name="position">1</property>
52 </packing>
53 </child>
54 </object>
55
56=== modified file 'plugins/jobs_prompt.py'
57--- plugins/jobs_prompt.py 2012-02-22 21:54:24 +0000
58+++ plugins/jobs_prompt.py 2012-02-28 14:37:18 +0000
59@@ -127,7 +127,11 @@
60 if not messages:
61 break
62
63+ done_count = self.store.get_pending_offset()
64+ pending_count = self.store.count_pending_messages()
65+
66 job = messages[0]
67+ job["progress"] = (done_count, done_count + pending_count)
68 self._manager.reactor.fire("prompt-job", interface, job)
69 self.store.update(job)
70

Subscribers

People subscribed via source and target branches