Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions docs/INTERNALS.md
Original file line number Diff line number Diff line change
Expand Up @@ -1721,6 +1721,43 @@ by `HomePage`, returned by `linkAt` in front of the address). The same page is v
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.

### Opening a favourite never told the app it had left the start screen

His fourth report, and the best one:

> when i open site that had images turned on previously from favourites and turn
> images off it turns off images globally instead for that site … if visit website
> by typing url turning images on/off changes setting only for that site

`_atHome` was set by `ShowHome` and cleared by `Navigate` — that is, by whoever
**asked** for a page. That misses every navigation this app does not perform
itself, and **opening a favourite is one of them**: a tile is an ordinary link
(which is the whole reason the start screen is a page and not a native screen), so
the engine follows it and nothing tells the app it left. The browser then sat on
Instagram still believing it was showing its own start screen.

One wrong bit, and everything keyed to it went wrong in a different way:

- the images and identity switches wrote the **browser-wide** setting instead of
the site's, because "no site" is exactly what the start screen is (`PageUrl`
returns null there, and `SiteRules.SetImages(null, …)` correctly declines);
- key `8` looked for a tile under the pointer and found none;
- the address bar said `start screen` over a page that was plainly not it;
- and turning images back on flashed them for a single frame before the reload
put the site's own rule back — which is the symptom that proves the diagnosis,
because it is the global setting and the site rule disagreeing out loud.

The fix is one line in the right place, and the rule behind it is the general one:
**state that describes the page must be derived from the page, not from who asked
for it.** `NoteShowing` is called at every load boundary and sets the flag from
`Store.IsGenerated`, so a navigation nobody in this app initiated still moves it.
`ShowHome` and `Navigate` still set it optimistically, since the bar should not lag
a press, but they are no longer the authority.

This had been wrong since the start screen existed. It cost nothing until #74 gave
the app something important to decide with it, which is the usual shape: the bug
ships years before the feature that makes it reachable.

## 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
Expand Down
33 changes: 33 additions & 0 deletions src/elm/BrowserApp.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1208,6 +1208,7 @@ private void ConfigureWebView()
ApplyViewportFix();
ReportMetrics();
Store.RecordVisit(_web.Url, _web.Title);
NoteShowing(_web.Url);

// Backstop for UrlChanged, which is where a site's rules normally
// land. A page that somehow arrives without one still gets them,
Expand Down Expand Up @@ -1235,6 +1236,10 @@ private void ConfigureWebView()
// early enough for the images and one reload late for the identity.
string arrived = e.GetAsString();
_urlLabel.Text = Markup(arrived);

// Before the site rules, which ask whether this page is on a site
// at all — and "the start screen" is the answer that means no.
NoteShowing(arrived);
ApplySiteRules(arrived, true);
UpdateStatus();
};
Expand Down Expand Up @@ -1769,6 +1774,34 @@ private void Reload(string why)
}
}

/// <summary>
/// Records what the view has actually got on it, which is the only thing
/// that may decide whether we are on the start screen.
/// </summary>
/// <remarks>
/// <c>_atHome</c> used to be set by <c>ShowHome</c> and cleared by
/// <c>Navigate</c> — that is, by whoever *asked* for a page, which misses
/// every navigation the app does not perform itself. **Opening a favourite
/// is one of those**: a tile is an ordinary link, so the engine follows it
/// and nothing tells the app it left. So the browser sat on Instagram still
/// believing it was showing the start screen, and issue #80's reporter found
/// what that costs once things started keying off it: the images and
/// identity switches wrote the browser-wide setting instead of the site's
/// (because "no site" is what the start screen is), key 8 looked for a tile
/// under the pointer, the address bar said "start screen", and turning
/// images back on flashed them for one frame before the site's own rule
/// reloaded them away again. Every one of those is the same wrong bit.
///
/// A page is the start screen if it *is* the start screen. Both shapes of
/// that are <see cref="Store.IsGenerated"/>, and this is called at every
/// load boundary, so a navigation nobody in this app initiated still moves
/// the flag.
/// </remarks>
private void NoteShowing(string url)
{
_atHome = Store.IsGenerated(url);
}

/// <summary>
/// The address the page is on, or null when it is one of ours. Everything
/// per-site keys off this, and "no site" is a real answer rather than a
Expand Down
34 changes: 33 additions & 1 deletion src/nui/NuiBrowserApp.cs
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,10 @@ private bool TryStartEngine()
// The engine reports the destination here, which is where a
// site's own settings go on the view — see ApplySiteRules for
// why that is early enough for the images and one reload late
// for the identity.
// for the identity. NoteShowing comes first: the rules ask
// whether this page is on a site at all, and "the start screen"
// is the answer that means no.
NoteShowing(SafeUrl());
ApplySiteRules(SafeUrl(), true);

// The engine answered, so the view is not the dead kind.
Expand Down Expand Up @@ -521,6 +524,7 @@ private bool TryStartEngine()
Probe();
ApplyViewportFix();
Store.RecordVisit(SafeUrl(), SafeTitle());
NoteShowing(SafeUrl());

// Backstop, for a load whose start we somehow missed. Normally
// a no-op: the site is already the applied one by now.
Expand Down Expand Up @@ -2330,6 +2334,34 @@ private void Reload(string why)
}
}

/// <summary>
/// Records what the view has actually got on it, which is the only thing
/// that may decide whether we are on the start screen.
/// </summary>
/// <remarks>
/// <c>_atHome</c> used to be set by <c>ShowHome</c> and cleared by
/// <c>Navigate</c> — that is, by whoever *asked* for a page, which misses
/// every navigation the app does not perform itself. **Opening a favourite
/// is one of those**: a tile is an ordinary link, so the engine follows it
/// and nothing tells the app it left. So the browser sat on Instagram still
/// believing it was showing the start screen, and issue #80's reporter found
/// what that costs once things started keying off it: the images and
/// identity switches wrote the browser-wide setting instead of the site's
/// (because "no site" is what the start screen is), key 8 looked for a tile
/// under the pointer, the address bar said "start screen", and turning
/// images back on flashed them for one frame before the site's own rule
/// reloaded them away again. Every one of those is the same wrong bit.
///
/// A page is the start screen if it *is* the start screen. Both shapes of
/// that are <see cref="Store.IsGenerated"/>, and this is called at every
/// load boundary, so a navigation nobody in this app initiated still moves
/// the flag.
/// </remarks>
private void NoteShowing(string url)
{
_atHome = Store.IsGenerated(url);
}

/// <summary>
/// The address the page is on, or null when it is one of ours. Everything
/// per-site keys off this, and "no site" is a real answer rather than a
Expand Down
Loading