Match a favourite by the page it is, not by the string it was written as

docs/INTERNALS.md

@@ -1607,6 +1607,40 @@ the two it just did — from a sofa, "kept" and "removed" are the same screen
 otherwise. And the tile's name is the site's own name (`SiteRules.KeyFor`), because
 there is no page title to take from a page nobody opened.
 
+**And then it found two older faults, within the hour.** His report on
+`build-afa4ce8`: "pressing 8 again mostly doesn't show anything on favourited
+site. even when app shows unkept this page sometimes the page still shows in
+favourites." Both halves are real and neither was new — keeping an address by
+typing it is simply the first thing that made them easy to hit.
+
+- **Favourites were matched by exact string.** He kept `instagram.com/reel` by
+  typing it; Instagram's own address for that page ends in a slash. Two strings,
+  so pressing `8` on the page did not find his favourite — it added a second one,
+  and both tiles are named after the same site. What he saw was a page he had just
+  been told was removed, still sitting in his favourites, because it was a
+  different entry that looked identical. `Store.SameKey` now folds a trailing
+  slash on the path, and **only** that: a query and a fragment stay significant,
+  because two addresses that differ there are two pages as often as they are one,
+  and a favourite is an explicit act nobody should have quietly widened for them.
+  The fold has a floor at the scheme's own `//`, or every site on earth would be
+  one favourite. `Init` heals a file that already has both spellings in it, the
+  same way #53's generated pages and waypoints are healed, because his set has one
+  now and a fix that only stopped new ones would leave him deleting the old by
+  hand from a page that cannot tell them apart.
+- **Key `8` was reading a cache.** `_cachedUrl` is refreshed by `UpdateStatus`,
+  which runs when a load *finishes* — and a site that moves between reels without
+  finishing another load leaves it several pages behind what is on the screen. So
+  `8` kept, or removed, a page the user was not looking at. It reads the engine
+  now, as everything per-site already did. The bar's star was reading the same
+  cache, so it is refreshed on the tick while the bar is up: a few seconds at a
+  time, and it leaves a flash alone.
+
+A third thing, smaller and the likeliest reading of "doesn't show anything":
+**pressing `8` on the start screen did nothing, silently.** There is no page there
+to keep, which is a fine reason to decline and no reason at all to say nothing —
+and the start screen, where the tiles are, is exactly where somebody goes to get
+rid of one. It now says so, and points at *Keep an address…*.
+
 ## Settings that belong to a site, not to the browser
 
 Issues #74 and #75, from the same reporter, four days apart. Both are the same
@@ -2550,7 +2584,10 @@ the one its report has to come from. The state is:
   pointer keeps up on Instagram and Spotify now. If it does not, the `pointer`
   line on the report says which of the two is drawing it, and the answer that
   would matter is "drawn by Overscan and still slow", because that is the one
-  saying the lag was never our script.
+  saying the lag was never our script. **His first report on it was about #80**,
+  within the hour, and found two older faults behind the new feature — exact-string
+  favourite matching and key `8` reading a stale cache. See *Keeping an address you
+  cannot land on* above; both are fixed and his files are healed on load.
 
 Five things about that set are settled and should not be re-derived: **key `5` is
 his, not ours** — the engine's overlay path is the only one that gives him a

src/common/Store.cs

@@ -66,6 +66,15 @@ public static void Init(string dataDirectory)
                 DiagLog.Add("store: dropped " + passed + " sign-in page(s) from history");
             }
 
+            // And the same page kept twice under two spellings of its address —
+            // issue #80. See SameKey for how somebody ends up with two.
+            int twice = DropDuplicates(Favourites, "favourites.tsv") +
+                        DropDuplicates(History, "history.tsv");
+            if (twice > 0)
+            {
+                DiagLog.Add("store: dropped " + twice + " duplicate page(s)");
+            }
+
             DiagLog.Add("store: " + Favourites.Count + " favourites, " + History.Count +
                         " history, " + Settings.Count + " settings");
         }
@@ -293,11 +302,62 @@ public static void Set(string key, bool value)
             Set(key, value ? "1" : "0");
         }
 
+        /// <summary>
+        /// The two spellings of one page, reduced to one.
+        ///
+        /// Favourites were matched by exact string, and issue #80's reporter found
+        /// what that costs the moment there are two ways to write the same address:
+        /// he kept <c>instagram.com/reel</c> by typing it, and Instagram's own URL
+        /// for that page is <c>instagram.com/reel/</c>. So pressing 8 there did not
+        /// find his favourite — it added a second one — and both tiles are named
+        /// after the same site, so what he saw was a page he had just been told was
+        /// removed, still sitting in his favourites.
+        ///
+        /// Only the trailing slash is folded, and only on the path. A query and a
+        /// fragment stay significant, because two addresses that differ there are
+        /// two pages as often as they are one, and a favourite is an explicit act
+        /// that nobody should have quietly widened for them.
+        /// </summary>
+        private static string SameKey(string url)
+        {
+            if (string.IsNullOrEmpty(url))
+            {
+                return string.Empty;
+            }
+
+            int cut = url.Length;
+            int query = url.IndexOf('?');
+            if (query >= 0)
+            {
+                cut = query;
+            }
+
+            int fragment = url.IndexOf('#');
+            if (fragment >= 0 && fragment < cut)
+            {
+                cut = fragment;
+            }
+
+            string head = url.Substring(0, cut);
+
+            // Never into the "//" of the scheme: "https://" must not become
+            // "https:/", which would fold every site on earth onto one key.
+            int authority = head.IndexOf("://", StringComparison.Ordinal);
+            int floor = authority < 0 ? 0 : authority + 3;
+            while (head.Length > floor && head[head.Length - 1] == '/')
+            {
+                head = head.Substring(0, head.Length - 1);
+            }
+
+            return head + url.Substring(cut);
+        }
+
         private static int IndexOf(List<Bookmark> list, string url)
         {
+            string key = SameKey(url);
             for (int i = 0; i < list.Count; i++)
             {
-                if (string.Equals(list[i].Url, url, StringComparison.OrdinalIgnoreCase))
+                if (string.Equals(SameKey(list[i].Url), key, StringComparison.OrdinalIgnoreCase))
                 {
                     return i;
                 }
@@ -306,6 +366,41 @@ private static int IndexOf(List<Bookmark> list, string url)
             return -1;
         }
 
+        /// <summary>
+        /// Removes entries that are the same page written two ways, keeping the
+        /// first — which in both files is the more recent. Heals a file an earlier
+        /// build wrote, the same way the generated pages and the sign-in waypoints
+        /// are healed: issue #80's reporter has look-alike favourites on his set
+        /// now, and a fix that only stops new ones appearing would leave him
+        /// deleting the old ones by hand from a page that cannot tell them apart.
+        /// </summary>
+        private static int DropDuplicates(List<Bookmark> list, string fileName)
+        {
+            var seen = new List<string>();
+            int dropped = 0;
+            for (int i = 0; i < list.Count; i++)
+            {
+                string key = SameKey(list[i].Url).ToLowerInvariant();
+                if (seen.Contains(key))
+                {
+                    list.RemoveAt(i);
+                    i--;
+                    dropped++;
+                }
+                else
+                {
+                    seen.Add(key);
+                }
+            }
+
+            if (dropped > 0)
+            {
+                Save(fileName, list);
+            }
+
+            return dropped;
+        }
+
         /// <summary>Removes generated pages a previous build let in, and saves if any were.</summary>
         private static int DropGenerated(List<Bookmark> list, string fileName)
         {

src/elm/BrowserApp.cs

@@ -699,6 +699,16 @@ private bool OnTick()
                 _cursor.FlushPending();
             }
 
+            // The bar is read from a cache that only a finished load refreshes, and
+            // a site that moves between pages without finishing one (a reel, a
+            // playlist) would leave the address and the star showing the page
+            // before. Only while the bar is up, which is a few seconds at a time,
+            // and UpdateStatus leaves a flash alone.
+            if (_chromeVisible)
+            {
+                UpdateStatus();
+            }
+
             // The menu is on the list because arrowing through it is pointer keys,
             // which no longer keep the bar alive; without this it would time out
             // underneath an open menu.
@@ -1778,16 +1788,52 @@ private string PageUrl()
             }
         }
 
+        /// <summary>
+        /// Keeps the page that is open, or takes it out again.
+        ///
+        /// Reads the address from the engine rather than from <c>_cachedUrl</c>,
+        /// and issue #80's reporter is why. That cache is refreshed by
+        /// <c>UpdateStatus</c>, which runs when a load finishes — and a site like
+        /// Instagram moves between reels without ever finishing another load, so
+        /// the cache could be several pages behind what was on the screen. Pressing
+        /// 8 then kept, or removed, a page the user was not looking at.
+        ///
+        /// The start screen has no page of its own to keep, and said so by doing
+        /// nothing at all, which from a sofa is indistinguishable from a key that
+        /// does not work — and the start screen, where the tiles are, is exactly
+        /// where somebody goes to get rid of one.
+        /// </summary>
         private void ToggleFavourite()
         {
-            if (_atHome)
+            string url = PageUrl();
+            if (string.IsNullOrEmpty(url) || url == "-")
             {
+                Flash("Open a page first, or use \u201cKeep an address\u2026\u201d in the menu");
                 return;
             }
 
-            bool kept = Store.ToggleFavourite(_cachedUrl, _cachedTitle);
-            DiagLog.Add((kept ? "kept " : "removed ") + _cachedUrl);
+            bool kept = Store.ToggleFavourite(url, PageTitle());
+            DiagLog.Add((kept ? "kept " : "removed ") + url);
             Flash(kept ? "Kept this page" : "Removed from favourites");
+
+            // The star on the bar is read from the same cache, so it would
+            // otherwise keep showing the answer from before the press.
+            _cachedUrl = url;
+        }
+
+        /// <summary>The page's title now, not as of the last status refresh.</summary>
+        private string PageTitle()
+        {
+            try
+            {
+                string title = _web == null ? null : _web.Title;
+                return string.IsNullOrEmpty(title) ? _cachedTitle : title;
+            }
+            catch (Exception ex)
+            {
+                DiagLog.Add("reading the page title failed: " + ex.Message);
+                return _cachedTitle;
+            }
         }
 
         /// <summary>

src/nui/NuiBrowserApp.cs

@@ -890,6 +890,16 @@ private bool OnTick()
 
             CheckSomethingLoaded();
 
+            // The bar is read from a cache that only a finished load refreshes, and
+            // a site that moves between pages without finishing one (a reel, a
+            // playlist) would leave the address and the star showing the page
+            // before. Only while the bar is up, which is a few seconds at a time,
+            // and UpdateStatus leaves a flash alone.
+            if (_chromeVisible)
+            {
+                UpdateStatus();
+            }
+
             // The menu joins the list now that the pointer no longer keeps the bar
             // alive: arrowing through the menu is pointer keys, and without this the
             // chrome would time out underneath an open menu.
@@ -2330,16 +2340,44 @@ private string PageUrl()
             return _web == null || _atHome ? null : SafeUrl();
         }
 
+        /// <summary>
+        /// Keeps the page that is open, or takes it out again.
+        ///
+        /// Reads the address from the engine rather than from <c>_cachedUrl</c>,
+        /// and issue #80's reporter is why. That cache is refreshed by
+        /// <c>UpdateStatus</c>, which runs when a load finishes — and a site like
+        /// Instagram moves between reels without ever finishing another load, so
+        /// the cache could be several pages behind what was on the screen. Pressing
+        /// 8 then kept, or removed, a page the user was not looking at.
+        ///
+        /// The start screen has no page of its own to keep, and said so by doing
+        /// nothing at all, which from a sofa is indistinguishable from a key that
+        /// does not work — and the start screen, where the tiles are, is exactly
+        /// where somebody goes to get rid of one.
+        /// </summary>
         private void ToggleFavourite()
         {
-            if (_atHome)
+            string url = PageUrl();
+            if (string.IsNullOrEmpty(url) || url == "-")
             {
+                Flash("Open a page first, or use \u201cKeep an address\u2026\u201d in the menu");
                 return;
             }
 
-            bool kept = Store.ToggleFavourite(_cachedUrl, _cachedTitle);
-            DiagLog.Add((kept ? "kept " : "removed ") + _cachedUrl);
+            bool kept = Store.ToggleFavourite(url, PageTitle());
+            DiagLog.Add((kept ? "kept " : "removed ") + url);
             Flash(kept ? "Kept this page" : "Removed from favourites");
+
+            // The star on the bar is read from the same cache, so it would
+            // otherwise keep showing the answer from before the press.
+            _cachedUrl = url;
+        }
+
+        /// <summary>The page's title now, not as of the last status refresh.</summary>
+        private string PageTitle()
+        {
+            string title = SafeTitle();
+            return string.IsNullOrEmpty(title) ? _cachedTitle : title;
         }
 
         /// <summary>

tools/startpage/Program.cs

@@ -155,6 +155,55 @@ private static int Main(string[] args)
             Check(DiagLog.Lines.Exists(l => l.Contains("dropped 2 sign-in")),
                   "the log says how many: " + string.Join(" | ", DiagLog.Lines));
 
+            // 5b. Two spellings of one page are one favourite (issue #80). His
+            //     case exactly: kept by typing instagram.com/reel, pressed 8 on the
+            //     page, whose own address ends in a slash.
+            string dupDir = Path.Combine(dir, "duplicates");
+            Directory.CreateDirectory(dupDir);
+            Store.Init(dupDir);
+            Check(Store.ToggleFavourite("https://www.instagram.com/reel", "instagram.com"),
+                  "an address typed without a trailing slash is kept");
+            Check(Store.IsFavourite("https://www.instagram.com/reel/"),
+                  "and the page's own address, with one, is the same favourite");
+            Check(!Store.ToggleFavourite("https://www.instagram.com/reel/", "Instagram") &&
+                  Store.AllFavourites.Count == 0,
+                  "so pressing 8 there removes it instead of adding a second");
+
+            // The fold is the trailing slash and nothing else. A query and a
+            // fragment still tell two pages apart, because they usually do.
+            Store.ToggleFavourite("https://example.com/a?x=1", "A");
+            Check(!Store.IsFavourite("https://example.com/a?x=2"),
+                  "a different query is a different page");
+            Check(!Store.IsFavourite("https://example.com/b"),
+                  "and so is a different path");
+            Check(Store.IsFavourite("https://example.com/a/?x=1"),
+                  "while a slash before the query is not");
+
+            // The floor: nothing may fold into the scheme's own slashes, or every
+            // site on earth would be one favourite.
+            Store.ToggleFavourite("https://one.test/", "one");
+            Check(!Store.IsFavourite("https://two.test/"), "two sites are never one favourite");
+
+            // 5c. A file an earlier build wrote, with both spellings in it, is
+            //     healed on load — his set has one now.
+            string dupHealDir = Path.Combine(dir, "duplicates-heal");
+            Directory.CreateDirectory(dupHealDir);
+            File.WriteAllLines(Path.Combine(dupHealDir, "favourites.tsv"), new[]
+            {
+                "https://www.instagram.com/reel\tinstagram.com",
+                "https://www.instagram.com/reel/\tInstagram",
+                "https://open.spotify.com/\tSpotify",
+            });
+            DiagLog.Lines.Clear();
+            Store.Init(dupHealDir);
+            Check(Store.AllFavourites.Count == 2, "the look-alike favourite is dropped on load");
+            Check(Store.AllFavourites[0].Url == "https://www.instagram.com/reel",
+                  "keeping the first, which is the more recent");
+            Check(File.ReadAllLines(Path.Combine(dupHealDir, "favourites.tsv")).Length == 2,
+                  "and the file is written back clean");
+            Check(DiagLog.Lines.Exists(l => l.Contains("dropped 1 duplicate")),
+                  "the log says how many: " + string.Join(" | ", DiagLog.Lines));
+
             // 6. Where the browser opens — three states, not two (issue #79).
             //    The one that has to keep working is the upgrade: an install made
             //    before this existed has an address and no mode, and its start page