the banner on the Welcome page saying that a helper is not configured should not be red

[davidsarah]

On 13/05/12 08:55, Michael Rogers wrote:

4. The client's welcome page has scary red warning about not being

connected to a helper. I wasn't sure whether I needed a helper (apparently not?).

If a helper is not configured, the banner should not be red. (It should be white or yellow or something like that.)

[warner]

yeah, I think:

• helper.furl not configured: white, or something neutral
• helper.furl configured and connected: pale green
• helper.furl configured and not connected: red

I wouldn't want to imply that Helper-less clients are somehow inferior, but at the same time if you've configured a Helper and it's not connected, then things are mildly wrong (upload performance will be lower than you'd expect).

OTOH, an introducer that isn't connected is more serious.. that should be properly red.

[kick]

The patch submitted highlights the helper banner as green if the helper is disabled. A note is left in the banner saying faster uploads can be achieved. Change the wording and styling as necessary...

[warner]

Kick: looks good! Some ideas we had:

• could you change the "not-configured" color to plain white (to match the background), instead of light green? We figured it'd be good to be as neutral as possible for something which isn't configured.
• I'd use "unconfigured" instead of "disabled", in general (since "disabled" kind of implies that using a Helper is the normal state, whereas actually Helpers are somewhat unusual).
• I'd recommend leaving out the "no, enable for quicker uploads" line: the actual benefit of the helper will depend upon having a helper that's closer to the servers than you are, and on using encoding parameters that help (in some cases the helper will be slower than a direct upload). Simply saying "unconfigured" would be enough.
• the indentation in data_connected_to_helper_description() looks wrong.. maybe it's using tabs? All python code should use four-space indents.
• we should add a test for this. The test should local the Welcome page three times: once with no helper configured, a second time with the helper furl configured but no connection, and a third time with the helper connected. test_web.py is the place to add it, in the existing Web.test_welcome case (around line 526). The test will need a little more support in FakeUploader (line 102), to return e.g. self.helper_furl and self.helper_connected instead of always returning (None, False). Then the test case can reach into self.s.uploader and set helper_furl/helper_connected between the different cases. You'll need to follow the pattern in test_welcome(): self.GET() returns a Deferred, so you'll add a helper function (that scans the HTML output for "Connected to helper?: unconfigured", etc) through a d.addCallback.

Grab any of us (warner, zooko, davidsarah) on IRC if you need a hand with the test.

thanks!

-Brian

[kick1]

Added new attachment with suggestions. Having the background white with no border looks a bit off. Maybe spacing can help?

[warner]

I'll try to land this over the coming weekend.

[davidsarah]

Taking this over.

[davidsarah]

I added a grey (#CCC) 1-pixel border around the "not configured" box, fixed the trailing whitespace, and made some minor coding style cleanups to the tests.

The patch is not working when I smoke-test it, though; it's still displaying "no" for my gateway that doesn't have a helper configured, rather than "not configured". Investigating.

[davidsarah]

Replying to davidsarah:

The patch is not working when I smoke-test it, though; it's still displaying "no" for my gateway that doesn't have a helper configured, rather than "not configured". Investigating.

The problem was that I had "helper.furl =" rather than "helper.furl = None", or no "helper.furl" line.

I decided to normalize the furl to None in client.py, and add a test that the normalization is done correctly.

Pull request at ​https://github.com/tahoe-lafs/tahoe-lafs/pull/27, review needed.

[davidsarah]

Committed in bf0e7ce3cc4a39a7. Leaving open for review.

[zooko]

​https://github.com/davidsarah/tahoe-lafs/commit/a5895b9d59212ab1a000b6e1d099cad4fe630c4d#commitcomment-2546443