Add bwtester dials and tabs GUI by mwfarb · Pull Request #37 · perrig/scionlab

mwfarb · 2018-05-30T05:51:40Z

Added tabs for each app.
Moved client/server input into color static svg/inputs.
Moved css and js into separate files.
Added color arrows showing directionality of tests/content.
Moved console output into scrollable window.
Added bwtester tab with dials for each parameter/direction.
Fixed bwtester command arg formatting issue.
Added bwtester radio icon lock for each parameter.
Added bwtester feedback to alter non-locked dials on change.
Added bwtester feedback when change would exceed max on non-locked
dial.
Added camerapp content over arrow, scaled to fit.
Added camerapp ability to click and expand image.
Added button disable when app is running.
Updated readme with new directions and screenshots.
Added tooltip note over args input about formatting.

mwfarb · 2018-05-30T06:01:36Z

@FR4NK-W @chaehni Let me know how this works for you.

chaehni · 2018-05-30T09:34:28Z

@mwfarb It works really well! Some feedback/ideas for improvement:

the packets dial doesn't seem to work properly for me. When I lock the packet size and try to turn the packets dial it shows huge numbers and then springs back to its original value.
When I click the image on the camerapp tab the image does not expand.
When running the bwtester, in the console the app prints the server-client direction as extra string while the client-server direction is not printed as extra string. Is this intended?
Can you add some form validation for the client/server addresses to the web interface?

mwfarb · 2018-05-30T14:23:03Z

@chaehni Thanks for taking a look and finding these issues.

The packets dial is interesting since it's essentially a potentially large range, so entering large numbers will almost always cause, in your example, bw to far exceed the max 150 Mbps, and so I have it restoring packets to its previous value. I could add a overlay-ed red error message that would fade explaining briefly when chaging a dial exceeds the bounds of the algebra.
The camerapp link appears to work in Firefox, but not Chrome with this error in the console "Not allowed to navigate top frame to data URL". I'll choose another method that is more cross-platform.
If you mean the webapp terminal logging printing %!(EXTRA string=-sc=3,1000,30,80000bps, string=). I'm not sure why, but giving golang's exec.Command() more than 5 args results with this logging, but seems to take the "extra" args just fine. I'll see if I can find a way to remove it.
The html includes field validation, but the Execute button needs to be fixed to validate it.

mwfarb · 2018-06-06T02:25:10Z

@chaehni I've addressed your comments and fixed a few other issues. Let me know if you have time for another look and review. Thanks!

mwfarb · 2018-06-08T02:44:39Z

@chaehni Sorry to add another commit to your review. The scionlab environment appears not to have $GOBIN set or run go install on setup, so the scionlab apps may not be compiled for new users. Also, I was mistakenly launching go run for each app run, causing extra compilation time on each run. The majority of this change is to ensure the binaries build on startup, and using the binary only on each app run.

chaehni · 2018-06-10T13:00:35Z

@mwfarb I had another look and my comments from above are addressed.
One idea: Can we limit the range of the dials based on the locked value and the limit of the remaining dial such that it cannot exceed any limits?
So if we, for example, lock the packet size the packets dial would be limited to a value that does not exceed the bandwidth limit of 150mbps. Otherwise, it's really hard to set a value for the packets via the dial.

I like that the apps are now installed instead of compiled for every run. I talked to Juan and we want to pre-install the apps in the VMs and run your webapp automatically. So we might only need to check if the apps are installed. (see netsec-ethz/scion-coord#228)

mwfarb · 2018-06-11T14:12:12Z

The knob library has a non-standard way of displaying the text input box. Have you already tried to click on the number of packets and type in the number you want? If that is the root of setting the number of packets, I should change the display to look more like a standard input box.

chaehni · 2018-06-13T17:01:57Z

I didn't know you can enter it like this.
That makes it a lot more convenient!

mwfarb · 2018-06-14T05:35:39Z

@chaehni I'd like to update the dial visually in another PR. Ultimately I'd like hash marks for the range of values on the dial, and an input box that is more intuitive to edit. For now, I've added a line of instruction above the dials making it clear that numbers can be typed, edited, clicked, or scrolled to change. Integrating a new dial library now may drag use of other improvements out more weeks.

chaehni

I went through the code and left some minor comments.
Also, I can not see where the timeDurationEst() function is used.

chaehni · 2018-06-14T09:14:59Z

webapp/webapp.go

+}
+
+// Queries network interfaces and writes local client SCION addresses as json
+func genClientNodeDefults(cli_fp string) {


typo: ...Defults

chaehni · 2018-06-14T09:55:07Z

webapp/webapp.go

+}
+
 // Handles loading index.html for user at root.
 func mainHandler(w http.ResponseWriter, r *http.Request) {


In case of an error reading the index.html there is no response back to the browser. The request just times out.

The same happens in the image handler when the file is not found. I think we should respond with an appropriate error code.

chaehni · 2018-06-14T10:28:31Z

@mwfarb Thanks for your work. I think the instructions are good enough for now. We don't need to change the dials in this PR.

mwfarb · 2018-06-14T14:04:32Z

OK, sounds good. I've removed the unused code and added the error messaging.

chaehni · 2018-06-15T06:21:55Z

webapp/webapp.go

    data, err := ioutil.ReadFile(filepath)
    if err != nil {
        log.Fatal("ioutil.ReadFile() error: " + err.Error())
+        fmt.Fprint(w, "Error: Unable to read "+filepath)


We should return here to ensure we don't have multiple writes to w.
Also, we could use the convenience function Error() to send the error. (https://golang.org/pkg/net/http/#Error)

Great! Thanks for the tip to use http.Error() as well. Let me know if there are any more issues before merging.

Looks good to me.
I think we can merge it.

- Added tabs for each app. - Moved client/server input into color static svg/inputs. - Moved css and js into separate files. - Added color arrows showing directionality of tests/content. - Moved console output into scrollable window. - Added bwtester tab with dials for each parameter/direction. - Fixed bwtester command arg formatting issue. - Added bwtester radio icon lock for each parameter. - Added bwtester feedback to alter non-locked dials on change. - Added bwtester feedback when change exceeds max on non-locked dial. - Added camerapp content over arrow, scaled to fit. - Added camerapp ability to click and expand image. - Added button disable when app is running. - Updated readme with new directions and screenshots. - Added tooltip note over args input about formatting. - Added red error message when user exceeds dial limits. - Restored html field validation for client and server. - Added select box preloading of known servers for each app. - Added select box preloading of local client network interfaces. - Adjusted default bind addresses to 0.0.0.0 where needed. - Mitgated issue with stale cached file browsing directory in Chrome. - Add go-watcher notes to readme. - Added install check for all apps on startup, prevents recompiling - Adjusted app calls to run locally. - Removed redundant client/server tests, which... - Resolved issue #36. - Updated readme with new build instructions. - Added bwtester instruction line to help editing understanding. - Addressed comments/code reviews.

mwfarb · 2018-06-19T14:24:56Z

@chaehni @perrig All commits and comments have been squashed into one. Ready for merging.

chaehni reviewed Jun 14, 2018

View reviewed changes

mwfarb mentioned this pull request Jun 14, 2018

Update bwtester dials #40

Open

chaehni reviewed Jun 15, 2018

View reviewed changes

perrig merged commit 7247b05 into perrig:master Jun 19, 2018

mwfarb deleted the dials branch July 5, 2018 17:31

Conversation

mwfarb commented May 30, 2018

Uh oh!

mwfarb commented May 30, 2018

Uh oh!

chaehni commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mwfarb commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mwfarb commented Jun 6, 2018

Uh oh!

mwfarb commented Jun 8, 2018

Uh oh!

chaehni commented Jun 10, 2018

Uh oh!

mwfarb commented Jun 11, 2018

Uh oh!

chaehni commented Jun 13, 2018

Uh oh!

mwfarb commented Jun 14, 2018

Uh oh!

chaehni left a comment

Choose a reason for hiding this comment

Uh oh!

chaehni Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

chaehni Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

chaehni Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

chaehni commented Jun 14, 2018

Uh oh!

mwfarb commented Jun 14, 2018

Uh oh!

chaehni Jun 15, 2018

Choose a reason for hiding this comment

Uh oh!

mwfarb Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

chaehni Jun 19, 2018

Choose a reason for hiding this comment

Uh oh!

mwfarb commented Jun 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chaehni commented May 30, 2018 •

edited

Loading

mwfarb commented May 30, 2018 •

edited

Loading