✨ Use XBEL for bookmarks - #68

CHANGELOG.md

@@ -7,10 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 ### Changed
 - Favicon support removed (#199)
+- Bookmarks are stored using XML in the XBEL format, old bookmarks are transferred (#68)
 
 ### Fixed
 - Help text is now the same color as `regular_text` in the theme config
 - Non-ASCII (multibyte) characters can now be used as keybindings (#198, #200)
+- Possible subscription update race condition on startup
 
 
 ## [1.8.0] - 2021-02-17

amfora.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 
+	"github.com/makeworld-the-better-one/amfora/bookmarks"
 	"github.com/makeworld-the-better-one/amfora/client"
 	"github.com/makeworld-the-better-one/amfora/config"
 	"github.com/makeworld-the-better-one/amfora/display"
@@ -44,13 +45,18 @@ func main() {
 		fmt.Fprintf(os.Stderr, "Config error: %v\n", err)
 		os.Exit(1)
 	}
+	client.Init()
+
 	err = subscriptions.Init()
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "subscriptions.json error: %v\n", err)
 		os.Exit(1)
 	}
-
-	client.Init()
+	err = bookmarks.Init()
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "bookmarks.xml error: %v\n", err)
+		os.Exit(1)
+	}
 
 	// Initialize lower-level cview app
 	if err = display.App.Init(); err != nil {

bookmarks/bookmarks.go

@@ -2,55 +2,88 @@ package bookmarks
 
 import (
 	"encoding/base32"
+	"encoding/xml"
+	"fmt"
+	"io/ioutil"
+	"os"
 	"sort"
 	"strings"
 
 	"github.com/makeworld-the-better-one/amfora/config"
 )
 
-var bkmkStore = config.BkmkStore
+func Init() error {
+	f, err := os.Open(config.BkmkPath)
+	if err == nil {
+		// File exists and could be opened
 
-// bkmkKey returns the viper key for the given bookmark URL.
-// Note that URLs are the keys, NOT the bookmark name.
-func bkmkKey(url string) string {
-	// Keys are base32 encoded URLs to prevent any special chars like periods from being used
-	return "bookmarks." + base32.StdEncoding.EncodeToString([]byte(url))
-}
+		fi, err := f.Stat()
+		if err == nil && fi.Size() > 0 {
+			// File is not empty
 
-func Set(url, name string) {
-	bkmkStore.Set(bkmkKey(url), name)
-	bkmkStore.WriteConfig() //nolint:errcheck
-}
-
-// Get returns the NAME of the bookmark, given the URL.
-// It also returns a bool indicating whether it exists.
-func Get(url string) (string, bool) {
-	name := bkmkStore.GetString(bkmkKey(url))
-	return name, name != ""
-}
-
-func Remove(url string) {
-	// XXX: Viper can't actually delete keys, which means the bookmarks file might get clouded
-	// with non-entries over time.
-	bkmkStore.Set(bkmkKey(url), "")
-	bkmkStore.WriteConfig() //nolint:errcheck
-}
-
-// All returns all the bookmarks in a map of URLs to names.
-// It also returns a slice of map keys, sorted so that the map *values*
-// are in alphabetical order, with case ignored.
-func All() (map[string]string, []string) {
-	bkmks := make(map[string]string)
-
-	bkmksMap, ok := bkmkStore.AllSettings()["bookmarks"].(map[string]interface{})
-	if !ok {
-		// No bookmarks stored yet, return empty map
-		return bkmks, []string{}
+			xbelBytes, err := ioutil.ReadAll(f)
+			f.Close()
+			if err != nil {
+				return fmt.Errorf("read bookmarks.xml error: %w", err)
+			}
+			err = xml.Unmarshal(xbelBytes, &data)
+			if err != nil {
+				return fmt.Errorf("bookmarks.xml is corrupted: %w", err)
+			}
+		}
+		f.Close()
+	} else if !os.IsNotExist(err) {
+		// There's an error opening the file, but it's not bc is doesn't exist
+		return fmt.Errorf("open bookmarks.xml error: %w", err)
 	}
 
-	inverted := make(map[string]string)       // Holds inverted map, name->URL
-	names := make([]string, 0, len(bkmksMap)) // Holds bookmark names, for sorting
-	keys := make([]string, 0, len(bkmksMap))  // Final sorted keys (URLs), for returning at the end
+	if data.Bookmarks == nil {
+		data.Bookmarks = make([]*xbelBookmark, 0)
+		data.Version = xbelVersion
+	}
+
+	if config.BkmkStore != nil {
+		// There's still bookmarks stored in the old format
+		// Add them and delete the file
+
+		names, urls := oldBookmarks()
+		for i := range names {
+			data.Bookmarks = append(data.Bookmarks, &xbelBookmark{
+				URL:  urls[i],
+				Name: names[i],
+			})
+		}
+
+		err := writeXbel()
+		if err != nil {
+			return fmt.Errorf("error saving old bookmarks into new format: %w", err)
+		}
+
+		err = os.Remove(config.OldBkmkPath)
+		if err != nil {
+			return fmt.Errorf(
+				"couldn't delete old bookmarks file (%s), you must delete it yourself to prevent duplicate bookmarks: %w",
+				config.OldBkmkPath,
+				err,
+			)
+		}
+		config.BkmkStore = nil
+	}
+
+	return nil
+}
+
+// oldBookmarks returns a slice of names and a slice of URLs of the
+// bookmarks in config.BkmkStore.
+func oldBookmarks() ([]string, []string) {
+	bkmksMap, ok := config.BkmkStore.AllSettings()["bookmarks"].(map[string]interface{})
+	if !ok {
+		// No bookmarks stored yet, return empty map
+		return []string{}, []string{}
+	}
+
+	names := make([]string, 0, len(bkmksMap))
+	urls := make([]string, 0, len(bkmksMap))
 
 	for b32Url, name := range bkmksMap {
 		if n, ok := name.(string); n == "" || !ok {
@@ -63,15 +96,89 @@ func All() (map[string]string, []string) {
 			// This would only happen if a user messed around with the bookmarks file
 			continue
 		}
-		bkmks[string(url)] = name.(string)
-		inverted[name.(string)] = string(url)
 		names = append(names, name.(string))
+		urls = append(urls, string(url))
 	}
-	// Sort, then turn back into URL keys
-	sort.Strings(names)
-	for _, name := range names {
-		keys = append(keys, inverted[name])
+	return names, urls
+}
+
+func writeXbel() error {
+	xbelBytes, err := xml.MarshalIndent(&data, "", "    ")
+	if err != nil {
+		return err
 	}
 
-	return bkmks, keys
+	xbelBytes = append(xbelHeader, xbelBytes...)
+	err = ioutil.WriteFile(config.BkmkPath, xbelBytes, 0666)
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
+// Change the name of the bookmark at the provided URL.
+func Change(url, name string) {
+	for _, bkmk := range data.Bookmarks {
+		if bkmk.URL == url {
+			bkmk.Name = name
+			writeXbel() //nolint:errcheck
+			return
+		}
+	}
+}
+
+// Add will add a new bookmark.
+func Add(url, name string) {
+	data.Bookmarks = append(data.Bookmarks, &xbelBookmark{
+		URL:  url,
+		Name: name,
+	})
+	writeXbel() //nolint:errcheck
+}
+
+// Get returns the NAME of the bookmark, given the URL.
+// It also returns a bool indicating whether it exists.
+func Get(url string) (string, bool) {
+	for _, bkmk := range data.Bookmarks {
+		if bkmk.URL == url {
+			return bkmk.Name, true
+		}
+	}
+	return "", false
+}
+
+func Remove(url string) {
+	for i, bkmk := range data.Bookmarks {
+		if bkmk.URL == url {
+			data.Bookmarks[i] = data.Bookmarks[len(data.Bookmarks)-1]
+			data.Bookmarks = data.Bookmarks[:len(data.Bookmarks)-1]
+			writeXbel() //nolint:errcheck
+			return
+		}
+	}
+}
+
+// All returns all the bookmarks in a map of URLs to names.
+// It also returns a slice of map keys, sorted so that the map *values*
+// are in alphabetical order, with case ignored.
+func All() (map[string]string, []string) {
+	bkmksMap := make(map[string]string)
+
+	inverted := make(map[string]string)          // Holds inverted map, name->URL
+	names := make([]string, len(data.Bookmarks)) // Holds bookmark names, for sorting
+	keys := make([]string, len(data.Bookmarks))  // Final sorted keys (URLs), for returning at the end
+
+	for i, bkmk := range data.Bookmarks {
+		bkmksMap[bkmk.URL] = bkmk.Name
+		inverted[bkmk.Name] = bkmk.URL
+		names[i] = bkmk.Name
+	}
+
+	// Sort, then turn back into URL keys
+	sort.Strings(names)
+	for i, name := range names {
+		keys[i] = inverted[name]
+	}
+
+	return bkmksMap, keys
 }

bookmarks/xbel.go

@@ -0,0 +1,43 @@
+package bookmarks
+
+// Structs and code for the XBEL XML bookmark format.
+// https://github.com/makeworld-the-better-one/amfora/issues/68
+
+import (
+	"encoding/xml"
+)
+
+var xbelHeader = []byte(xml.Header + `<!DOCTYPE xbel
+  PUBLIC "+//IDN python.org//DTD XML Bookmark Exchange Language 1.1//EN//XML"
+         "http://www.python.org/topics/xml/dtds/xbel-1.1.dtd">
+`)
+
+const xbelVersion = "1.1"
+
+type xbelBookmark struct {
+	XMLName xml.Name `xml:"bookmark"`
+	URL     string   `xml:"href,attr"`
+	Name    string   `xml:"title"`
+}
+
+// xbelFolder is unused as folders aren't supported by the UI yet.
+// Follow #56 for details.
+// https://github.com/makeworld-the-better-one/amfora/issues/56
+type xbelFolder struct {
+	XMLName   xml.Name        `xml:"folder"`
+	Version   string          `xml:"version,attr"`
+	Folded    string          `xml:"folded,attr"` // Idk if this will be used or not
+	Name      string          `xml:"title"`
+	Bookmarks []*xbelBookmark `xml:"bookmark"`
+	Folders   []*xbelFolder   `xml:"folder"`
+}
+
+type xbel struct {
+	XMLName   xml.Name        `xml:"xbel"`
+	Version   string          `xml:"version,attr"`
+	Bookmarks []*xbelBookmark `xml:"bookmark"`
+	// Later: Folders []*xbelFolder
+}
+
+// Instance of xbel - loaded from bookmarks file
+var data xbel

config/config.go

@@ -32,10 +32,10 @@ var tofuDBDir string
 var tofuDBPath string
 
 // Bookmarks
-
-var BkmkStore = viper.New()
+var BkmkStore = viper.New() // TOML API for old bookmarks file
 var bkmkDir string
-var bkmkPath string
+var OldBkmkPath string // Old bookmarks file that used TOML format
+var BkmkPath string    // New XBEL (XML) bookmarks file, see #68
 
 var DownloadsDir string
 var TempDownloadsDir string
@@ -111,7 +111,8 @@ func Init() error {
 		// XDG data dir on POSIX systems
 		bkmkDir = filepath.Join(basedir.DataHome, "amfora")
 	}
-	bkmkPath = filepath.Join(bkmkDir, "bookmarks.toml")
+	OldBkmkPath = filepath.Join(bkmkDir, "bookmarks.toml")
+	BkmkPath = filepath.Join(bkmkDir, "bookmarks.xml")
 
 	// Feeds dir and path
 	if runtime.GOOS == "windows" {
@@ -160,10 +161,8 @@ func Init() error {
 	if err != nil {
 		return err
 	}
-	f, err = os.OpenFile(bkmkPath, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0666)
-	if err == nil {
-		f.Close()
-	}
+	// OldBkmkPath isn't created because it shouldn't be there anyway
+
 	// Feeds
 	err = os.MkdirAll(subscriptionDir, 0755)
 	if err != nil {
@@ -179,16 +178,12 @@ func Init() error {
 		return err
 	}
 
-	BkmkStore.SetConfigFile(bkmkPath)
+	BkmkStore.SetConfigFile(OldBkmkPath)
 	BkmkStore.SetConfigType("toml")
 	err = BkmkStore.ReadInConfig()
 	if err != nil {
-		return err
-	}
-	BkmkStore.Set("DO NOT TOUCH", true)
-	err = BkmkStore.WriteConfig()
-	if err != nil {
-		return err
+		// File doesn't exist, so remove the viper
+		BkmkStore = nil
 	}
 
 	// Setup main config

display/bookmarks.go

@@ -15,8 +15,17 @@ import (
 // For adding and removing bookmarks, basically a clone of the input modal.
 var bkmkModal = cview.NewModal()
 
+type bkmkAction int
+
+const (
+	add bkmkAction = iota
+	change
+	cancel
+	remove
+)
+
 // bkmkCh is for the user action
-var bkmkCh = make(chan int) // 1, 0, -1 for add/update, cancel, and remove
+var bkmkCh = make(chan bkmkAction)
 var bkmkModalText string // The current text of the input field in the modal
 
 func bkmkInit() {
@@ -60,15 +69,15 @@ func bkmkInit() {
 	m.SetDoneFunc(func(buttonIndex int, buttonLabel string) {
 		switch buttonLabel {
 		case "Add":
-			bkmkCh <- 1
+			bkmkCh <- add
 		case "Change":
-			bkmkCh <- 1
+			bkmkCh <- change
 		case "Remove":
-			bkmkCh <- -1
+			bkmkCh <- remove
 		case "Cancel":
-			bkmkCh <- 0
+			bkmkCh <- cancel
 		case "":
-			bkmkCh <- 0
+			bkmkCh <- cancel
 		}
 	})
 }
@@ -76,9 +85,8 @@ func bkmkInit() {
 // Bkmk displays the "Add a bookmark" modal.
 // It accepts the default value for the bookmark name that will be displayed, but can be changed by the user.
 // It also accepts a bool indicating whether this page already has a bookmark.
-// It returns the bookmark name and the bookmark action:
-// 1, 0, -1 for add/update, cancel, and remove
-func openBkmkModal(name string, exists bool) (string, int) {
+// It returns the bookmark name and the bookmark action.
+func openBkmkModal(name string, exists bool) (string, bkmkAction) {
 	// Basically a copy of Input()
 
 	// Reset buttons before input field, to make sure the input is in focus
@@ -152,11 +160,12 @@ func addBookmark() {
 	// Open a bookmark modal with the current name of the bookmark, if it exists
 	newName, action := openBkmkModal(name, exists)
 	switch action {
-	case 1:
-		// Add/change the bookmark
-		bookmarks.Set(p.URL, newName)
-	case -1:
+	case add:
+		bookmarks.Add(p.URL, newName)
+	case change:
+		bookmarks.Change(p.URL, newName)
+	case remove:
 		bookmarks.Remove(p.URL)
 	}
-	// Other case is action = 0, meaning "Cancel", so nothing needs to happen
+	// Other case is action == cancel, so nothing needs to happen
 }