[Toaster] [review-request] dreyna/project-config-vars-6588

[Damian, Alexandru]

Taken for submission with some changes -

- whitespace fixing
- split in 2 patches, one for bitbake and one for the meta-yocto layer
changes

Cheers,
Alex

On Thu, Jan 15, 2015 at 12:37 PM, Barros Pena, Belen <
belen.barros.pena at intel.com> wrote:

> Thanks, David!
>
> For the design point of view, I think we can submit this. I've added some
> comments inline. I have also a smallish list of issues (I can open them as
> separate Bugzilla entries and assign to you):
>
> * When I add a variable using the 'Add variable' form, the 'delete'
> tooltip is missing from the icon next to the added variable name. The
> tooltip is also missing from the IMAGE_INSTALL_append variable when you
> set it to a value
>
>
> * For the DISTRO variable, I can't see the validation messages (we check
> only for spaces in this one)
>
> IMAGE_FSTYPES
>
> * The value of the IMAGE_FSTYPES variable in my instance is set to 'ext3
> jffs2 tar.bz2'. When I click the change icon, I can see 'jffs2' and
> 'tar.bz2' checked at the top, but ext3 is nowhere on the list. Neither at
> the top nor with the unchecked fstypes.
>
> * The search field is not filtering out the content of the fstypes list as
> I type. I've used some javascript in the prototype for this (lines 417 to
> 429 in
> http://git.yoctoproject.org/cgit/cgit.cgi/yocto-webhob-design/tree/WebHob_1
> .5/localconf.html) You might want to look at it and see if it can be
> reused, or change as required.
>
> * You can uncheck all fstypes and then 'save'. That should not be allowed.
> If you uncheck all checkboxes, we need to disable the "save" button and
> add a message (you can see it in page 8 of the design document)
>
>
> IMAGE_INSTALL_append
>
> * When you set a value then refresh the page, the value gets the .muted
> class. This should not happen. To replicate: click the change button,
> enter a value, click save, then refresh the page.
>
>
> * When the input field is empty, the "save" button needs to be disabled. I
> can see it enabled with an empty input field when the variable is empty
> (not set) and I click the "change" icon. When you click "save" while the
> input field is empty, the page shows only the "change" and "delete" icons.
> That shouldn't happen.
>
> * Something funny happening with this field. Set it to a value, then click
> the "change" icon, delete the value from the input field, then click
> cancel. The previous value stays (as expected), but the "delete" icon
> disappears (it should still be there). If you click the "edit" button, the
> field is empty, instead of being set to the current value of the variable.
>
> * There is also a bug with the delete icon. Set the variable to a value,
> then click the "delete" icon. The variable changes to "not set" (as
> expected). Click the "change" icon: the input field is set to the value
> you just deleted (it should be blank).
>
> * I am not sure how to test if Toaster adds the required space between the
> quotation character and the start of the first package name.  Any ideas?
>
> PACKAGE_CLASSES
>
> * This is working great. The only thing is that we should probably provide
> some kind of transition for the checkboxes value changes. Otherwise it
> becomes really hard to follow what's happening. I've pushed an example to
>
> http://www.yoctoproject.org/toaster/localconf.html
>
> Some more comments inline in the email below.
>
>
> On 15/01/2015 09:52, "Reyna, David" <david.reyna at windriver.com> wrote:
>
> >Hi Belen,
> >
> >Here is my re-based and updated commit for the project configure page. I
> >will list the remaining (non-blocking) issues that I would like to
> >postpone till the next sprint, and I will also list the items I believe
> >fixed now.
> >
> >1) You have to start with a clean database to get the JSON file updates.
> >
> >  $ cd Š/poky/..
> >  $ rm ­f toaster.sqlite
> >  $ ./poky/bitbake/bin/toaster
> >  Š
> >  -- Do you want to import basic layer configuration from
> >"/opt/dreyna/yp-1.8/poky/meta-yocto/conf/toasterconf.json" ? (y/N):y
> >  Š
> >  $
>
> >
> >2) Here are the remaining issues that I propose postponing till the next
> >check-in
>
> >
> >[?] ** When I add a variable, and then I click the "change" icon: ... 2)
> >the "delete" icon disappears (it should stay next to the
> >variable name).
> >
> >DLR: For #2, I believe that the "delete" icon _must_ disappear here,
> >because I would think that one should not be able to delete the variable
> >while one is in the state of changing its value.
>
> I completely agree :) Thanks!
>
> >
> >[?] ** While a validation message is showing, the 'add variable' button
> >should
> >be disabled: we don't want users to submit an invalid value.
> >
> >DLR: I was unable to replicate this issue. Do you have a specific
> >workflow that led to this observation?
>
> I can't replicate this either in your latest branch.
>
> >
> >[-] ** The validation message should be cleared the moment I remove the
> >offending character (.i.e. on keyup). Right now they are cleared on blur.
> >
> >DLR: Postpone?
>
> Agreed. I will open an issue for this.
>
> >
> >[-] ** this might be too much to ask, but I'll write it down any way.
> >Variable
> >names are case sensitive, which I guess means that IMAGE_FSTYPES is not
> >the same as image_fstypes. However, since we are checking for the
> >blacklisted variables any way, is there any chance we could check for
> >their lower case variants as well so that they prompt the same validation
> >message the uppercase ones show?
> >
> >DLR: Postpone?
>
> Agreed. I will open an issue for this
>
> >
> >[-] ** When I click the "delete" icon, the variable just disappears: we
> >are
> >missing the fade out animation (the same one we are using for
> >IMAGE_INSTALL_append)
> >
> >DLR: I almost have this fully working. There is just a remaining flash of
> >the value after it slides up and before the page is redrawn.
>
> Right now the variable simply disappears. This is fine, but it would be
> nice to have the animation in to match similar behaviours across Toaster.
> We can postpone for the moment and I'll open a separate issue for this.
>
> >
> >
> >3) Here are the issues that believe are now fixed
> >
> >* 'add variable' form
> >
>
> >
> >
> >[x] ** For the variable field, all validation messages are working
> >correctly,
> >except the one caused by the presence of a space. When I type a space, I
> >get this message: "A valid variable name can only include letters,
> >numbers, underscores, dashes and cannot include spaces". But I should get
> >this one: "A valid variable name can only include spaces", so that I am
> >told specifically what is causing the problem. Yes, I know: I am a
> >validation freak :/ I hope you'll entertain this quirk of mine ;)
>
> This is not working for me, but it doesn't really matter. The validation
> is pretty good as is :)
>
> >
> >
> >- David
> >
> >
>
>


-- 
Alex Damian
Yocto Project
SSG / OTC
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.yoctoproject.org/pipermail/toaster/attachments/20150116/6495e010/attachment-0001.html>