Thanks for taking a look at this Broc, comments below: JR
Brock Pytlik wrote: > jmr wrote: >> Michal - new webrev at: >> >> http://cr.opensolaris.org/~jmr/1723_statup_3087_shutdown_1915_categorysort_v1_Aug27/ >> >> >> >> > > Lines 1507-1511 make it seem like this is a dead function. Is it a > stub yet to be filled or a function on its way out? Either way, the > comment should reflect that. Also, one space after the # in comments > please. Not sure which section you are referring to, are you talking about get_installed_version() ? > > Lines 1562-1565: > Would this be clearer as... > while task.next(): > pass > gobject.idle_add(self.w_progress_dialog.hide) Yep much clearer, this had evolved a little. > > Would the separation be even cleaner if __get_image_from_directory > yielded the percentage, or a tuple of pkg_cnt and total_pkg_count > instead of making the call to gobject.idle_add from inside that function? I see what you mean we have the setup code in the caller and then manipulate the progress dialog in __get_image_from_directory(). I could do as you suggest or I could pass in the self.w_progress_dialog widget to __get_image_from_directory(), so folks reading the code could see what we were intending. I'll play around with it a bit. > I'm also unclear why you're yielding to a while loop which just keeps > churning through things. A third option would be to have > __get_image_from_directory simple make the calls to gobject._idle_add > both from inside __get_image_from_directory. I'm not understanding the > code structure. I also don't know a great deal about python GUI > programming, so I'd like to understand the reasoning behind this layout. > > Is what these changes do basically put off starting the package view > until after the graphical shell has been displayed? That's exactly what we are doing. Otherwise on startup we just get nothing and then a GUI that's a series of gray boxes until the setup functions return, not a very nice startup experience. We tried lots of different approaches here, but the GUI refused to redraw during startup, even when we put the __get_image_from_directory in its own thread, the GUI just wasn't getting the CPU. The gobject.idle_add puts a function on the GUI's task list and the main GTK thread will pick it up when its not doing anything else. With the older structure it never got a chance to service these low priority GUI events. Forcing the __get_image_from_directory function to yield in this way allows us to give control back to the main GTK thread and so we can get the progress updated and all is well. The goal here is not so much about saving time, but clearly showing a user what is being done during startup. I intend to look at optimizing the startup elements next, where it makes sense, particulary in the __application_filter code. Note: We use gobject.idle_add generally when we are in other threads and want to do something which needs to happen on the GTK main loop. See http://faq.pygtk.org/index.py?req=show&file=faq23.020.htp for more details on progress bars in PyGTK and using python generators to create these pseudo-threads. More details on generators and threading in http://www.ibm.com/developerworks/linux/library/l-pythrd.html#author1 > > Did the static methods just get moved wholesale without any internal > changes? I didn't pick up any differences when I scanned them > side-by-side, but I wanted to be sure. > Yep - was doing a little house keeping. > My experience has been that the packagemanager reexecs itself, is that > still the case? If it is, should the comment on line 630 reflect that? I'll update the comment. On line 695 we respawn the pakagemanager if need be on shutdown, this was the cleanest way we found to do it: gobject.spawn_async([self.application_path, "-R", \ 696 self.image_dir_arg]) > > > I realize not all these comments are germane to this particular > changeset, but since this is the first time I've looked closely at the > GUI code, I thought I'd throw out the questions I had. > Appreciate the review, more eyes always help :) > Thanks, > Brock _______________________________________________ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
