Created attachment 767 [details]
settings SSTATE_MIRRORS field

Yes it is a bug, it should apper as in the image I have attached. But we have also to change the interface because the variable SSTATE_MIRRORS isn't a path for a directory. It is a link, so the button to browse should be change in something else. Belen can you say us how should this be? Thanks.

After I will know how this should be, I will make a patch with all.

Hi Cristiana,

You are right: it needs to be a valid URL. But it can be a URL pointing to a local or remote resource. The easiest way of inputting a local resource is using a file selection dialog, and that's why the interface is the way it is. I totally agree though that we need to be more clear about what inputs are valid for this field. To help with that, we should change the text that comes up when you click on the 'i' icon to: 

"A URL pointing to a pre-built mirror that will speed your build. The URL can point to a remote location (e.g. http://someserver.tld/sstate/) or to a local directory (e.g. file:///some/local/dir/sstate/)."

About the 'mirrors' typo, it will be fixed as part of bug 2162 (the label will be changed to "Shared mirror URL").

About the value displayed inside the text field, I see in your screenshot that the mirror URL is preceded by "file://.*". We should take that out and display only the URL (in your screenshot ftp://10.237.112.100/sstate-cache). Also, if you use the file selection dialog to select a local directory, we should add "file://" to the beginning of the path automatically, so that users don't need to type it by hand. 

I hope this makes sense. I have attached a pdf document explaining the above (shared-mirror-URL.pdf), just in case it helps.

If you have any questions, please let me know.

Belen



(In reply to comment #2)
> Yes it is a bug, it should apper as in the image I have attached. But we
> have also to change the interface because the variable SSTATE_MIRRORS isn't
> a path for a directory. It is a link, so the button to browse should be
> change in something else. Belen can you say us how should this be? Thanks.
> 
> After I will know how this should be, I will make a patch with all.

Created attachment 779 [details]
shared-mirror-URL.pdf

Hi Scott, 

I am assigning this bug to you so that you can review the content of the information bubble and make sure you are happy with it. To give you some context, we need to provide a bit more detail about what constitutes a valid SSTATE_MIRROR value. To do so, I am suggesting we change the label to:

"Shared mirror URL"

And the information bubble should say something like: 

"A URL pointing to a pre-built mirror that will speed your build. The URL can point to a remote location (e.g. http://someserver.tld/sstate/) or to a local directory (e.g. file:///some/local/dir/sstate/)."

The screenshots show the Hob screens involved.

Any changes you'd like to make in the text?

Thanks!

Belen

Belen, 

The bubble looks fine.  No changes recommended. Do I mark this resolved?

Scott

Hi Belen,

(In reply to comment #3)
> Hi Cristiana,
> 
> You are right: it needs to be a valid URL. But it can be a URL pointing to a
> local or remote resource. The easiest way of inputting a local resource is
> using a file selection dialog, and that's why the interface is the way it
> is.

I understand your comments, but unfortunately I don't think this is enough. The main problem is that SSTATE_MIRRORS can have more than one entry (and with the last changes it apparently _needs_ to have more than one entry, one for native packages and one for everything else). For example, my SSTATE_MIRRORS looks like this now:

SSTATE_MIRRORS ?= "\
file://Ubuntu-12.04/(.*) ftp://10.237.112.100/sstate-cache/Ubuntu-12.04/\1 \n \
file://(.*) ftp://10.237.112.100/sstate-cache/\1 \n \
"
(the first line is needed for native packages, the second one for everything else).

With this in mind, the file selection dialog is no longer an option IMO. The easy way to fix this is to let the user enter the content of SSTATE_MIRROS manually, without any dialogs. If we want to get more user-friendly, SSTATE_MIRRORS should probably get its own dialog that would have a list with the current SSTATE_MIRRORS entries and buttons for "Add new URL", "Add new local dir", "Remove entry". But note that the user-friendly part is further complicated by the contents of SSTATE_MIRRORS. Take for example the first part of SSTATE_MIRRORS above:

file://Ubuntu-12.04/(.*) ftp://10.237.112.100/sstate-cache/Ubuntu-12.04/\1

Note that this is actually a regexp (the '\1' at the end takes its value from the '(.*)' after the file:// URL). This is a very flexible mechanism and unfortunately quite hard to capture in a GUI. 
In conclusion, I'd make SSTATE_MIRRORS a simple text entry widget, without any associated file selection dialog and let the user have full control over it. Validating its contents might be yet another challenge, due to the issue explained above.
There are some good news too: the fix for the original bug (SSTATE_MIRRORS not reflected in the GUI) should be quite easy :)

Thanks Scott. I'll assign the bug back to Cristiana.

Cheers

Belen

(In reply to comment #6)
> Belen, 
> 
> The bubble looks fine.  No changes recommended. Do I mark this resolved?
> 
> Scott

Hi Bogdan,

Thanks for the information. I am a bit confused now: just had a brief chat with Paul E about this, and he believes that you don't need two entries. You just need to add 'PATH' to the SSTATE_MIRRORS URL and that will append the appropriate distro-specific path for native recipes. 

If the above is correct, we should probably add 'PATH' to the URL entered in the SSTATE_MIRROR field behind the scenes. We would then use the info icon to explain that mirrors must have the correct directory structure, and point users to the section of the manual where they can find more information about this. 

Before I run away to redesign the Settings dialogue, maybe we should confirm Paul's comments. What do you think?

Cheers

Belen



(In reply to comment #7)
> Hi Belen,
> 
> (In reply to comment #3)
> > Hi Cristiana,
> > 
> > You are right: it needs to be a valid URL. But it can be a URL pointing to a
> > local or remote resource. The easiest way of inputting a local resource is
> > using a file selection dialog, and that's why the interface is the way it
> > is.
> 
> I understand your comments, but unfortunately I don't think this is enough.
> The main problem is that SSTATE_MIRRORS can have more than one entry (and
> with the last changes it apparently _needs_ to have more than one entry, one
> for native packages and one for everything else). For example, my
> SSTATE_MIRRORS looks like this now:
> 
> SSTATE_MIRRORS ?= "\
> file://Ubuntu-12.04/(.*) ftp://10.237.112.100/sstate-cache/Ubuntu-12.04/\1
> \n \
> file://(.*) ftp://10.237.112.100/sstate-cache/\1 \n \
> "
> (the first line is needed for native packages, the second one for everything
> else).
> 
> With this in mind, the file selection dialog is no longer an option IMO. The
> easy way to fix this is to let the user enter the content of SSTATE_MIRROS
> manually, without any dialogs. If we want to get more user-friendly,
> SSTATE_MIRRORS should probably get its own dialog that would have a list
> with the current SSTATE_MIRRORS entries and buttons for "Add new URL", "Add
> new local dir", "Remove entry". But note that the user-friendly part is
> further complicated by the contents of SSTATE_MIRRORS. Take for example the
> first part of SSTATE_MIRRORS above:
> 
> file://Ubuntu-12.04/(.*) ftp://10.237.112.100/sstate-cache/Ubuntu-12.04/\1
> 
> Note that this is actually a regexp (the '\1' at the end takes its value
> from the '(.*)' after the file:// URL). This is a very flexible mechanism
> and unfortunately quite hard to capture in a GUI. 
> In conclusion, I'd make SSTATE_MIRRORS a simple text entry widget, without
> any associated file selection dialog and let the user have full control over
> it. Validating its contents might be yet another challenge, due to the issue
> explained above.
> There are some good news too: the fix for the original bug (SSTATE_MIRRORS
> not reflected in the GUI) should be quite easy :)

Hi Belen,

I talked to Paul and apparently adding PATH doesn't really work. However, I made a few more tests and it turns out that it is actually possible to use SSTATE_MIRRORS with a single line like the one below:

SSTATE_MIRRORS ?= "file://(.*) ftp://10.237.112.100/sstate-cache/\1"

So we're back to where we started. Personally though, I agree with Cristiana's comments. It seems a bit weird to use a "browse files" GUI button for a text entry element that can hold arbitrary URLs. I'd still leave the text entry element as-is, without any additional button and let the user edit it manually, with some old-fashioned typing :)

Thanks,
Bogdan

(In reply to comment #9)
> Hi Bogdan,
> 
> Thanks for the information. I am a bit confused now: just had a brief chat
> with Paul E about this, and he believes that you don't need two entries. You
> just need to add 'PATH' to the SSTATE_MIRRORS URL and that will append the
> appropriate distro-specific path for native recipes. 
> 
> If the above is correct, we should probably add 'PATH' to the URL entered in
> the SSTATE_MIRROR field behind the scenes. We would then use the info icon
> to explain that mirrors must have the correct directory structure, and point
> users to the section of the manual where they can find more information
> about this. 
> 
> Before I run away to redesign the Settings dialogue, maybe we should confirm
> Paul's comments. What do you think?
> 
> Cheers
> 
> Belen
> 
> 
> 
> (In reply to comment #7)
> > Hi Belen,
> > 
> > (In reply to comment #3)
> > > Hi Cristiana,
> > > 
> > > You are right: it needs to be a valid URL. But it can be a URL pointing to a
> > > local or remote resource. The easiest way of inputting a local resource is
> > > using a file selection dialog, and that's why the interface is the way it
> > > is.
> > 
> > I understand your comments, but unfortunately I don't think this is enough.
> > The main problem is that SSTATE_MIRRORS can have more than one entry (and
> > with the last changes it apparently _needs_ to have more than one entry, one
> > for native packages and one for everything else). For example, my
> > SSTATE_MIRRORS looks like this now:
> > 
> > SSTATE_MIRRORS ?= "\
> > file://Ubuntu-12.04/(.*) ftp://10.237.112.100/sstate-cache/Ubuntu-12.04/\1
> > \n \
> > file://(.*) ftp://10.237.112.100/sstate-cache/\1 \n \
> > "
> > (the first line is needed for native packages, the second one for everything
> > else).
> > 
> > With this in mind, the file selection dialog is no longer an option IMO. The
> > easy way to fix this is to let the user enter the content of SSTATE_MIRROS
> > manually, without any dialogs. If we want to get more user-friendly,
> > SSTATE_MIRRORS should probably get its own dialog that would have a list
> > with the current SSTATE_MIRRORS entries and buttons for "Add new URL", "Add
> > new local dir", "Remove entry". But note that the user-friendly part is
> > further complicated by the contents of SSTATE_MIRRORS. Take for example the
> > first part of SSTATE_MIRRORS above:
> > 
> > file://Ubuntu-12.04/(.*) ftp://10.237.112.100/sstate-cache/Ubuntu-12.04/\1
> > 
> > Note that this is actually a regexp (the '\1' at the end takes its value
> > from the '(.*)' after the file:// URL). This is a very flexible mechanism
> > and unfortunately quite hard to capture in a GUI. 
> > In conclusion, I'd make SSTATE_MIRRORS a simple text entry widget, without
> > any associated file selection dialog and let the user have full control over
> > it. Validating its contents might be yet another challenge, due to the issue
> > explained above.
> > There are some good news too: the fix for the original bug (SSTATE_MIRRORS
> > not reflected in the GUI) should be quite easy :)

Ah, the proverbial circle ;) Don't worry: we go around all the time. 

Apparently, PATH does work for the same reason your single line solution works. But I'll let you discuss that with Paul. 

I'd like to find a way of presenting the mirrors that helps users enter the right input in the right format. This might require something a bit more sophisticated than a simple text input field (we can always fall back to that). 

It might take a little longer than I thought, but I'll try to send you some mockups today and we can then discuss if they would work. 

In the meantime, I've changed the status of the bug to NEEDINFO.

Belen

(In reply to comment #10)
> Hi Belen,
> 
> I talked to Paul and apparently adding PATH doesn't really work. However, I
> made a few more tests and it turns out that it is actually possible to use
> SSTATE_MIRRORS with a single line like the one below:
> 
> SSTATE_MIRRORS ?= "file://(.*) ftp://10.237.112.100/sstate-cache/\1"
> 
> So we're back to where we started. Personally though, I agree with
> Cristiana's comments. It seems a bit weird to use a "browse files" GUI
> button for a text entry element that can hold arbitrary URLs. I'd still
> leave the text entry element as-is, without any additional button and let
> the user edit it manually, with some old-fashioned typing :)
> 
> Thanks,
> Bogdan
> 
> (In reply to comment #9)
> > Hi Bogdan,
> > 
> > Thanks for the information. I am a bit confused now: just had a brief chat
> > with Paul E about this, and he believes that you don't need two entries. You
> > just need to add 'PATH' to the SSTATE_MIRRORS URL and that will append the
> > appropriate distro-specific path for native recipes. 
> > 
> > If the above is correct, we should probably add 'PATH' to the URL entered in
> > the SSTATE_MIRROR field behind the scenes. We would then use the info icon
> > to explain that mirrors must have the correct directory structure, and point
> > users to the section of the manual where they can find more information
> > about this. 
> > 
> > Before I run away to redesign the Settings dialogue, maybe we should confirm
> > Paul's comments. What do you think?
> > 
> > Cheers
> > 
> > Belen
> > 
> > 
> > 
> > (In reply to comment #7)
> > > Hi Belen,
> > > 
> > > (In reply to comment #3)
> > > > Hi Cristiana,
> > > > 
> > > > You are right: it needs to be a valid URL. But it can be a URL pointing to a
> > > > local or remote resource. The easiest way of inputting a local resource is
> > > > using a file selection dialog, and that's why the interface is the way it
> > > > is.
> > > 
> > > I understand your comments, but unfortunately I don't think this is enough.
> > > The main problem is that SSTATE_MIRRORS can have more than one entry (and
> > > with the last changes it apparently _needs_ to have more than one entry, one
> > > for native packages and one for everything else). For example, my
> > > SSTATE_MIRRORS looks like this now:
> > > 
> > > SSTATE_MIRRORS ?= "\
> > > file://Ubuntu-12.04/(.*) ftp://10.237.112.100/sstate-cache/Ubuntu-12.04/\1
> > > \n \
> > > file://(.*) ftp://10.237.112.100/sstate-cache/\1 \n \
> > > "
> > > (the first line is needed for native packages, the second one for everything
> > > else).
> > > 
> > > With this in mind, the file selection dialog is no longer an option IMO. The
> > > easy way to fix this is to let the user enter the content of SSTATE_MIRROS
> > > manually, without any dialogs. If we want to get more user-friendly,
> > > SSTATE_MIRRORS should probably get its own dialog that would have a list
> > > with the current SSTATE_MIRRORS entries and buttons for "Add new URL", "Add
> > > new local dir", "Remove entry". But note that the user-friendly part is
> > > further complicated by the contents of SSTATE_MIRRORS. Take for example the
> > > first part of SSTATE_MIRRORS above:
> > > 
> > > file://Ubuntu-12.04/(.*) ftp://10.237.112.100/sstate-cache/Ubuntu-12.04/\1
> > > 
> > > Note that this is actually a regexp (the '\1' at the end takes its value
> > > from the '(.*)' after the file:// URL). This is a very flexible mechanism
> > > and unfortunately quite hard to capture in a GUI. 
> > > In conclusion, I'd make SSTATE_MIRRORS a simple text entry widget, without
> > > any associated file selection dialog and let the user have full control over
> > > it. Validating its contents might be yet another challenge, due to the issue
> > > explained above.
> > > There are some good news too: the fix for the original bug (SSTATE_MIRRORS
> > > not reflected in the GUI) should be quite easy :)

Created attachment 789 [details]
shared_state_tab.pdf

Have a look to the 'shared_state_tab.pdf' document I've attached, although it might make no sense or be too much effort. 

In any case, let me know what you think.

Belen

Hello Belen,

The tab looks ok, but I think we don't need "Test" button. I will start to implement it.

Cristiana

(In reply to comment #12)
> Created attachment 789 [details]
> shared_state_tab.pdf
> 
> Have a look to the 'shared_state_tab.pdf' document I've attached, although
> it might make no sense or be too much effort. 
> 
> In any case, let me know what you think.
> 
> Belen

Created attachment 803 [details]
shared_state_tab.pdf

Thanks, Cristiana. I am fine with not implementing the 'test' functionality. Paul wasn't very happy with how the 'Delete' and 'Add another mirror' buttons were handled, so I've updated the design to use inactive states instead (see shared_state_tab.pdf).

If you have any questions during the implementation, let me know.

Cheers

Belen

Created attachment 811 [details]
shared_state_tab.pdf

Hi Cristiana,

I've updated the design document. No major changes: just the reset button for the shared state directory and the content of the information bubble. 

If there is anything missing or have any questions, let me know.

Belen

I've tried to make a commit, but I received many conflicts. Waiting the patch for bug 3117 to be merged, and then to make the changes.

Hi Cristiana,

In the meantime, do you think I could have a look at the fix in your branch? Could you point me to it?

Thanks!!

Belen

(In reply to comment #16)
> I've tried to make a commit, but I received many conflicts. Waiting the
> patch for bug 3117 to be merged, and then to make the changes.

Hi Belen,

After I have made a git pull, I found that I have a lot of conflicts. I kept only a git diff result to know what I have to add/modifiy. I don't have it in another branch, but after settings button will works, I will fix this fast. I'm sorry.

Cristiana


(In reply to comment #17)
> Hi Cristiana,
> 
> In the meantime, do you think I could have a look at the fix in your branch?
> Could you point me to it?
> 
> Thanks!!
> 
> Belen
> 
> (In reply to comment #16)
> > I've tried to make a commit, but I received many conflicts. Waiting the
> > patch for bug 3117 to be merged, and then to make the changes.

Created attachment 836 [details]
setting shared state tab

This is an image with Shared state tab. I thought it will be better to make "add mirror" button like the others from "Others" tab. Belen what do you think?

patch merged to master

verified