From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.windriver.com ([147.11.1.11]) by linuxtogo.org with esmtp (Exim 4.72) (envelope-from ) id 1Sc6DB-00078p-DI for bitbake-devel@lists.openembedded.org; Wed, 06 Jun 2012 04:43:05 +0200 Received: from ALA-HCA.corp.ad.wrs.com (ala-hca [147.11.189.40]) by mail.windriver.com (8.14.3/8.14.3) with ESMTP id q562Wakq020515 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=FAIL); Tue, 5 Jun 2012 19:32:36 -0700 (PDT) Received: from [128.224.162.164] (128.224.162.164) by ALA-HCA.corp.ad.wrs.com (147.11.189.50) with Microsoft SMTP Server (TLS) id 14.1.255.0; Tue, 5 Jun 2012 19:32:36 -0700 Message-ID: <4FCEC15E.30701@windriver.com> Date: Wed, 6 Jun 2012 10:33:02 +0800 From: Kang Kai User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.2) Gecko/20120216 Thunderbird/10.0.2 MIME-Version: 1.0 To: Darren Hart References: <4FCE4403.6090701@linux.intel.com> In-Reply-To: <4FCE4403.6090701@linux.intel.com> X-Originating-IP: [128.224.162.164] X-MIME-Autoconverted: from 8bit to quoted-printable by mail.windriver.com id q562Wakq020515 Cc: bitbake-devel@lists.openembedded.org, zhenfeng.zhao@windriver.com Subject: Re: [PATCH 1/2] hob2: update DeployImageDialog for seperated tool X-BeenThere: bitbake-devel@lists.openembedded.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Jun 2012 02:43:05 -0000 Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable On 2012=E5=B9=B406=E6=9C=8806=E6=97=A5 01:38, Darren Hart wrote: > > On 06/04/2012 08:37 PM, Kang Kai wrote: >> Part of [Yocto 2388] >> >> Update class DeployImageDialog that could be used by a standalone >> deploy image tool. >> >> Update the method to get all usb device to avoid runtime error, and >> get the deploy image process exit status then give user a information >> dialog. Hi Darren, Thanks a lot for your detail comments. One thing I want to address that the difference between standaonle=20 widget and called by hob is that the standalone widget add a image=20 selection button. Maybe I use the wrong word "singleton" but that why I update the deploy=20 image dialog layout. > These are functionally different tasks that should probably broken up > into a couple patches. This allows us to revert one while maintaining > the other should a problem arise. It also makes reviewing the code easi= er. Ok, I'll break up it. > >> Remove some extra spaces at same time. >> >> Signed-off-by: Kang Kai >> --- >> bitbake/lib/bb/ui/crumbs/hig.py | 82 +++++++++++++++++++++++++++++= +++------- >> 1 files changed, 68 insertions(+), 14 deletions(-) >> >> diff --git a/bitbake/lib/bb/ui/crumbs/hig.py b/bitbake/lib/bb/ui/crumb= s/hig.py >> index 7c9e73f..7d8daad 100644 >> --- a/bitbake/lib/bb/ui/crumbs/hig.py >> +++ b/bitbake/lib/bb/ui/crumbs/hig.py >> @@ -20,6 +20,7 @@ >> # with this program; if not, write to the Free Software Foundation, = Inc., >> # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. >> >> +import glob >> import gtk >> import gobject >> import hashlib >> @@ -63,7 +64,7 @@ class CrumbsMessageDialog(CrumbsDialog): >> """ >> def __init__(self, parent=3DNone, label=3D"", icon=3Dgtk.STOCK_I= NFO): >> super(CrumbsMessageDialog, self).__init__("", parent, gtk.DI= ALOG_DESTROY_WITH_PARENT) >> - >> + >> self.set_border_width(6) >> self.vbox.set_property("spacing", 12) >> self.action_area.set_property("spacing", 12) >> @@ -748,21 +749,28 @@ class DeployImageDialog (CrumbsDialog): >> >> __dummy_usb__ =3D "--select a usb drive--" >> >> - def __init__(self, title, image_path, parent, flags, buttons=3DNo= ne): >> + def __init__(self, title, image_path, parent, flags, buttons=3DNo= ne, singleton=3DFalse): >> super(DeployImageDialog, self).__init__(title, parent, flags= , buttons) >> >> self.image_path =3D image_path >> + self.singleton =3D singleton >> >> self.create_visual_elements() >> self.connect("response", self.response_cb) >> >> def create_visual_elements(self): >> + self.set_size_request(600, 400) >> label =3D gtk.Label() >> label.set_alignment(0.0, 0.5) >> markup =3D "The image to be written i= nto usb drive:" >> label.set_markup(markup) >> self.vbox.pack_start(label, expand=3DFalse, fill=3DFalse, pa= dding=3D2) >> >> + table =3D gtk.Table(2, 10, False) >> + table.set_col_spacings(5) >> + table.set_row_spacings(5) >> + self.vbox.pack_start(table, expand=3DTrue, fill=3DTrue) >> + > > It isn't obvious to me why things like setting the size request and > creating a new table are required to make this stand alone. Do these > changes fit under the "Update class DeployImageDialog that could be use= d > by a standalone deploy image tool." ? If so, fine. If not, they might b= e > better done in a separate patch. Without set the size request, the scroll bar doesn't show the text when=20 the deploy dialog shows. It is ok to a separate patch. > > >> scroll =3D gtk.ScrolledWindow() >> scroll.set_policy(gtk.POLICY_NEVER, gtk.POLICY_AUTOMATIC) >> scroll.set_shadow_type(gtk.SHADOW_IN) >> @@ -770,11 +778,26 @@ class DeployImageDialog (CrumbsDialog): >> tv.set_editable(False) >> tv.set_wrap_mode(gtk.WRAP_WORD) >> tv.set_cursor_visible(False) >> - buf =3D gtk.TextBuffer() >> - buf.set_text(self.image_path) >> - tv.set_buffer(buf) >> + self.buf =3D gtk.TextBuffer() >> + self.buf.set_text(self.image_path) >> + tv.set_buffer(self.buf) >> scroll.add(tv) >> - self.vbox.pack_start(scroll, expand=3DTrue, fill=3DTrue) >> + table.attach(scroll, 0, 10, 0, 1) >> + >> + if self.singleton: >> + gobject.signal_new("select_image_clicked", self, gobj= ect.SIGNAL_RUN_FIRST, >> + gobject.TYPE_NONE, ()) >> + icon =3D gtk.Image() >> + pix_buffer =3D gtk.gdk.pixbuf_new_from_file(hic.ICON_= IMAGES_DISPLAY_FILE) >> + icon.set_from_pixbuf(pix_buffer) >> + button =3D gtk.Button("Select Image") >> + button.set_image(icon) >> + button.set_size_request(140, 50) >> + table.attach(button, 9, 10, 1, 2, gtk.FILL, 0, 0, 0) >> + button.connect("clicked", self.select_image_button_cl= icked_cb) >> + >> + separator =3D gtk.HSeparator() >> + self.vbox.pack_start(separator, expand=3DFalse, fill=3DFalse,= padding=3D10) > What does the self.singleton indicate? How is it related to running > stand-alone or within Hob? As I mentioned above, I just add a image selection button when the tool=20 is run separately. It is convenience So I need to adjust the layout. > >> self.usb_desc =3D gtk.Label() >> self.usb_desc.set_alignment(0.0, 0.5) >> @@ -789,7 +812,7 @@ class DeployImageDialog (CrumbsDialog): >> for usb in self.find_all_usb_devices(): >> self.usb_combo.append_text("/dev/" + usb) >> self.usb_combo.set_active(0) >> - self.vbox.pack_start(self.usb_combo, expand=3DTrue, fill=3DTr= ue) >> + self.vbox.pack_start(self.usb_combo, expand=3DFalse, fill=3DF= alse) > This would appear to be a superfluous layout change unrelated to the > patch topic... > >> self.vbox.pack_start(self.usb_desc, expand=3DFalse, fill=3DF= alse, padding=3D2) >> >> self.progress_bar =3D HobProgressBar() >> @@ -800,13 +823,19 @@ class DeployImageDialog (CrumbsDialog): >> self.vbox.show_all() >> self.progress_bar.hide() >> >> + def set_image_text_buffer(self, image_path): >> + self.buf.set_text(image_path) >> + >> + def set_image_path(self, image_path): >> + self.image_path =3D image_path >> + >> def popen_read(self, cmd): >> tmpout, errors =3D bb.process.run("%s" % cmd) >> return tmpout.strip() >> >> def find_all_usb_devices(self): >> usb_devs =3D [ os.readlink(u) >> - for u in self.popen_read('ls /dev/disk/by-id/usb*').split= () >> + for u in glob.glob('/dev/disk/by-id/usb*') > Secondary to the goal of the patch, but why are we restricting ourselve= s > to sub drives? Why not read /proc/partitions and omit a configurable > blacklist of devices (like /dev/sda for example). Again, secondary to > this patch series. This update just to fix runtime error after commit=20 094742bed2fc01d55f572da946fcfa7a48521401 when there is no USB device=20 then the program crashes. If we want to deploy to other device, I'll update it. > >> if not re.search(r'part\d+', u) ] >> return [ '%s' % u[u.rfind('/')+1:] for u in usb_devs ] >> >> @@ -815,6 +844,9 @@ class DeployImageDialog (CrumbsDialog): >> (self.popen_read('cat /sys/class/block/%s/device/vendor'= % dev), >> self.popen_read('cat /sys/class/block/%s/device/model' %= dev)) >> >> + def select_image_button_clicked_cb(self, button): >> + self.emit('select_image_clicked') >> + >> def usb_combo_changed_cb(self, usb_combo): >> combo_item =3D self.usb_combo.get_active_text() >> if not combo_item or combo_item =3D=3D self.__dummy_usb__: >> @@ -826,12 +858,34 @@ class DeployImageDialog (CrumbsDialog): >> >> def response_cb(self, dialog, response_id): >> if response_id =3D=3D gtk.RESPONSE_YES: >> + lbl =3D '' >> combo_item =3D self.usb_combo.get_active_text() >> - if combo_item and combo_item !=3D self.__dummy_usb__: >> + if combo_item and combo_item !=3D self.__dummy_usb__ and = self.image_path: >> cmdline =3D bb.ui.crumbs.utils.which_terminal() >> if cmdline: >> - cmdline +=3D "\"sudo dd if=3D" + self.image_path = + " of=3D" + combo_item + "\"" >> - bb.process.Popen(shlex.split(cmdline)) >> + tmpname =3D os.tmpnam() >> + cmdline +=3D "\"sudo dd if=3D" + self.image_path = + " of=3D" + combo_item + "; echo $?> " + tmpname + "\"" >> + deploy_process =3D bb.process.Popen(shlex.split(c= mdline)) >> + deploy_process.wait() >> + tmpfile =3D open(tmpname) >> + if int(tmpfile.readline().strip()) =3D=3D 0: >> + lbl =3D "Deploy image successfully" >> + else: >> + lbl =3D "Deploy image failed\nPlease t= ry again" >> + tmpfile.close() >> + os.remove(tmpname) >> + >> + else: >> + if not self.image_path: >> + lbl =3D "No selection made\nYou have not s= elected image to deploy" > "No selection made.\nPlease select an image." > > Is
appropriate here instead of \n? Ok, thanks. > >> + else: >> + lbl =3D "No selection made\nYou have not s= elected USB device" > "No selection made.\nPlease select a device." > > Again, why are we limiting devices to USB devices? Fine, if need I would update this part. Regards, Kai > >> + if len(lbl): >> + crumbs_dialog =3D CrumbsMessageDialog(self, lbl, gtk.= STOCK_DIALOG_INFO) >> + button =3D crumbs_dialog.add_button("Close", gtk.RESP= ONSE_OK) >> + HobButton.style_button(button) >> + crumbs_dialog.run() >> + crumbs_dialog.destroy() >> >> def update_progress_bar(self, title, fraction, status=3DNone): >> self.progress_bar.update(fraction) >> @@ -1035,7 +1089,7 @@ class LayerSelectionDialog (CrumbsDialog): >> # create visual elements on the dialog >> self.create_visual_elements() >> self.connect("response", self.response_cb) >> - >> + >> def create_visual_elements(self): >> layer_widget, self.layer_store =3D self.gen_layer_widget(sel= f.layers, self.all_layers, self, None) >> layer_widget.set_size_request(450, 250) >> @@ -1210,7 +1264,7 @@ class ImageSelectionDialog (CrumbsDialog): >> if f.endswith('.' + real_image_type): >> imageset.add(f.rsplit('.' + real_image_t= ype)[0].rsplit('.rootfs')[0]) >> self.image_list.append(f) >> - >> + >> for image in imageset: >> self.image_store.set(self.image_store.append(), 0, image= , 1, False) >> >> @@ -1226,7 +1280,7 @@ class ImageSelectionDialog (CrumbsDialog): >> for f in self.image_list: >> if f.startswith(self.image_store[path][0] + = '.'): >> self.image_names.append(f) >> - break >> + break >> iter =3D self.image_store.iter_next(iter) >> >> class ProxyDetailsDialog (CrumbsDialog):