Network test popup #36
Conversation
| var ( | ||
| // NetworkPassing is used to track if the latest network configuration | ||
| // is passing; changes in proxy, etc. | ||
| NetworkPassing bool |
There was a problem hiding this comment.
we could use the model to store this flag, we don't even need to export it anyway.
There was a problem hiding this comment.
@dorileo But is "the network working" really part of the model of the install? To me, it was a status of something during execution. But if you feel strongly I can relocate it.
| if dialog.RunNetworkTest() { | ||
| // Automatically close if it worked | ||
| clui.RefreshScreen() | ||
| time.Sleep(time.Second) |
There was a problem hiding this comment.
Do you really need this sleep here?
There was a problem hiding this comment.
It was only there to give the user a "second" to see the Success status before the dialog automatically goes away. I added it because it bothered me that the popup went away too quickly. If you have a strong feeling about this, we can remove it.
| // the progress bar to "full" | ||
| func (dialog *NetworkTestDialog) Success() { | ||
| dialog.progressBar.SetValue(dialog.progressMax) | ||
| clui.RefreshScreen() |
There was a problem hiding this comment.
You're refreshing the screen twice (here and in the menu callback), is that intentional?
There was a problem hiding this comment.
Who doesn't like a Fresh a screen? :) I can remove one of these and retest.
There was a problem hiding this comment.
@dorileo We are also doing a refresh after SetValue in the tui/installer code and after SetTitle; this seems to be the BKM pattern, no?
| time.Sleep(flashTime) | ||
| dialog.progressBar.SetBackColor(bg) | ||
| clui.RefreshScreen() | ||
| time.Sleep(flashTime) |
There was a problem hiding this comment.
sleeping and refreshing twice?
There was a problem hiding this comment.
This is the "Flashing" between Red and the default color to make the failure obvious to the user. We have the same code in Failure code for the Progress bar during the installation code.
| // Desc is part of the progress.Client implementation and sets the progress bar label | ||
| func (dialog *NetworkTestDialog) Desc(desc string) { | ||
| dialog.resultLabel.SetTitle(desc) | ||
| clui.RefreshScreen() |
There was a problem hiding this comment.
Also refreshing? are you running this dialog in a different goroutine?
There was a problem hiding this comment.
This refresh after setting the Title was imitated from the Dialog window code in clui.
| // OnClose sets the callback that is called when the | ||
| // dialog is closed | ||
| func (dialog *NetworkTestDialog) OnClose(fn func()) { | ||
| clui.WindowManager().BeginUpdate() |
There was a problem hiding this comment.
You shouldn't need this bend and end update pairs, specially here.
There was a problem hiding this comment.
This Begin and End was imitated from the Dialog window code in clui.
|
|
||
| // Close closes the dialog window and executes a callback if registered | ||
| func (dialog *NetworkTestDialog) Close() { | ||
| clui.WindowManager().DestroyWindow(dialog.DialogBox) |
| dialog.DialogBox.SetTitleButtons(0) | ||
| dialog.DialogBox.SetMovable(false) | ||
| dialog.DialogBox.SetSizable(false) | ||
| clui.WindowManager().BeginUpdate() |
There was a problem hiding this comment.
This is how it is code in the PopUp dialog code in clui.
|
@mdhorn all those RefreshScreen() calls might have been inherited from my original code, I think it's a good opportunity to review'em. |
|
@mdhorn I've just tested the patches and it breaks when resizing terminal. If you hit "test network" and resize the terminal then the TUI window is not properly placed/moved, I also saw some window corruption. |
|
if we fail the testing after changing the proxy settings we shouldn't revert the user's change. |
|
if we fail the testing after changing the network settings we shouldn't revert the user's change. |
|
@dorileo After testing proxy settings, if testing fails, we do reset the proxy to the previous value. |
mdhorn
force-pushed
the
network-test-popup
branch
2 times, most recently
from
a24db1a
to
3f3510c
Compare
|
@dorileo Can you please re-review now that I have pushed changes. |
Switch to using a model PopUp window for Network testing. This should be re-used for all network testing: - Manual - Change of Proxy - Change of Interface - Before starting Install Added URL checking to the Proxy setting Signed-off-by: Mark D Horn <mark.d.horn@intel.com>
mdhorn
force-pushed
the
network-test-popup
branch
from
3f3510c
to
866b926
Compare
Fixes Issue: #27
Changes proposed in this pull request: