[bitbake-devel] [PATCH] Fix bug 8940 - Alow Hob to run images on a custom simulator, other than qemu

[Mirela Rabulea]

Hi Joshua,
Please see my comments below, prefixed with [M.R.].
Thanks a lot for the review, v2 of the patch is attached (squashed version, sorry, but my git send-email is still not working).

Only the latest changes, for easier review:

From: Mirela Rabulea <mirela.rabulea at nxp.com>
Date: Thu, 4 Feb 2016 16:35:33 +0200
Subject: [PATCH] Allow Hob to run images on a custom simulator, other than
 qemu - changes after review

Signed-off-by: Mirela Rabulea <mirela.rabulea at nxp.com>
---
 bitbake/lib/bb/ui/crumbs/builder.py          | 6 +++---
 bitbake/lib/bb/ui/crumbs/imagedetailspage.py | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/bitbake/lib/bb/ui/crumbs/builder.py b/bitbake/lib/bb/ui/crumbs/builder.py
index 80f329a..457cadc 100755
--- a/bitbake/lib/bb/ui/crumbs/builder.py
+++ b/bitbake/lib/bb/ui/crumbs/builder.py
@@ -1360,11 +1360,11 @@ class Builder(gtk.Window):
         return kernel_path
 
     def show_load_run_script_dialog(self):
-        dialog = gtk.FileChooserDialog("Load Run Script", self,
-                                       gtk.FILE_CHOOSER_ACTION_SAVE)
+        dialog = gtk.FileChooserDialog("Select Run Script", self,
+                                       gtk.FILE_CHOOSER_ACTION_OPEN)
         button = dialog.add_button("Cancel", gtk.RESPONSE_NO)
         HobAltButton.style_button(button)
-        button = dialog.add_button("Open", gtk.RESPONSE_YES)
+        button = dialog.add_button("Select", gtk.RESPONSE_YES)
         HobButton.style_button(button)
 
         dialog.set_current_folder(self.parameters.image_addr)
diff --git a/bitbake/lib/bb/ui/crumbs/imagedetailspage.py b/bitbake/lib/bb/ui/crumbs/imagedetailspage.py
index e73827d..6cce8a1 100755
--- a/bitbake/lib/bb/ui/crumbs/imagedetailspage.py
+++ b/bitbake/lib/bb/ui/crumbs/imagedetailspage.py
@@ -337,7 +337,7 @@ class ImageDetailsPage (HobPage):
         change_run_script_button.connect("clicked", self.change_run_script_cb)
         change_run_script_button.set_tooltip_text("Change run script")
         self.run_script_detail = self.DetailBox(varlist=varlist, vallist=vallist, button=change_run_script_button)
-        self.box_group_area.pack_start(self.run_script_detail, expand=True, fill=True)
+        self.box_group_area.pack_start(self.run_script_detail, expand=False, fill=True)
 
         # The default kernel box for the qemu images
         self.sel_kernel = ""
-- 
1.9.1

Regards,
Mirela

-----Original Message-----
From: Joshua G Lock [mailto:joshua.g.lock at linux.intel.com] 
Sent: Wednesday, January 27, 2016 4:03 PM
To: Mirela Rabulea <mirela.rabulea at nxp.com>; bitbake-devel at lists.openembedded.org
Cc: Roman, Alexandru CostinX <alexandru.costinx.roman at intel.com>; Voiculescu, BogdanX A <bogdanx.a.voiculescu at intel.com>
Subject: Re: [bitbake-devel] [PATCH] Fix bug 8940 - Alow Hob to run images on a custom simulator, other than qemu

> I think you meant "and it is qemu-compatible,"?
[M.R.] Yes, my bad. Added a comment to the bug also.


> On an initial look this change looks good, I have a couple of minor comments on the UI:

> FILE_CHOOSER_ACTION_SAVE seems wrong, I think this should use FILE_CHOOSER_ACTION_OPEN as we only want to select existing files. 

> See:
> https://developer.gnome.org/pygtk/stable/gtk-constants.html#gtk-filecho
> oser-action-constants

> Also I think we'd probably want to use something other than "Open" to confirm selection of the emulator, but that's more of a question for Belen. Personally I think "Select" or "Choose" makes more sense.

[M.R.] I agree, I patched show_load_run_script_dialog() with FILE_CHOOSER_ACTION_OPEN and "Select".

> When I ran Hob with your change I noticed the "Run script:" detail is positioned vertically in the middle of its parent box, the other boxes in the "Image details" page have their content at the top of the parent box — could you change that?


[M.R.] All the children in the dialog are added with pack_start(), what varies is the expand=True/False, fill=True/False. I changed for the run script expand=True => False, now it looks as the content is at the top because it is not expanded. I don't see an obvious way to position/center it. 

> When you submit your v2 could you send the patch directly, rather than as an attachment. This allows us to perform any review in-line in an email reply.
[M.R.] I tried to do that, but git send-email in my repo reported status ok, however I never got the email. I'll put the patch by hand in the email, for now (also I'll attach it, in case there's a problem with that).

> You can use either the scripts in the openembedded-core repository
> (scripts/[create|send]-pull-request) or git send-email directly.

> Furthermore as the patch, when ready, will be applied directly to the bitbake repository it would be useful to improve the commit message in line with the OpenEmbedded recommendations: 
> http://www.openembedded.org/wiki/Commit_Patch_Message_Guidelines

> Specifically could you:
> * include some detail in the commit message, perhaps use the reasoning above?
> * move the bug ID to the bottom of the
> commit message, above the signed-off-by. It should be formatted as:
> [YOCTO #8940]
> * Fix the typo in the one-line description "Alow" -> "Allow"

[M.R.] Done. The squashed commit has a detailed message, fixed typo, bug ID as asked.

> Of course having written this I realise we don't have a good resource for BitBake contribution guidelines I've filed a bug in the Yocto Project bugzilla to ensure we develop some guidelines for contributing to the BitBake repository:
> https://bugzilla.yoctoproject.org/show_bug.cgi?id=9007

[M.R.] Good idea, thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug_8940_squashed.patch
Type: application/octet-stream
Size: 11476 bytes
Desc: bug_8940_squashed.patch
URL: <http://lists.openembedded.org/pipermail/bitbake-devel/attachments/20160204/c1cd1652/attachment-0002.obj>