From be09ffcf913662983a0362431d8e4765d06b4187 Mon Sep 17 00:00:00 2001 From: makeworld Date: Mon, 6 Jul 2020 20:30:54 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9A=A7=20Switch=20to=20using=20tab=20poin?= =?UTF-8?q?ters=20instead=20of=20ints?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Almost finished overall work. --- CHANGELOG.md | 1 + NOTES.md | 3 - display/bookmarks.go | 6 +- display/display.go | 50 +++++++------ display/history.go | 40 +++++----- display/private.go | 170 +++++++++++++++++++++++++------------------ display/tab.go | 2 +- 7 files changed, 150 insertions(+), 122 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a5eb998..7e2a8d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Many potential network and display race conditions eliminated - Whether a tab is loading stays indicated when you switch away from it and go back +- Plain text documents are displayed faithfully (there were some edge conditions) ## [1.2.0] - 2020-07-02 ### Added diff --git a/NOTES.md b/NOTES.md index b241f04..efd38c5 100644 --- a/NOTES.md +++ b/NOTES.md @@ -1,12 +1,9 @@ # Notes - URL for each tab should not be stored as a string - in the current code there's lots of reparsing the URL -- Switch to UUIDs for each tab maybe? So that if `handleURL` completes after the tab is closed (and reopened) it doesn't go anywhere - - New UUID every time a new page is loaded? ## Bugs - Can't go back or do other things while page is loading - need a way to stop `handleURL` -- `handleURL` will reference a tab that doesn't exist and cause a panic if the tab is closed ## Upstream Bugs - Wrapping messes up on brackets diff --git a/display/bookmarks.go b/display/bookmarks.go index b4c94dd..a690cb1 100644 --- a/display/bookmarks.go +++ b/display/bookmarks.go @@ -96,7 +96,7 @@ func openBkmkModal(name string, exists bool) (string, int) { } // Bookmarks displays the bookmarks page on the current tab. -func Bookmarks(tab int) { +func Bookmarks(t *tab) { // Gather bookmarks rawContent := "# Bookmarks\r\n\r\n" m, keys := bookmarks.All() @@ -113,8 +113,8 @@ func Bookmarks(tab int) { Width: termW, Mediatype: structs.TextGemini, } - setPage(curTab, &page) - tabs[tab].applyBottomBar() + setPage(t, &page) + t.applyBottomBar() } // addBookmark goes through the process of adding a bookmark for the current page. diff --git a/display/display.go b/display/display.go index a4edbed..43ed6c5 100644 --- a/display/display.go +++ b/display/display.go @@ -65,7 +65,7 @@ var layout = cview.NewFlex(). var renderedNewTabContent string var newTabLinks []string -var newTabPage *structs.Page +var newTabPage structs.Page var App = cview.NewApplication(). EnableMouse(false). @@ -76,12 +76,12 @@ var App = cview.NewApplication(). termH = height // Make sure the current tab content is reformatted when the terminal size changes - go func(tab int) { - tabs[tab].reformatMut.Lock() // Only one reformat job per tab - defer tabs[tab].reformatMut.Unlock() + go func(t *tab) { + t.reformatMut.Lock() // Only one reformat job per tab + defer t.reformatMut.Unlock() // Use the current tab, but don't affect other tabs if the user switches tabs - reformatPageAndSetView(tab, tabs[tab].page) - }(curTab) + reformatPageAndSetView(t, t.page) + }(tabs[curTab]) }) func Init() { @@ -180,7 +180,7 @@ func Init() { } if i <= len(tabs[tab].page.Links) && i > 0 { // It's a valid link number - followLink(tab, tabs[tab].page.Url, tabs[tab].page.Links[i-1]) + followLink(tabs[tab], tabs[tab].page.Url, tabs[tab].page.Links[i-1]) return } // Invalid link number, don't do anything @@ -199,7 +199,7 @@ func Init() { // Render the default new tab content ONCE and store it for later renderedNewTabContent, newTabLinks = renderer.RenderGemini(newTabContent, textWidth(), leftMargin()) - newTabPage = &structs.Page{ + newTabPage = structs.Page{ Raw: newTabContent, Content: renderedNewTabContent, Links: newTabLinks, @@ -230,11 +230,11 @@ func Init() { // History arrow keys if event.Modifiers() == tcell.ModAlt { if event.Key() == tcell.KeyLeft { - histBack() + histBack(tabs[curTab]) return nil } if event.Key() == tcell.KeyRight { - histForward() + histForward(tabs[curTab]) return nil } } @@ -242,7 +242,7 @@ func Init() { switch event.Key() { case tcell.KeyCtrlT: if tabs[curTab].page.Mode == structs.ModeLinkSelect { - next, err := resolveRelLink(curTab, tabs[curTab].page.Url, tabs[curTab].page.Selected) + next, err := resolveRelLink(tabs[curTab], tabs[curTab].page.Url, tabs[curTab].page.Selected) if err != nil { Error("URL Error", err.Error()) return nil @@ -260,7 +260,7 @@ func Init() { URL(viper.GetString("a-general.home")) return nil case tcell.KeyCtrlB: - Bookmarks(curTab) + Bookmarks(tabs[curTab]) tabs[curTab].addToHistory("about:bookmarks") return nil case tcell.KeyCtrlD: @@ -286,10 +286,10 @@ func Init() { Reload() return nil case "b": - histBack() + histBack(tabs[curTab]) return nil case "f": - histForward() + histForward(tabs[curTab]) return nil case "u": tabs[curTab].pageUp() @@ -383,7 +383,8 @@ func NewTab() { curTab = NumTabs() tabs = append(tabs, makeNewTab()) - setPage(curTab, newTabPage) + temp := newTabPage // Copy + setPage(tabs[curTab], &temp) // Can't go backwards, but this isn't the first page either. // The first page will be the next one the user goes to. @@ -479,7 +480,7 @@ func SwitchTab(tab int) { curTab = tab % NumTabs() // Display tab - reformatPageAndSetView(curTab, tabs[curTab].page) + reformatPageAndSetView(tabs[curTab], tabs[curTab].page) tabPages.SwitchToPage(strconv.Itoa(curTab)) tabRow.Highlight(strconv.Itoa(curTab)).ScrollToHighlight() tabs[curTab].applySelected() @@ -497,13 +498,13 @@ func Reload() { } cache.Remove(tabs[curTab].page.Url) - go func(tab int) { - handleURL(tab, tabs[tab].page.Url) // goURL is not used bc history shouldn't be added to - if tab == curTab { + go func(t *tab) { + handleURL(t, t.page.Url) // goURL is not used bc history shouldn't be added to + if t == tabs[curTab] { // Display the bottomBar state that handleURL set - tabs[tab].applyBottomBar() + t.applyBottomBar() } - }(curTab) + }(tabs[curTab]) } // URL loads and handles the provided URL for the current tab. @@ -512,12 +513,13 @@ func URL(u string) { // Some code is copied in followLink() if u == "about:bookmarks" { - Bookmarks(curTab) + Bookmarks(tabs[curTab]) tabs[curTab].addToHistory("about:bookmarks") return } if u == "about:newtab" { - setPage(curTab, newTabPage) + temp := newTabPage // Copy + setPage(tabs[curTab], &temp) return } if strings.HasPrefix(u, "about:") { @@ -525,7 +527,7 @@ func URL(u string) { return } - go goURL(curTab, u) + go goURL(tabs[curTab], u) } func NumTabs() int { diff --git a/display/history.go b/display/history.go index c7d2de7..3a8cb9f 100644 --- a/display/history.go +++ b/display/history.go @@ -1,35 +1,35 @@ package display -func histForward() { - if tabs[curTab].history.pos >= len(tabs[curTab].history.urls)-1 { +func histForward(t *tab) { + if t.history.pos >= len(t.history.urls)-1 { // Already on the most recent URL in the history return } - tabs[curTab].history.pos++ - go func(tab int) { - handleURL(tab, tabs[tab].history.urls[tabs[tab].history.pos]) // Load that position in history - tabs[tab].applyScroll() - tabs[tab].applySelected() - if tab == curTab { + t.history.pos++ + go func(tt *tab) { + handleURL(tt, tt.history.urls[tt.history.pos]) // Load that position in history + tt.applyScroll() + tt.applySelected() + if tt == tabs[curTab] { // Display the bottomBar state that handleURL set - tabs[tab].applyBottomBar() + tt.applyBottomBar() } - }(curTab) + }(t) } -func histBack() { - if tabs[curTab].history.pos <= 0 { +func histBack(t *tab) { + if t.history.pos <= 0 { // First tab in history return } - tabs[curTab].history.pos-- - go func(tab int) { - handleURL(tab, tabs[tab].history.urls[tabs[tab].history.pos]) // Load that position in history - tabs[tab].applyScroll() - tabs[tab].applySelected() - if tab == curTab { + t.history.pos-- + go func(tt *tab) { + handleURL(tt, tt.history.urls[tt.history.pos]) // Load that position in history + tt.applyScroll() + tt.applySelected() + if tt == tabs[curTab] { // Display the bottomBar state that handleURL set - tabs[tab].applyBottomBar() + tt.applyBottomBar() } - }(curTab) + }(t) } diff --git a/display/private.go b/display/private.go index c428c00..b4b1447 100644 --- a/display/private.go +++ b/display/private.go @@ -18,6 +18,16 @@ import ( // This file contains the functions that aren't part of the public API. +// isValidTab indicates whether the passed tab is still being used, even if it's not currently displayed. +func isValidTab(t *tab) bool { + for i := range tabs { + if tabs[i] == t { + return true + } + } + return false +} + func leftMargin() int { return int(float64(termW) * viper.GetFloat64("a-general.left_margin")) } @@ -51,8 +61,8 @@ func queryEscape(path string) string { // resolveRelLink returns an absolute link for the given absolute link and relative one. // It also returns an error if it could not resolve the links, which should be displayed // to the user. -func resolveRelLink(tab int, prev, next string) (string, error) { - if !tabs[tab].hasContent() { +func resolveRelLink(t *tab, prev, next string) (string, error) { + if !t.hasContent() { return next, nil } @@ -67,12 +77,12 @@ func resolveRelLink(tab int, prev, next string) (string, error) { // 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. -func followLink(tab int, prev, next string) { +func followLink(t *tab, prev, next string) { // Copied from URL() if next == "about:bookmarks" { - Bookmarks(tab) - tabs[tab].addToHistory("about:bookmarks") + Bookmarks(t) + t.addToHistory("about:bookmarks") return } if strings.HasPrefix(next, "about:") { @@ -80,14 +90,14 @@ func followLink(tab int, prev, next string) { return } - if tabs[tab].hasContent() { - tabs[tab].saveScroll() // Likely called later on, it's here just in case - nextURL, err := resolveRelLink(tab, prev, next) + if t.hasContent() { + t.saveScroll() // Likely called later on, it's here just in case + nextURL, err := resolveRelLink(t, prev, next) if err != nil { Error("URL Error", err.Error()) return } - go goURL(tab, nextURL) + go goURL(t, nextURL) return } // No content on current tab, so the "prev" URL is not valid. @@ -97,7 +107,7 @@ func followLink(tab int, prev, next string) { Error("URL Error", "Link URL could not be parsed") return } - go goURL(tab, next) + go goURL(t, next) } // reformatPage will take the raw page content and reformat it according to the current terminal dimensions. @@ -126,33 +136,38 @@ func reformatPage(p *structs.Page) { // reformatPageAndSetView is for reformatting a page that is already being displayed. // setPage should be used when a page is being loaded for the first time. -func reformatPageAndSetView(tab int, p *structs.Page) { - tabs[tab].saveScroll() +func reformatPageAndSetView(t *tab, p *structs.Page) { + t.saveScroll() reformatPage(p) - tabs[tab].view.SetText(p.Content) - tabs[tab].applyScroll() // Go back to where you were, roughly + t.view.SetText(p.Content) + t.applyScroll() // Go back to where you were, roughly } // setPage displays a Page on the passed tab number. // The bottomBar is not actually changed in this func -func setPage(tab int, p *structs.Page) { - tabs[tab].saveScroll() // Save the scroll of the previous page +func setPage(t *tab, p *structs.Page) { + if !isValidTab(t) { + // Don't waste time reformatting an invalid tab + return + } + + t.saveScroll() // Save the scroll of the previous page // Make sure the page content is fitted to the terminal every time it's displayed reformatPage(p) // Change page on screen - tabs[tab].page = p - tabs[tab].view.SetText(p.Content) - tabs[tab].view.Highlight("") // Turn off highlights - tabs[tab].view.ScrollToBeginning() + t.page = p + t.view.SetText(p.Content) + t.view.Highlight("") // Turn off highlights, other funcs may restore if necessary + t.view.ScrollToBeginning() // Setup display - App.SetFocus(tabs[tab].view) + App.SetFocus(t.view) // Save bottom bar for the tab - TODO: other funcs will apply/display it - tabs[tab].barLabel = "" - tabs[tab].barText = p.Url + t.barLabel = "" + t.barText = p.Url } // goURL is like handleURL, but takes care of history and the bottomBar. @@ -160,14 +175,14 @@ func setPage(tab int, p *structs.Page) { // It has no return values to be processed. // // It should be called in a goroutine. -func goURL(tab int, u string) { - final, displayed := handleURL(tab, u) +func goURL(t *tab, u string) { + final, displayed := handleURL(t, u) if displayed { - tabs[tab].addToHistory(final) + t.addToHistory(final) } - if tab == curTab { + if t == tabs[curTab] { // Display the bottomBar state that handleURL set - tabs[tab].applyBottomBar() + t.applyBottomBar() } } @@ -183,15 +198,32 @@ func goURL(tab int, u string) { // // The bottomBar is not actually changed in this func, except during loading. // The func that calls this one should apply the bottomBar values if necessary. -func handleURL(tab int, u string) (string, bool) { +func handleURL(t *tab, u string) (string, bool) { defer App.Draw() // Just in case - App.SetFocus(tabs[tab].view) + // Save for resetting on error + oldLable := t.barLabel + oldText := t.barText + + // Custom return function + ret := func(s string, b bool) (string, bool) { + if !b { + // Reset bottomBar if page wasn't loaded + t.barLabel = oldLable + t.barText = oldText + } + return s, b + } + + t.barLabel = "" + bottomBar.SetLabel("") + + App.SetFocus(t.view) // To allow linking to the bookmarks page, and history browsing if u == "about:bookmarks" { - Bookmarks(tab) - return "about:bookmarks", true + Bookmarks(t) + return ret("about:bookmarks", true) } u = normalizeURL(u) @@ -199,8 +231,7 @@ func handleURL(tab int, u string) (string, bool) { parsed, err := url.Parse(u) if err != nil { Error("URL Error", err.Error()) - tabs[tab].barText = tabs[tab].page.Url - return "", false + return ret("", false) } if strings.HasPrefix(u, "http") { @@ -222,33 +253,36 @@ func handleURL(tab int, u string) (string, bool) { Error("HTTP Error", "Error executing custom browser command: "+err.Error()) } } - tabs[tab].barText = tabs[tab].page.Url - return "", false + return ret("", false) } if !strings.HasPrefix(u, "gemini") { Error("Protocol Error", "Only gemini and HTTP are supported. URL was "+u) - tabs[tab].barText = tabs[tab].page.Url - return "", false + return ret("", false) } // Gemini URL // Load page from cache if possible page, ok := cache.Get(u) if ok { - setPage(tab, page) - return u, true + setPage(t, page) + return ret(u, true) } // Otherwise download it bottomBar.SetText("Loading...") - tabs[tab].barText = "Loading..." // Save it too, in case the tab switches during loading - tabs[tab].mode = tabModeLoading - defer func(t int) { - tabs[t].mode = tabModeDone - }(tab) + t.barText = "Loading..." // Save it too, in case the tab switches during loading + t.mode = tabModeLoading + defer func(tt *tab) { + tt.mode = tabModeDone + }(t) App.Draw() res, err := client.Fetch(u) + // Loading may have taken a while, make sure tab is still valid + if !isValidTab(t) { + return ret("", false) + } + if err == client.ErrTofu { if Tofu(parsed.Host) { // They want to continue anyway @@ -256,37 +290,31 @@ func handleURL(tab int, u string) (string, bool) { // Response can be used further down, no need to reload } else { // They don't want to continue - // Set the bar back to original URL - tabs[tab].barText = tabs[tab].page.Url - return "", false + return ret("", false) } } else if err != nil { Error("URL Fetch Error", err.Error()) - // Set the bar back to original URL - tabs[tab].barText = tabs[tab].page.Url - return "", false + return ret("", false) } if renderer.CanDisplay(res) { page, err := renderer.MakePage(u, res, textWidth(), leftMargin()) + // Rendering may have taken a while, make sure tab is still valid + if !isValidTab(t) { + return ret("", false) + } + page.Width = termW if err != nil { Error("Page Error", "Issuing creating page: "+err.Error()) - // Set the bar back to original URL - tabs[tab].barText = tabs[tab].page.Url - return "", false + return ret("", false) } go cache.Add(page) - setPage(tab, page) - return u, true + setPage(t, page) + return ret(u, true) } // Not displayable // Could be a non 20 (or 21) status code, or a different kind of document - // Set the bar back to original URL - bottomBar.SetText(tabs[curTab].page.Url) - tabs[tab].barText = tabs[curTab].page.Url - App.Draw() - // Handle each status code switch gemini.SimplifyStatus(res.Status) { case 10: @@ -298,31 +326,31 @@ func handleURL(tab int, u string) (string, bool) { if len(parsed.String()) > 1024 { // 1024 is the max size for URLs in the spec Error("Input Error", "URL for that input would be too long.") - return "", false + return ret("", false) } - return handleURL(tab, parsed.String()) + return ret(handleURL(t, parsed.String())) } - return "", false + return ret("", false) case 30: parsedMeta, err := url.Parse(res.Meta) if err != nil { Error("Redirect Error", "Invalid URL: "+err.Error()) - return "", false + return ret("", false) } redir := parsed.ResolveReference(parsedMeta).String() if YesNo("Follow redirect?\n" + redir) { - return handleURL(tab, redir) + return handleURL(t, redir) } - return "", false + return ret("", false) case 40: Error("Temporary Failure", cview.Escape(res.Meta)) - return "", false + return ret("", false) case 50: Error("Permanent Failure", cview.Escape(res.Meta)) - return "", false + return ret("", false) case 60: Info("The server requested a certificate. Cert handling is coming to Amfora soon!") - return "", false + return ret("", false) } // Status code 20, but not a document that can be displayed yes := YesNo("This type of file can't be displayed. Downloading will be implemented soon. Would like to open the file in a HTTPS proxy for now?") @@ -345,7 +373,7 @@ func handleURL(tab int, u string) (string, bool) { } App.Draw() } - return "", false + return ret("", false) } // normalizeURL attempts to make URLs that are different strings diff --git a/display/tab.go b/display/tab.go index 6deabcf..7e6c0d4 100644 --- a/display/tab.go +++ b/display/tab.go @@ -79,7 +79,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(tab, tabs[tab].page.Url, tabs[tab].page.Links[linkN]) + followLink(tabs[tab], tabs[tab].page.Url, tabs[tab].page.Links[linkN]) return } else { // They've started link highlighting