Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Modal fixes (#281) #284

Merged
merged 6 commits into from
Dec 22, 2021
Merged

Modal fixes (#281) #284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7955ca5
Do download error modals in a goroutine [#281]
awfulcooking Dec 12, 2021
08c1d65
Do clipboard/copy error modals in a goroutine [#281]
awfulcooking Dec 12, 2021
26407d8
Do followLink in a new goroutine, update its doc [#281]
awfulcooking Dec 12, 2021
98a6550
Make Info() block, update modal func docs, refactor var declaration […
awfulcooking Dec 12, 2021
1d45c79
Do dlChoice modal in a blocking fashion [fixes #283]
awfulcooking Dec 12, 2021
a7c27c6
Do the other dlChoice calls blocking, too [really fixes #283]
awfulcooking Dec 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion display/display.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ func Init(version, commit, builtBy string) {
}
if i <= len(tabs[tab].page.Links) && i > 0 {
// It's a valid link number
followLink(tabs[tab], tabs[tab].page.URL, tabs[tab].page.Links[i-1])
go followLink(tabs[tab], tabs[tab].page.URL, tabs[tab].page.Links[i-1])
return
}
// Invalid link number, don't do anything
Expand Down
8 changes: 4 additions & 4 deletions display/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,15 +352,15 @@ func handleURL(t *tab, u string, numRedirects int) (string, bool) {
// Disable read timeout and go back to start
res.SetReadTimeout(0) //nolint: errcheck
res.Body.(*rr.RestartReader).Restart()
go dlChoice("That page is too large. What would you like to do?", u, res)
dlChoice("That page is too large. What would you like to do?", u, res)
return ret("", false)
}
if errors.Is(err, renderer.ErrTimedOut) {
// Downloading now
// Disable read timeout and go back to start
res.SetReadTimeout(0) //nolint: errcheck
res.Body.(*rr.RestartReader).Restart()
go dlChoice("Loading that page timed out. What would you like to do?", u, res)
dlChoice("Loading that page timed out. What would you like to do?", u, res)
return ret("", false)
}
if err != nil {
Expand Down Expand Up @@ -493,7 +493,7 @@ func handleURL(t *tab, u string, numRedirects int) (string, bool) {
// Disable read timeout and go back to start
res.SetReadTimeout(0) //nolint: errcheck
res.Body.(*rr.RestartReader).Restart()
go dlChoice("That file could not be displayed. What would you like to do?", u, res)
dlChoice("That file could not be displayed. What would you like to do?", u, res)
}
}()
return ret("", false)
Expand All @@ -503,6 +503,6 @@ func handleURL(t *tab, u string, numRedirects int) (string, bool) {
// Disable read timeout and go back to start
res.SetReadTimeout(0) //nolint: errcheck
res.Body.(*rr.RestartReader).Restart()
go dlChoice("That file could not be displayed. What would you like to do?", u, res)
dlChoice("That file could not be displayed. What would you like to do?", u, res)
return ret("", false)
}
32 changes: 16 additions & 16 deletions display/modals.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,21 @@ import (
// The bookmark modal is in bookmarks.go

var infoModal = cview.NewModal()

var errorModal = cview.NewModal()
var errorModalDone = make(chan struct{})

var inputModal = cview.NewModal()
var inputCh = make(chan string)
var inputModalText string // The current text of the input field in the modal

var yesNoModal = cview.NewModal()

// Channel to receive yesNo answer on
var inputCh = make(chan string)
var yesNoCh = make(chan bool)

var inputModalText string // The current text of the input field in the modal

// Internal channel used to know when a modal has been dismissed
var modalDone = make(chan struct{})

func modalInit() {
infoModal.AddButtons([]string{"Ok"})

errorModal.AddButtons([]string{"Ok"})

yesNoModal.AddButtons([]string{"Yes", "No"})

panels.AddPanel(PanelInfoModal, infoModal, false, false)
Expand Down Expand Up @@ -145,6 +142,7 @@ func modalInit() {
panels.HidePanel(PanelInfoModal)
App.SetFocus(tabs[curTab].view)
App.Draw()
modalDone <- struct{}{}
})

errorModal.SetBorder(true)
Expand All @@ -153,7 +151,7 @@ func modalInit() {
panels.HidePanel(PanelErrorModal)
App.SetFocus(tabs[curTab].view)
App.Draw()
errorModalDone <- struct{}{}
modalDone <- struct{}{}
})

inputModal.SetBorder(true)
Expand Down Expand Up @@ -183,7 +181,7 @@ func modalInit() {
dlInit()
}

// Error displays an error on the screen in a modal.
// Error displays an error on the screen in a modal, and blocks until dismissed by the user.
func Error(title, text string) {
if text == "" {
text = "No additional information."
Expand All @@ -203,19 +201,21 @@ func Error(title, text string) {
App.SetFocus(errorModal)
App.Draw()

<-errorModalDone
<-modalDone
}

// Info displays some info on the screen in a modal.
// Info displays some info on the screen in a modal, and blocks until dismissed by the user.
func Info(s string) {
infoModal.SetText(s)
panels.ShowPanel(PanelInfoModal)
panels.SendToFront(PanelInfoModal)
App.SetFocus(infoModal)
App.Draw()

<-modalDone
}

// Input pulls up a modal that asks for input, and returns the user's input.
// Input pulls up a modal that asks for input, waits for that input, and returns it.
// It returns an bool indicating if the user chose to send input or not.
func Input(prompt string, sensitive bool) (string, bool) {
// Remove elements and re-add them - to clear input text and keep input in focus
Expand Down Expand Up @@ -257,7 +257,7 @@ func Input(prompt string, sensitive bool) (string, bool) {
return resp, true
}

// YesNo displays a modal asking a yes-or-no question.
// YesNo displays a modal asking a yes-or-no question, waits for an answer, then returns it as a bool.
func YesNo(prompt string) bool {
if viper.GetBool("a-general.color") {
m := yesNoModal
Expand Down Expand Up @@ -289,7 +289,7 @@ func YesNo(prompt string) bool {
}

// Tofu displays the TOFU warning modal.
// It returns a bool indicating whether the user wants to continue.
// It blocks then returns a bool indicating whether the user wants to continue.
func Tofu(host string, expiry time.Time) bool {
// Reuses yesNoModal, with error color

Expand Down
28 changes: 17 additions & 11 deletions display/private.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
package display

// This file contains the functions that aren't part of the public API.
// The funcs are for network and displaying.

import (
"net/url"
"strconv"
Expand All @@ -9,17 +12,20 @@ import (
"github.com/makeworld-the-better-one/amfora/structs"
)

// This file contains the functions that aren't part of the public API.
// The funcs are for network and displaying.

// followLink should be used when the user "clicks" a link on a page.
// Not when a URL is opened on a new tab for the first time.
// It will handle setting the bottomBar.
// followLink should be used when the user "clicks" a link on a page,
// but not when a URL is opened on a new tab for the first time.
//
// It will handle updating the bottomBar.
//
// It should be called with the `go` keyword to spawn a new goroutine if
// it would otherwise block the UI loop, such as when called from an input
// handler.
//
// It blocks until navigation is finished, and we've completed any user
// interaction related to loading the URL (such as info, error modals)
func followLink(t *tab, prev, next string) {
if strings.HasPrefix(next, "about:") {
if final, ok := handleAbout(t, next); ok {
t.addToHistory(final)
}
goURL(t, next)
return
}

Expand All @@ -29,7 +35,7 @@ func followLink(t *tab, prev, next string) {
Error("URL Error", err.Error())
return
}
go goURL(t, nextURL)
goURL(t, nextURL)
return
}
// No content on current tab, so the "prev" URL is not valid.
Expand All @@ -39,7 +45,7 @@ func followLink(t *tab, prev, next string) {
Error("URL Error", "Link URL could not be parsed")
return
}
go goURL(t, next)
goURL(t, next)
}

// reformatPage will take the raw page content and reformat it according to the current terminal dimensions.
Expand Down
16 changes: 8 additions & 8 deletions display/tab.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func makeNewTab() *tab {
linkN, _ := strconv.Atoi(currentSelection[0])
tabs[tab].page.Selected = tabs[tab].page.Links[linkN]
tabs[tab].page.SelectedID = currentSelection[0]
followLink(tabs[tab], tabs[tab].page.URL, tabs[tab].page.Links[linkN])
go followLink(tabs[tab], tabs[tab].page.URL, tabs[tab].page.Links[linkN])
return
}
if len(currentSelection) == 0 && (key == tcell.KeyEnter || key == tcell.KeyTab) {
Expand Down Expand Up @@ -156,12 +156,12 @@ func makeNewTab() *tab {
if t.hasContent() {
savePath, err := downloadPage(t.page)
if err != nil {
Error("Download Error", fmt.Sprintf("Error saving page content: %v", err))
go Error("Download Error", fmt.Sprintf("Error saving page content: %v", err))
Copy link
Owner

@makew0rld makew0rld Dec 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My problem with this, and all the other go <modal> calls in this file is that it opens up the possibility of other things happening before the modal is dismissed. I'm not sure what the solution is though. Maybe all the code in t.view.SetInputCapture can be put in a goroutine (with a mutex that blocks multiple versions of it running) that blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a good situation to ask about in #go-nuts on Libera. I wonder if they'd have any advice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bear in mind the Info() and Error() functions previously just didn't block, but would return as soon as the modal had opened. Using go Info() should give similar behaviour to before, the only difference being that now the parent function may return before the modal has even rendered.

But if anything happened immediately after it the modals rendered, it would have shown UI breakage before #272, I think. And so we could expect this just to be a return to the previous, non-deadlocking behaviour

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun that we're addressing deadlocking and not-waiting-enough code in one PR :-)
As two sides of one coin.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bear in mind the Info() and Error() functions previously just didn't block, but would return as soon as the modal had opened. Using go Info() should give similar behaviour to before, the only difference being that now the parent function may return before the modal has even rendered.

Yep, and that behavior worked well enough in general, so maybe it's fine. I'm just cautiious about future issues caused by this being non-blocking. But it's not a reason to not merge this PR or anything.

But if anything happened immediately after it the modals rendered, it would have shown UI breakage before #272

What makes you say this? Like it would draw focus away from the modal? And how does this PR change that? Seems like it could easily break things if something happened after still.

} else {
Info(fmt.Sprintf("Page content saved to %s. ", savePath))
go Info(fmt.Sprintf("Page content saved to %s. ", savePath))
}
} else {
Info("The current page has no content, so it couldn't be downloaded.")
go Info("The current page has no content, so it couldn't be downloaded.")
}
return nil
case config.CmdBack:
Expand All @@ -178,7 +178,7 @@ func makeNewTab() *tab {
currentURL := tabs[curTab].page.URL
err := clipboard.WriteAll(currentURL)
if err != nil {
Error("Copy Error", err.Error())
go Error("Copy Error", err.Error())
return nil
}
return nil
Expand All @@ -193,14 +193,14 @@ func makeNewTab() *tab {
if err != nil {
err := clipboard.WriteAll(selectedURL)
if err != nil {
Error("Copy Error", err.Error())
go Error("Copy Error", err.Error())
return nil
}
return nil
}
err = clipboard.WriteAll(copiedURL.String())
if err != nil {
Error("Copy Error", err.Error())
go Error("Copy Error", err.Error())
return nil
}
return nil
Expand All @@ -209,7 +209,7 @@ func makeNewTab() *tab {
if cmd >= config.CmdLink1 && cmd <= config.CmdLink0 {
if int(cmd) <= len(t.page.Links) {
// It's a valid link number
followLink(&t, t.page.URL, t.page.Links[cmd-1])
go followLink(&t, t.page.URL, t.page.Links[cmd-1])
return nil
}
}
Expand Down