Give each favourites gesture one meaning

README.md

@@ -99,12 +99,24 @@ Every digit is spoken for, so these live in the menu only (**hold OK**):
 | **Pointer style** | Who draws the pointer — Overscan (keeps up) or the page (an arrow) |
 | **Ad blocking on/off** | 2025+ package only |
 
-### Keeping a page you can't land on
+### Favourites
 
-**Keep an address…** in the menu is for URLs that redirect. `https://www.instagram.com/reel`
-sends you to one particular reel, so pressing `8` there would save that clip
-for ever; type the address instead and the tile is the address. It's prefilled
-with wherever you are, so it's usually a matter of deleting the end of it.
+Three gestures, one meaning each:
+
+| | |
+| --- | --- |
+| **8** on a page | Keep it, or drop it |
+| **8** on a tile | Remove that tile — point at it on the start screen |
+| **Keep an address…** | Keep a URL you can't land on |
+
+**Keep an address…** is for URLs that redirect. `https://www.instagram.com/reel`
+sends you to one particular reel, so pressing `8` there would save that clip for
+ever; type the address instead and the tile is the address. It's prefilled with
+wherever you are, so it's usually a matter of deleting the end of it.
+
+To get rid of anything, point at its tile on the start screen and press **8** —
+you never have to remember how it got there or what it was called. That works on
+**Recent** tiles too, if you'd rather a page wasn't listed.
 
 ### Where it opens
 

docs/INTERNALS.md

@@ -1677,6 +1677,50 @@ Shipped in `build-a77a661`. The rule this leaves behind: **anything the app will
 act on by name must be displayed under that name, or be reachable without one.**
 Favourites were neither.
 
+### One gesture, one meaning
+
+And then he reported it a third time, within the hour, and the third report is the
+one that names the actual fault:
+
+> app allows two duplicates url to be added when used both 8 button & keep page
+> menu together, if we point at the tile added using kept menu and press 8 it
+> duplicates that site.
+
+Pointing at a tile and pressing `8` **added a second copy of it** — in the build
+that had just been shipped to let him remove tiles by pointing at them. That is
+three reports in one afternoon, and every one of them was fixed by a better rule
+for matching addresses. The rule was never the problem.
+
+**The fault is that `8` meant two opposite things depending on state the screen did
+not show.** On a page it kept-or-removed depending on an address the tile never
+displayed. On the start screen it kept-or-removed depending on which of two grids
+the pointer was over — and the two grids are the same tiles, drawn the same way,
+one above the other, very often containing the same page. Point at something you
+already kept, press the key that removes things, get another copy. Each fix that
+preserved the toggle bought exactly one more shape of the same report.
+
+So the gestures are separated by meaning rather than by state, and each has one:
+
+| Gesture | Means | Never |
+| --- | --- | --- |
+| `8` on a page | keep this page, or drop it | — |
+| `8` on a tile | get rid of this tile | adds |
+| *Keep an address…* | keep this address | removes, or adds a second copy |
+
+`8` stays a toggle **on a page** because you are looking at the thing itself, so
+both outcomes are legible before you press. Nowhere else is that true. And a menu
+row with the word *keep* in it must not sometimes delete, which is the reading its
+name promises and the one it did not have.
+
+There is now no sequence of presses that produces a second tile for something
+already on the screen, and `tools/startpage/run.sh` holds that as a property
+rather than as a list of cases.
+
+One supporting detail: a tile carries **which grid it is in** (`data-kind`, written
+by `HomePage`, returned by `linkAt` in front of the address). The same page is very
+often a favourite and a recent visit both, and removing it from the list the
+pointer was not on is indistinguishable, from a sofa, from a key that did nothing.
+
 ## 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

src/common/HomePage.cs

@@ -61,13 +61,13 @@ public static string Build(IList<Bookmark> favourites, IList<Bookmark> history,
             }
             else
             {
-                AppendGrid(html, favourites, 12);
+                AppendGrid(html, favourites, 12, "fav");
             }
 
             if (history.Count > 0)
             {
                 html.Append("<h2>Recent</h2>");
-                AppendGrid(html, history, 8);
+                AppendGrid(html, history, 8, "recent");
             }
 
             // Four things that get somebody unstuck on their first evening, and
@@ -76,20 +76,28 @@ public static string Build(IList<Bookmark> favourites, IList<Bookmark> history,
             html.Append(@"<div class='hint'>
 <b>0</b> type an address &nbsp;·&nbsp; <b>7</b> the remote card — every key, and what it is for &nbsp;·&nbsp;
 <b>9</b> back to this screen &nbsp;·&nbsp; <b>channel up/down</b> scrolls, on every remote<br/>
+<b>8</b> on a tile removes it &nbsp;·&nbsp; on a page, keeps it here.<br/>
 Move the pointer with the D-pad and press OK to click. On the keyboard, <b>start</b>
 makes what you typed the page this browser opens on; press it with nothing typed
 to get this screen back.
 </div></body></html>");
             return html.ToString();
         }
 
-        private static void AppendGrid(StringBuilder html, IList<Bookmark> items, int limit)
+        /// <summary>
+        /// One grid of tiles. <paramref name="kind"/> is written onto each tile so
+        /// that pressing 8 on one knows which list it is looking at: the same page
+        /// is very often in both grids, and taking it out of the wrong one is
+        /// indistinguishable, from a sofa, from a key that did nothing.
+        /// </summary>
+        private static void AppendGrid(StringBuilder html, IList<Bookmark> items, int limit, string kind)
         {
             html.Append("<div class='grid'>");
             for (int i = 0; i < items.Count && i < limit; i++)
             {
                 Bookmark item = items[i];
-                html.Append("<a class='tile' href='").Append(Escape(item.Url)).Append("'>")
+                html.Append("<a class='tile' data-kind='").Append(kind)
+                    .Append("' href='").Append(Escape(item.Url)).Append("'>")
                     .Append("<span class='host'>").Append(Escape(HostOf(item.Url))).Append("</span>")
                     .Append("<span class='name'>").Append(Escape(item.Title)).Append("</span>")
                     .Append("</a>");

src/common/PageScript.cs

@@ -220,10 +220,17 @@ removed by any spelling he could see. Pointing at the thing you want gone is
       var n = at();
       for (var depth = 0; n && depth < 8; depth++) {
         if (n.tagName === 'A' && n.getAttribute && n.getAttribute('href')) {
-          /* .href is resolved against the page's base, which is what the app
+          /* Which list the tile is in, then the address. The same page is very
+             often a favourite *and* a recent visit, so a caller told only the
+             address cannot know which of the two tiles the pointer was on — and
+             taking a page out of the wrong list is indistinguishable, from a sofa,
+             from a key that did nothing.
+
+             .href is resolved against the page's base, which is what the app
              stored; the attribute is the fallback for engines that do not give a
-             resolved one on a detached node. */
-          return String(n.href || n.getAttribute('href'));
+             resolved one. */
+          return String(n.getAttribute('data-kind') || 'link') + ' ' +
+                 String(n.href || n.getAttribute('href'));
         }
 
         n = n.parentElement || n.parentNode;

src/common/Store.cs

@@ -234,6 +234,54 @@ public static bool ToggleFavourite(string url, string title)
             return true;
         }
 
+        /// <summary>
+        /// Keeps a page, and says whether it was already kept. Never removes.
+        ///
+        /// The add-only half of <see cref="ToggleFavourite"/>, for "Keep an
+        /// address…" — a menu row with the word *keep* in it must not sometimes
+        /// delete, and the reporter on issue #80 spent an afternoon in the gap
+        /// between those two readings.
+        /// </summary>
+        public static bool Keep(string url, string title)
+        {
+            if (string.IsNullOrEmpty(url) || url == "-" || IsGenerated(url) || IndexOf(Favourites, url) >= 0)
+            {
+                return false;
+            }
+
+            Favourites.Insert(0, new Bookmark(url, string.IsNullOrEmpty(title) ? url : title));
+            Save("favourites.tsv", Favourites);
+            return true;
+        }
+
+        /// <summary>Drops a favourite. True when there was one to drop.</summary>
+        public static bool RemoveFavourite(string url)
+        {
+            int at = IndexOf(Favourites, url);
+            if (at < 0)
+            {
+                return false;
+            }
+
+            Favourites.RemoveAt(at);
+            Save("favourites.tsv", Favourites);
+            return true;
+        }
+
+        /// <summary>Drops a visit. True when there was one to drop.</summary>
+        public static bool ForgetVisit(string url)
+        {
+            int at = IndexOf(History, url);
+            if (at < 0)
+            {
+                return false;
+            }
+
+            History.RemoveAt(at);
+            Save("history.tsv", History);
+            return true;
+        }
+
         public static void RecordVisit(string url, string title)
         {
             if (string.IsNullOrEmpty(url) || url == "-" || url.StartsWith("about:", StringComparison.Ordinal))

src/elm/BrowserApp.cs

@@ -1275,7 +1275,7 @@ private void OnBridgeMessage(JavaScriptMessage message)
                     break;
 
                 case "tile":
-                    ToggleTile(parts.Length > 1 ? parts[1] : null);
+                    ForgetTile(parts.Length > 1 ? parts[1] : null);
                     break;
 
                 case "typed":
@@ -1812,7 +1812,7 @@ private void ToggleFavourite()
             if (_atHome)
             {
                 // On the start screen the answer is a tile, and the page has to be
-                // asked which one. It comes back over the bridge, into ToggleTile.
+                // asked which one. It comes back over the bridge, into ForgetTile.
                 try
                 {
                     _web.Eval("try{window." + BridgeName + ".postMessage('tile\u0001'+String(window." +
@@ -1843,55 +1843,74 @@ private void ToggleFavourite()
         }
 
         /// <summary>
-        /// Keeps or removes the favourite whose tile the pointer is on.
+        /// Removes the tile the pointer is on. On the start screen, that is all 8
+        /// does — it never adds.
+        /// </summary>
+        /// <remarks>
+        /// Issue #80 arrived three times in one afternoon, each time as a different
+        /// symptom of one thing: <b>a gesture that meant two opposite things
+        /// depending on state the screen did not show.</b> First 8 kept or removed
+        /// depending on an address the tile never displayed; then it kept or removed
+        /// depending on which of two identical-looking grids the pointer happened to
+        /// be over, so pointing at a page already kept and pressing 8 added a second
+        /// copy of it. Every fix that kept the toggle bought one more shape of the
+        /// same report.
         ///
-        /// The start screen is where the tiles are, so it is where somebody goes to
-        /// get rid of one — and until this existed there was no way to. Removing a
-        /// favourite meant being on its page and pressing 8, or typing its address
-        /// into "Keep an address…" exactly as it was stored, and issue #80's
-        /// reporter had two tiles reading identically, pointing at different pages,
-        /// stored under addresses the screen never showed him. Pointing at the thing
-        /// you want gone is the one gesture that cannot be spelled wrong.
+        /// So the gestures are separated by meaning instead, and each one has
+        /// exactly one:
+        /// <list type="bullet">
+        /// <item>8 <em>on a page</em> — keep this page, or drop it. You are looking
+        /// at the thing, so a toggle is honest.</item>
+        /// <item>8 <em>on a tile</em> — get rid of this tile. Never adds; you can
+        /// see it is already there.</item>
+        /// <item><em>Keep an address…</em> — keeps. A menu row with the word keep in
+        /// it must not sometimes delete.</item>
+        /// </list>
+        /// There is now no sequence of presses that makes a second tile for
+        /// something already on the screen.
         ///
-        /// A recent tile gets kept rather than removed, which is the same key doing
-        /// the same thing it does everywhere else.
-        /// </summary>
-        private void ToggleTile(string url)
+        /// The tile carries which grid it is in (<c>data-kind</c>, written by
+        /// <see cref="HomePage"/>), because the same page is very often a favourite
+        /// and a recent visit both, and removing it from the list the pointer was
+        /// not on looks exactly like a key that did nothing.
+        /// </remarks>
+        private void ForgetTile(string answer)
         {
+            string kind = string.Empty;
+            string url = answer ?? string.Empty;
+            int space = url.IndexOf(' ');
+            if (space > 0)
+            {
+                kind = url.Substring(0, space);
+                url = url.Substring(space + 1);
+            }
+
             if (string.IsNullOrEmpty(url) || url == "null" || Store.IsGenerated(url))
             {
-                Flash("Point at a tile first, then press 8");
+                Flash("Point at a tile, then press 8 to remove it");
                 return;
             }
 
-            bool kept = Store.ToggleFavourite(url, TitleForTile(url));
-            DiagLog.Add((kept ? "kept tile " : "removed tile ") + url);
-            Flash((kept ? "Kept " : "Removed ") + Urls.Readable(url));
+            bool gone = kind == "recent" ? Store.ForgetVisit(url) : Store.RemoveFavourite(url);
+            if (!gone)
+            {
+                // Nothing was there to remove, which on this screen means the tile
+                // is not what we were told it is. Say so rather than reporting a
+                // removal that did not happen.
+                DiagLog.Add("tile not found in " + (kind == "recent" ? "history" : "favourites") + ": " + url);
+                Flash("That tile is not in the list any more");
+                ShowHome();
+                return;
+            }
+
+            DiagLog.Add("removed " + kind + " tile " + url);
+            Flash("Removed " + Urls.Readable(url));
 
             // Rebuilt, or the tile just removed is still on the screen — which is
             // exactly the "it did not work" this whole thread is about.
             ShowHome();
         }
 
-        /// <summary>
-        /// The name to keep a tile under: whatever it was already called if we know
-        /// it, and the address otherwise. Never the bare host — two favourites on
-        /// one site would then be one name twice, which is how issue #80's reporter
-        /// ended up unable to tell his apart.
-        /// </summary>
-        private static string TitleForTile(string url)
-        {
-            foreach (Bookmark seen in Store.RecentHistory)
-            {
-                if (string.Equals(seen.Url, url, StringComparison.OrdinalIgnoreCase))
-                {
-                    return seen.Title;
-                }
-            }
-
-            return Urls.Readable(url);
-        }
-
         /// <summary>The page's title now, not as of the last status refresh.</summary>
         private string PageTitle()
         {
@@ -1924,15 +1943,24 @@ private void KeepAddress(string text)
         {
             string url = Urls.Normalize(text);
 
-            // No title to take from a page nobody opened, so the site's own name is
-            // the honest one. The tiles are named by this.
             // The address, not the bare host. Two favourites on one site would
             // otherwise be one name twice, and the tiles are all somebody has to
             // tell them apart by — issue #80's follow-up.
             string title = Urls.Readable(url);
-            bool kept = Store.ToggleFavourite(url, title);
-            DiagLog.Add((kept ? "kept address " : "removed address ") + url);
-            Flash(kept ? "Kept " + title : "Removed " + title + " from favourites");
+
+            // Keeps. Never removes, and never a second copy of something already
+            // there: a row with the word keep in it that sometimes deletes is how
+            // this issue got to its third report. Removing is 8 on the tile.
+            if (Store.Keep(url, title))
+            {
+                DiagLog.Add("kept address " + url);
+                Flash("Kept " + title);
+            }
+            else
+            {
+                DiagLog.Add("already kept: " + url);
+                Flash(title + " is already kept — press 8 on its tile to remove it");
+            }
         }
 
         /// <summary>