perrig / scionlab

Software for supporting SCIONLab
Apache License 2.0
3 stars 7 forks source link

Add bwtester dials and tabs GUI #37

Closed mwfarb closed 6 years ago

mwfarb commented 6 years ago
mwfarb commented 6 years ago

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

chaehni commented 6 years ago

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

mwfarb commented 6 years ago

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

mwfarb commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 https://github.com/netsec-ethz/scion-coord/issues/228)

mwfarb commented 6 years ago

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 commented 6 years ago

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

mwfarb commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

mwfarb commented 6 years ago

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