Make the route loading more obvious. #260

Pete-Y-CS · 2023-07-05T14:16:32Z

No description provided.

Needs error handling and much more TS safety, but it initially works

dabreegster · 2023-07-06T09:43:40Z

src/lib/draw/route/route_tool.ts

@@ -32,11 +32,12 @@ export class RouteTool {
  ) => void)[];
  eventListenersFailure: (() => void)[];

-  constructor(map: Map, graphBytes: Uint8Array) {
+  constructor(map: Map, graphBytes: Uint8Array, deserialisedCallback=()=>{}) {


This has only one caller, who sets the callback, so a default value seems odd. How about we just write the type? : () => void

dabreegster · 2023-07-06T09:44:49Z

src/style/main.css

Accidentally added these files from the other branch, please remove from this PR

dabreegster · 2023-07-06T09:48:09Z

src/lib/draw/route/RouteMode.svelte

 </script>

-{#if !routeTool}
+{#if !routeToolReady}
  <!-- TODO the text should be fixed, and the progress bar float -->


Comment becomes out of date with this PR

dabreegster · 2023-07-06T09:55:22Z

src/lib/draw/route/RouteMode.svelte

  <!-- TODO the text should be fixed, and the progress bar float -->
-  <div bind:this={progress}>Route tool loading...</div>
+  <p>Route tool loading</p>
+  <ProgressBar bind:series={progress} />


I don't understand how this new dependency helps us. It can draw lots of fancier progress bars, but we just want a simple linear one. In the spirit of keeping dependencies low, shouldn't we avoid it? We could maybe replace the older CSS hacks with https://developer.mozilla.org/en-US/docs/Web/HTML/Element/progress

dabreegster · 2023-07-06T09:55:55Z

src/lib/draw/route/RouteMode.svelte

  export let routeTool: RouteTool;
  export let eventHandler: EventHandler;

+  let progress: Array<number> = [0 ];


Why is this an array with just one element?

(also, npm run fmt shows a diff here and elsewhere)

dabreegster · 2023-07-06T09:57:00Z

src/lib/draw/route/RouteMode.svelte

-      routeTool = new RouteTool($map, graphBytes);
+      const graphBytes = await fetchWithProgress(
+        url,
+        (percentLoaded) => (progress[0] = Math.min(percentLoaded, 90))


So here's where we say "we got all of Content-Length, cap at 90". This is subtle / unintuitive, so definitely worth a comment

dabreegster · 2023-07-06T09:59:59Z

src/lib/FetchProgressBar.svelte

@@ -0,0 +1,52 @@
+<script lang="ts">


This looks unused. Is the intention to keep this or the one in RouteMode? I'd vote for having the progress bar logic wrapped up in its own component, not in the middle of RouteMode. Especially since we may want to use this for route info (worker.ts) in some form in the future.

dabreegster

Looks great to me, after fixing the 2 requested changes!

A future idea is to extract the fetch logic and all the error / loading / done states into some kind of dedicated Svelte component. It could be agnostic to what's being loaded, except for understanding this Content-Length + gzip + extra unpack complication. Will probably need a bunch of callback plumbing, so not really important now

dabreegster · 2023-07-12T08:17:53Z

package-lock.json

Could we revert the changes here please? Guessing this was a rebase problem. Without changes to package.json, I wouldn't expect the lock file to change generally

dabreegster · 2023-07-12T08:27:34Z

src/lib/draw/route/RouteMode.svelte

@@ -61,11 +68,58 @@
      changeMode
    );
  });
+
+  async function fetchWithProgress(url, setProgress: (number) => void) {


Suggested change

async function fetchWithProgress(url, setProgress: (number) => void) {

// setProgress will generally be called with a number between 0 and 100, but it may exceed this for gzipped files.

// The server must send back a Content-Length header.

async function fetchWithProgress(url: string, setProgress: (number) => void) {

dabreegster · 2023-07-12T08:29:02Z

src/lib/draw/route/RouteMode.svelte

-  <!-- TODO the text should be fixed, and the progress bar float -->
-  <div bind:this={progress}>Route tool loading...</div>
+{#if !routeToolReady && !failedToLoadRouteTool && !downloadComplete}
+  <label for="route-loading">Route tool loading</label>


Elsewhere we've been doing <label>Route tool loading <progress value={progress} />, the second form mentioned by https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label, to avoid making up an ID. Not too opinionated about it here

dabreegster · 2023-07-12T08:42:00Z

src/lib/draw/route/RouteMode.svelte

+  <progress id="route-loading" value={progress} />
+{:else if downloadComplete && !routeToolReady && !failedToLoadRouteTool}
+  <label for="route-unpacking">Route data unpacking</label>
+  <progress id="route-unpacking" />


I really like the default animation of bouncing back and forth!

So the strategy is:

Show a 0-100 progress bar while we're downloading, up to the Content-Length of the .gzip file

We'll exceed 100%, because the browser is unpacking and passing along more than that Content-Length (and we have no way of knowing the uncompressed size, unless we store and send back a second header somehow)

As soon as we hit 100%, we switch to the bouncey animation and say unpacking. We might still be loading from the network for some of this, but it's fine.

Peter York added 3 commits July 3, 2023 13:50

Write a progress bar using Svelte and TS, and use it for route-snapper.

7ef20fe

Needs error handling and much more TS safety, but it initially works

Try to work on progress bar

682c69e

Use a progress bar

d208791

Pete-Y-CS changed the title ~~Use externally defined progress bar and make the route loading more obvious.~~ WIP: Use externally defined progress bar and make the route loading more obvious. Jul 5, 2023

Pete-Y-CS linked an issue Jul 5, 2023 that may be closed by this pull request

Improve progress bars #181

Closed

3 tasks

dabreegster reviewed Jul 6, 2023

View reviewed changes

Pete-Y-CS added 3 commits July 10, 2023 09:59

Re-add failed to load error message

ce01e61

Try explain that Route data unpacking

03d6a7b

add second phase for unpacking

061a24a

Pete-Y-CS force-pushed the progress-rebased branch 2 times, most recently from 57fd0ba to 75290fc Compare July 10, 2023 10:22

This comment was marked as off-topic.

Sign in to view

robinlovelace-ate approved these changes Jul 11, 2023

View reviewed changes

robinlovelace-ate mentioned this pull request Jul 11, 2023

Add a schema based on ATF4 guidance #262

Merged

Add non-perecentage-based progress bar for unpacking

acdf41b

Pete-Y-CS force-pushed the progress-rebased branch from 75290fc to acdf41b Compare July 11, 2023 12:53

Pete-Y-CS changed the title ~~WIP: Use externally defined progress bar and make the route loading more obvious.~~ Make the route loading more obvious. Jul 11, 2023

Remvoe accidentally added style files

6762dd0

dabreegster approved these changes Jul 12, 2023

View reviewed changes

Revert package lock

91ae41e

Pete-Y-CS merged commit 86e1514 into main Jul 12, 2023

Pete-Y-CS deleted the progress-rebased branch July 12, 2023 09:30

dabreegster added a commit that referenced this pull request Jul 12, 2023

Fix some TS errors from #260

0ad76f9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the route loading more obvious. #260

Make the route loading more obvious. #260

Pete-Y-CS commented Jul 5, 2023

dabreegster Jul 6, 2023

dabreegster Jul 6, 2023

dabreegster Jul 6, 2023

dabreegster Jul 6, 2023

dabreegster Jul 6, 2023

dabreegster Jul 6, 2023

dabreegster Jul 6, 2023

This comment was marked as off-topic.

dabreegster left a comment

dabreegster Jul 12, 2023

dabreegster Jul 12, 2023

dabreegster Jul 12, 2023

dabreegster Jul 12, 2023

-  async function fetchWithProgress(url, setProgress: (number) => void) {
+  // setProgress will generally be called with a number between 0 and 100, but it may exceed this for gzipped files.
+  // The server must send back a Content-Length header.
+  async function fetchWithProgress(url: string, setProgress: (number) => void) {

Make the route loading more obvious. #260

Make the route loading more obvious. #260

Conversation

Pete-Y-CS commented Jul 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

dabreegster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment