diff --git a/docs/INTERNALS.md b/docs/INTERNALS.md index ee97d51..ac3c9fd 100644 --- a/docs/INTERNALS.md +++ b/docs/INTERNALS.md @@ -1639,7 +1639,42 @@ 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…*. +rid of one. + +### A favourite you can add and cannot remove + +That message — "open a page first, or use *Keep an address…*" — was the correct +thing to say and useless advice, and his next report said so within the hour: two +tiles both reading `instagram.com`, both kept by typing, neither removable by any +spelling he could see. + +The hole is worth naming plainly, because it had been there since favourites +existed and only #80 made it reachable: **removal required knowing the string.** +Being on the page and pressing `8` worked because the engine handed us the exact +address; anything else meant typing it back. And what the start screen shows is a +*host* with the `www.` stripped and a *title* — neither of which is what the +favourite is stored as. Two kept addresses on one site therefore render as the +same two lines, and the thing you would type is nowhere on the screen. Adding was +one press; removing was a guess. + +Three changes, and the first is the one that matters: + +- **`8` on the start screen removes the tile the pointer is on.** `PageScript.linkAt()` + climbs from the hit-test to the nearest anchor and returns its `href`, which for + a tile is the stored address exactly, because `HomePage` wrote it there. Pointing + at the thing you want gone is the one gesture that cannot be spelled wrong, and + the pointer already knew how to hit-test — the whole addition is fifteen lines of + script and a bridge message. A *recent* tile gets kept instead, which is the same + key doing what it does everywhere else. +- **A kept address is named by its address.** It was named by `SiteRules.KeyFor`, + i.e. the bare host, which is why his two tiles read identically. `Urls.Readable` + drops the scheme and a trailing slash and keeps the rest. +- **`SameKey` folds a leading `www.` as well as the trailing slash** — the two + parts of an address a person neither sees nor types. Without it, typing back + exactly what the tile shows still missed the favourite that tile names. + +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. ## Settings that belong to a site, not to the browser diff --git a/src/common/PageScript.cs b/src/common/PageScript.cs index 5bf0563..a40792a 100644 --- a/src/common/PageScript.cs +++ b/src/common/PageScript.cs @@ -204,6 +204,35 @@ function scrollableAncestor(node) { hide: function () { if (st.el) { st.el.style.display = 'none'; } }, + /* The address of the link under the pointer, or ''. The start screen's tiles + are plain links (see HomePage), and this is how the app acts on the tile + somebody is pointing at without following it. + + Issue #80 is why it exists. A favourite kept by typing its address could be + added and never taken away: removing one meant typing the same address + again, and the tile does not show an address — it shows a host and a title, + neither of which is what the favourite is stored as. So the reporter had two + tiles that read identically, pointed at different pages, and could not be + removed by any spelling he could see. Pointing at the thing you want gone is + the gesture that cannot go wrong, and the pointer already knows how to + hit-test. */ + linkAt: function () { + 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 + 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')); + } + + n = n.parentElement || n.parentNode; + if (n && n.nodeType !== 1) { return ''; } + } + + return ''; + }, + /* fx, fy are fractions of the viewport, so the native side never needs to know the page's CSS pixel size or zoom level. */ move: function (fx, fy) { diff --git a/src/common/Store.cs b/src/common/Store.cs index d492772..598cfa7 100644 --- a/src/common/Store.cs +++ b/src/common/Store.cs @@ -313,10 +313,15 @@ public static void Set(string key, bool value) /// 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. + /// The trailing slash on the path is folded, and a leading www. on + /// the host — the two parts of an address a person neither sees nor types. + /// The start screen shows a tile's host with the www. already + /// stripped, so without that half, typing back exactly what is on the + /// screen still failed to find the favourite it names. + /// + /// 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. /// private static string SameKey(string url) { @@ -349,6 +354,15 @@ private static string SameKey(string url) head = head.Substring(0, head.Length - 1); } + // And the www. the tile does not show. Only at the start of the host, + // never anywhere else in the address. + if (authority >= 0 && + head.Length > floor + 4 && + string.Compare(head, floor, "www.", 0, 4, StringComparison.OrdinalIgnoreCase) == 0) + { + head = head.Substring(0, floor) + head.Substring(floor + 4); + } + return head + url.Substring(cut); } diff --git a/src/common/Urls.cs b/src/common/Urls.cs index a663c78..c2309e8 100644 --- a/src/common/Urls.cs +++ b/src/common/Urls.cs @@ -30,6 +30,27 @@ public static string Normalize(string input) : "https://duckduckgo.com/?q=" + Uri.EscapeDataString(trimmed); } + /// + /// An address as somebody would read it out: no scheme, no trailing slash. + /// + /// Used to name a favourite that was kept by typing its address rather + /// than by being on it, because there is no page title to take and the + /// host on its own is not a name — issue #80's reporter had two tiles both + /// reading "instagram.com", pointing at different pages, with nothing on + /// the screen to tell them apart or to type back in. + /// + public static string Readable(string url) + { + string rest = url ?? string.Empty; + int scheme = rest.IndexOf("://", StringComparison.Ordinal); + if (scheme > 0) + { + rest = rest.Substring(scheme + 3); + } + + return rest.Length > 1 ? rest.TrimEnd('/') : rest; + } + /// /// Quotes a string for embedding in injected JavaScript. Typed text reaches /// the page through a script, so an unescaped quote would break the script diff --git a/src/elm/BrowserApp.cs b/src/elm/BrowserApp.cs index ac626c3..77014e0 100644 --- a/src/elm/BrowserApp.cs +++ b/src/elm/BrowserApp.cs @@ -793,7 +793,7 @@ private void DrawHints() new[] { "5", "type in the field you clicked" }, new[] { "6", "fit page — when the page is cut off" }, new[] { "7", "hide this card" }, - new[] { "8", "keep this page as a tile" }, + new[] { "8", "keep this page — on the start screen, remove a tile" }, new[] { "9", "start screen" }, new[] { "Info", "images off — kept for this site" }, }; @@ -1274,6 +1274,10 @@ private void OnBridgeMessage(JavaScriptMessage message) DiagLog.Add("viewport fix: " + (parts.Length > 1 ? parts[1] : "?")); break; + case "tile": + ToggleTile(parts.Length > 1 ? parts[1] : null); + break; + case "typed": DiagLog.Add("typed: " + (parts.Length > 1 ? parts[1] : "?")); break; @@ -1805,6 +1809,23 @@ private string PageUrl() /// 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. + try + { + _web.Eval("try{window." + BridgeName + ".postMessage('tile\u0001'+String(window." + + PageScript.Namespace + ".linkAt()));}catch(e){}"); + } + catch (Exception ex) + { + DiagLog.Add("asking for the tile failed: " + ex.Message); + } + + return; + } + string url = PageUrl(); if (string.IsNullOrEmpty(url) || url == "-") { @@ -1821,6 +1842,56 @@ private void ToggleFavourite() _cachedUrl = url; } + /// + /// Keeps or removes the favourite whose tile the pointer is on. + /// + /// 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. + /// + /// A recent tile gets kept rather than removed, which is the same key doing + /// the same thing it does everywhere else. + /// + private void ToggleTile(string url) + { + if (string.IsNullOrEmpty(url) || url == "null" || Store.IsGenerated(url)) + { + Flash("Point at a tile first, then press 8"); + return; + } + + bool kept = Store.ToggleFavourite(url, TitleForTile(url)); + DiagLog.Add((kept ? "kept tile " : "removed tile ") + url); + Flash((kept ? "Kept " : "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(); + } + + /// + /// 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. + /// + 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); + } + /// The page's title now, not as of the last status refresh. private string PageTitle() { @@ -1855,7 +1926,10 @@ private void KeepAddress(string 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. - string title = SiteRules.KeyFor(url) ?? url; + // 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"); diff --git a/src/nui/NuiBrowserApp.cs b/src/nui/NuiBrowserApp.cs index 9fdcf45..1428be9 100644 --- a/src/nui/NuiBrowserApp.cs +++ b/src/nui/NuiBrowserApp.cs @@ -1210,7 +1210,7 @@ private void BuildOverlay() new[] { "5", "video: TV overlay / in page — if black" }, new[] { "6", "fit page — when the page is cut off" }, new[] { "7", "hide this card" }, - new[] { "8", "keep this page as a tile" }, + new[] { "8", "keep this page — on the start screen, remove a tile" }, new[] { "9", "start screen" }, new[] { "Info", "images off — kept for this site" }, }; @@ -2357,6 +2357,25 @@ private string PageUrl() /// private void ToggleFavourite() { + if (_atHome) + { + // On the start screen the answer is a tile, and the page has to be + // asked which one. + try + { + _web.EvaluateJavaScript( + "String(window." + PageScript.Namespace + " && window." + + PageScript.Namespace + ".linkAt())", + result => ToggleTile(result)); + } + catch (Exception ex) + { + DiagLog.Add("asking for the tile failed: " + ex.Message); + } + + return; + } + string url = PageUrl(); if (string.IsNullOrEmpty(url) || url == "-") { @@ -2373,6 +2392,56 @@ private void ToggleFavourite() _cachedUrl = url; } + /// + /// Keeps or removes the favourite whose tile the pointer is on. + /// + /// 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. + /// + /// A recent tile gets kept rather than removed, which is the same key doing + /// the same thing it does everywhere else. + /// + private void ToggleTile(string url) + { + if (string.IsNullOrEmpty(url) || url == "null" || Store.IsGenerated(url)) + { + Flash("Point at a tile first, then press 8"); + return; + } + + bool kept = Store.ToggleFavourite(url, TitleForTile(url)); + DiagLog.Add((kept ? "kept tile " : "removed tile ") + url); + Flash((kept ? "Kept " : "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(); + } + + /// + /// 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. + /// + 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); + } + /// The page's title now, not as of the last status refresh. private string PageTitle() { @@ -2397,9 +2466,10 @@ 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. - string title = SiteRules.KeyFor(url) ?? url; + // 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"); diff --git a/tools/startpage/Program.cs b/tools/startpage/Program.cs index 3734ee5..e4b70cb 100644 --- a/tools/startpage/Program.cs +++ b/tools/startpage/Program.cs @@ -184,6 +184,21 @@ private static int Main(string[] args) Store.ToggleFavourite("https://one.test/", "one"); Check(!Store.IsFavourite("https://two.test/"), "two sites are never one favourite"); + // www. is the other half a person never types — the start screen shows + // a tile's host with it already stripped, so typing back exactly what + // is on the screen has to find the favourite it names. + Store.ToggleFavourite("https://www.instagram.com/reels", "instagram.com/reels"); + Check(Store.IsFavourite("https://instagram.com/reels"), + "the host as the tile shows it is the same favourite"); + Check(Store.IsFavourite("https://instagram.com/reels/"), + "with or without the slash as well"); + Check(!Store.IsFavourite("https://wwwinstagram.com/reels"), + "but only a whole www. label at the front of the host"); + Check(!Store.IsFavourite("https://mail.www.test/reels"), + "and never a www. anywhere else in it"); + Check(!Store.IsFavourite("https://www.instagram.com/reel"), + "and /reel is still not /reels — the paths are what differ"); + // 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");