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

[Barros Pena, Belen]

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
> 
>