Make debug UI background layers configurable with new file `debug-ui-config.json` by leonardehrenfried · Pull Request #6295 · opentripplanner/OpenTripPlanner

leonardehrenfried · 2024-12-02T14:07:00Z

Summary

Introduces a new file for configuring the debug UI (debug-ui-config.json) and adds the ability to configure the background layers.

The UI for selecting the background map looks like this:

Issue

Closes #6275

Unit tests

Added a few.

Documentation

I added a new configuration page for the new config file and updated a few existing places.

Changelog

Autogenerated.

Bumping the serialization version id

No.

cc @fpurcell

# Conflicts: # application/src/test/resources/org/opentripplanner/apis/vectortiles/style.json

codecov · 2024-12-02T14:16:04Z

Codecov Report

Attention: Patch coverage is 70.40816% with 29 lines in your changes missing coverage. Please review.

Project coverage is 69.79%. Comparing base (27b18bd) to head (20696c1).
Report is 30 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...opentripplanner/standalone/config/ConfigModel.java	0.00%	10 Missing ⚠️
...entripplanner/apis/vectortiles/DebugStyleSpec.java	84.84%	5 Missing ⚠️
...tripplanner/standalone/config/OtpConfigLoader.java	0.00%	4 Missing ⚠️
...entripplanner/standalone/config/DebugUiConfig.java	91.42%	3 Missing ⚠️
.../vectortiles/GraphInspectorVectorTileResource.java	0.00%	2 Missing ⚠️
...tripplanner/apis/vectortiles/model/TileSource.java	66.66%	1 Missing ⚠️
...ripplanner/framework/application/OtpFileNames.java	50.00%	0 Missing and 1 partial ⚠️
...nner/standalone/config/configure/ConfigModule.java	0.00%	1 Missing ⚠️
...ner/standalone/configure/ConstructApplication.java	0.00%	1 Missing ⚠️
...standalone/server/DefaultServerRequestContext.java	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             dev-2.x    #6295   +/-   ##
==========================================
  Coverage      69.79%   69.79%           
- Complexity     17786    17798   +12     
==========================================
  Files           2017     2019    +2     
  Lines          76042    76126   +84     
  Branches        7781     7786    +5     
==========================================
+ Hits           53073    53132   +59     
- Misses         20264    20288   +24     
- Partials        2705     2706    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

t2gran · 2024-12-02T16:56:32Z

What should the debug UI be named? In the UI it is called "OTP Debug Client" and here it is called "Debug UI" - We should settle on one name. The name "OTP Debug Client" is taking up too much space. So I suggested to shorten it to just "OTP Debug" - In the UI repeating "UI" or "client" is redundant.

So, maybe use "OTP Debug UI" in doc, "OTP Debug" as title in UI and "debug-ui-config.json" ?

testower · 2024-12-03T09:41:05Z

I'm wondering if this is a little misleading? The debug-ui-config.json doesn't seem to directly configure the debug UI does it? I mean the debug UI code doesn't read the config file. Rather, the config file is read by the OTP application, and it configures some behavior related to the map style endpoint.

I guess my question is, do we want to use this config file also to configure other aspects of the debug UI or just the map styles?

leonardehrenfried · 2024-12-03T09:43:40Z

We are planning to expand its role to cover other aspects of the debug UI.

testower · 2024-12-03T09:45:27Z

We are planning to expand its role to cover other aspects of the debug UI.

I see, then we are looking at a way to serve this config file directly to the debug UI at runtime, since it then obviously can't be compiled into the static assets. Will it be available via a public endpoint in OTP?

leonardehrenfried · 2024-12-03T09:46:57Z

I see, then we are looking at a way to serve this config file directly to the debug UI at runtime, since it then obviously can't be compiled into the static assets. Will it be available via a public endpoint in OTP?

There are no concrete plans yet but I also imagine an endpoint from which the debug UI gets the configuration values at runtime.

testower · 2024-12-03T09:52:25Z

I'm ok with this change. My only two cents is that since we are introducing the concept configuration for the debug ui, it seems strange that we don't have a mechanism for the debug ui to fetch it. I suppose it can wait for some actual use case for that.

leonardehrenfried · 2024-12-03T10:29:12Z

We thought about using router-config.json for this particular case, but a Gitter poll showed that almost everyone preferred a separate file for it.

optionsome · 2024-12-03T21:29:10Z

 import org.opentripplanner.apis.vectortiles.model.ZoomDependentNumber;
 import org.opentripplanner.apis.vectortiles.model.ZoomDependentNumber.ZoomStop;
 import org.opentripplanner.service.vehiclerental.street.StreetVehicleRentalLink;
+import org.opentripplanner.standalone.config.debuguiconfig.BackgroundTileLayer;


config.debuguiconfig seems like a bit of repetition (however the same repetition seems to be done for other configs as well for some reason). Perhaps config.debugui/debugclient)

For go or bad, I want to follow the previous naming pattern.

If we can agree to a new structure, I'm happy to make a follow up.

optionsome · 2024-12-03T21:40:41Z

@@ -0,0 +1,107 @@
+package org.opentripplanner.standalone.config;


Why don't we put this inside the debugui package inside the config package (same question for other configs)?

I have no idea and have just followed the pattern that was there before.

I would like to separate refactoring from the feature development. If we can come to an agreement how the packages should be structured I'm happy to change it in a follow up PR.

optionsome · 2024-12-03T21:41:56Z

+/**
+ * This class is an object representation of the 'debug-ui-config.json'.
+ */
+public class DebugUiConfig implements Serializable {


Is this something we want to serialize? Isn't it enough to read this in at run time? Also, should we have some common interface/class for configs?

I removed Serializable: 8395345

I would love to refactor this class, but I request to do it in a separate PR.

…g/DebugUiConfig.java Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

leonardehrenfried · 2024-12-04T08:57:40Z

What should the debug UI be named? In the UI it is called "OTP Debug Client" and here it is called "Debug UI" - We should settle on one name. The name "OTP Debug Client" is taking up too much space. So I suggested to shorten it to just "OTP Debug" - In the UI repeating "UI" or "client" is redundant.

So, maybe use "OTP Debug UI" in doc, "OTP Debug" as title in UI and "debug-ui-config.json" ?

I agree with this and have removed the word "UI" from the UI where it is indeed redundant.

binh-dam-ibigroup · 2024-12-04T13:18:54Z

So I suggested to shorten it to just "OTP Debug" - In the UI repeating "UI" or "client" is redundant.

How about "OTP Debugger"?

testower · 2024-12-09T09:16:53Z

+    });
+    select.onchange = () => {
+      const layerId = select.value;
+      console.log(select.value);


Very minor, but this console.log doesn't seem necessary

testower

I would prefer no console.log, but otherwise ok

leonardehrenfried · 2024-12-09T09:19:51Z

I removed the console.log on dev-2.x: 0079a16

leonardehrenfried added 10 commits December 2, 2024 10:28

Add background tiles

769e380

# Conflicts: # application/src/test/resources/org/opentripplanner/apis/vectortiles/style.json

Add UI elements for selecting background map

77fdc5e

Add fixed TriMet raster tile

c0b4243

Add configuration for debug ui

6ae498d

Update tests

a934342

Fix configuration

25b4fee

Style background layer picker

9bd492a

Add nicer names to UI

441711b

Add documentation

2f0692b

Update config scope docs

57c6593

leonardehrenfried requested a review from a team as a code owner December 2, 2024 14:07

Fix TS linting

c6c6c3b

habrahamsson-skanetrafiken requested review from optionsome and testower December 3, 2024 09:20

testower previously approved these changes Dec 3, 2024

View reviewed changes

optionsome reviewed Dec 3, 2024

View reviewed changes

DebugUiConfig doesn't need to be Serializable

8395345

leonardehrenfried dismissed testower’s stale review via 8395345 December 4, 2024 07:15

leonardehrenfried and others added 4 commits December 4, 2024 08:15

Update application/src/main/java/org/opentripplanner/standalone/confi…

d831ec4

…g/DebugUiConfig.java Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

Update docs

2ac997f

Remove check for LOG.isWarnEnabled()

a83afac

Merge remote-tracking branch 'upstream/dev-2.x' into background-tiles

9230aa3

Use 'OTP Debug' in the UI

20696c1

leonardehrenfried requested a review from optionsome December 5, 2024 18:08

optionsome approved these changes Dec 9, 2024

View reviewed changes

leonardehrenfried requested a review from testower December 9, 2024 09:07

leonardehrenfried enabled auto-merge December 9, 2024 09:07

testower reviewed Dec 9, 2024

View reviewed changes

testower approved these changes Dec 9, 2024

View reviewed changes

leonardehrenfried merged commit 1438c98 into opentripplanner:dev-2.x Dec 9, 2024

t2gran pushed a commit that referenced this pull request Dec 9, 2024

Add changelog entry for #6295 [ci skip]

ff03169

leonardehrenfried deleted the background-tiles branch December 9, 2024 09:19

t2gran added this to the 2.7 (next release) milestone Jan 29, 2025

		@@ -0,0 +1,107 @@
		package org.opentripplanner.standalone.config;

Conversation

leonardehrenfried commented Dec 2, 2024

Summary

Issue

Unit tests

Documentation

Changelog

Bumping the serialization version id

Uh oh!

codecov Bot commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

t2gran commented Dec 2, 2024

Uh oh!

testower commented Dec 3, 2024

Uh oh!

leonardehrenfried commented Dec 3, 2024

Uh oh!

testower commented Dec 3, 2024

Uh oh!

leonardehrenfried commented Dec 3, 2024

Uh oh!

testower commented Dec 3, 2024

Uh oh!

leonardehrenfried commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

optionsome Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

optionsome Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

optionsome Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leonardehrenfried commented Dec 4, 2024

Uh oh!

binh-dam-ibigroup commented Dec 4, 2024

Uh oh!

testower Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

testower left a comment

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried commented Dec 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Dec 2, 2024 •

edited

Loading

leonardehrenfried commented Dec 3, 2024 •

edited

Loading