Skip to content

Commit d221b63

Browse files
committed
Chore: copilot code review
1 parent 501e5ac commit d221b63

7 files changed

Lines changed: 78 additions & 15 deletions

File tree

Source/NETworkManager.Localization/Resources/Strings.Designer.cs

Lines changed: 10 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Source/NETworkManager.Localization/Resources/Strings.resx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -576,6 +576,9 @@
576576
<data name="Map" xml:space="preserve">
577577
<value>Map</value>
578578
</data>
579+
<data name="PlusXMore" xml:space="preserve">
580+
<value>(+{0} more)</value>
581+
</data>
579582
<data name="ScrollToZoomDragToPan" xml:space="preserve">
580583
<value>Scroll = Zoom, Drag = Pan</value>
581584
</data>

Source/NETworkManager.Settings/GlobalStaticConfiguration.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ public static class GlobalStaticConfiguration
3636
public static int NotificationSoundThrottle => 3000;
3737

3838
// Tolerance for comparing two doubles (e.g. a resized panel width/height against a
39-
// known constant) that are expected to be "close enough" rather than bit-for-bit equal.s
39+
// known constant) that are expected to be "close enough" rather than bit-for-bit equal.
4040
public static double FloatPointFix => 1.0;
4141

4242
// Profile config

Source/NETworkManager/Controls/TracerouteMapControl.xaml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
MouseLeftButtonDown="BorderHost_MouseLeftButtonDown"
1818
MouseLeftButtonUp="BorderHost_MouseLeftButtonUp"
1919
MouseMove="BorderHost_MouseMove"
20+
LostMouseCapture="BorderHost_LostMouseCapture"
2021
SizeChanged="BorderHost_SizeChanged">
2122
<Border.Style>
2223
<Style TargetType="{x:Type Border}">
@@ -124,7 +125,7 @@
124125
since it's just a regular element, not a separate popup window.
125126
126127
Bottom-right (rather than sharing the top-left corner with the toggle button) so it never
127-
has to fight eitherew overlay button for space - the four corners now each have exactly one
128+
has to fight either overlay button for space - the four corners now each have exactly one
128129
piece of chrome: toggle (top-left), reset (top-right), zoom/pan hint (bottom-left), this
129130
panel (bottom-right).
130131
-->

Source/NETworkManager/Controls/TracerouteMapControl.xaml.cs

Lines changed: 60 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,14 @@ namespace NETworkManager.Controls;
2525
/// Control that visualizes traceroute hops with known geolocation on an abstract, offline world
2626
/// map (no tile/network requests) with mouse-wheel zoom and drag-to-pan.
2727
/// </summary>
28+
/// <remarks>
29+
/// Known limitations (flagged in PR #3520's review, deliberately left as-is for now):
30+
/// - Arrows are drawn using the raw longitude delta between two points, so a route crossing the
31+
/// antimeridian (e.g. Tokyo to the US west coast) is fitted/drawn the long way around the map
32+
/// instead of wrapping across the edge.
33+
/// - Markers/arrows are only reachable via mouse hover (no keyboard focus or automation names),
34+
/// so their info panel isn't accessible via keyboard or screen reader.
35+
/// </remarks>
2836
public partial class TracerouteMapControl
2937
{
3038
private static readonly ILog Log = LogManager.GetLogger(typeof(TracerouteMapControl));
@@ -372,8 +380,13 @@ private void RedrawHops()
372380

373381
// Merge into the previous group if it's the same city/country and directly
374382
// follows it, so a run of hops within the same city gets a single marker
375-
// instead of several overlapping dots.
376-
if (groups.Count > 0 && IsSameLocation(groups[^1].Info, info))
383+
// instead of several overlapping dots. "Directly follows" is checked against the
384+
// group's last actual hop number (not just group adjacency) - otherwise an
385+
// unresolved hop skipped by the check above (e.g. resolved hops 2 and 4 in the
386+
// same city with unresolved hop 3 between them) would silently merge into the
387+
// same marker and its "Hops 2-4" label would wrongly imply hop 3 belongs to it.
388+
if (groups.Count > 0 && groups[^1].Hops[^1].Hop == hop.Hop - 1 &&
389+
IsSameLocation(groups[^1].Info, info))
377390
{
378391
groups[^1].Hops.Add(hop);
379392
continue;
@@ -397,14 +410,19 @@ private void RedrawHops()
397410
// Repeated visits to the same city (not merged above, since something else was in
398411
// between - e.g. Frankfurt -> Cologne -> Frankfurt) would otherwise stack their markers
399412
// near enough to overlap, at every zoom level - fan them out around the true location
400-
// instead. Grouped by city/country name (like IsSameLocation), not by exact coordinate:
401-
// different IPs in the same city often resolve to slightly different lat/lon from the
402-
// geolocation service (different data centers), so an exact-point comparison here never
403-
// matched and the repeat visits were never detected as overlapping in the first place.
413+
// instead. Grouped by city/country name (like IsSameLocation) rather than exact
414+
// coordinate where possible - see BuildLocationKey for why a missing city needs its own
415+
// fallback here too.
416+
//
417+
// The spread offset is converted to map units using _minScale (the most-zoomed-out
418+
// scale ever reachable, not the route's current fit _scale) so the resulting map-unit
419+
// offset renders to *at least* the intended screen-pixel separation at every zoom level
420+
// the user can actually reach - using the current _scale here would bake in an offset
421+
// that's only correct at this exact zoom, and zooming back out toward _minScale would
422+
// shrink the on-screen gap between markers well below a pixel.
404423
var displayPoints = SpreadOverlappingPoints(
405-
groups.Select(g => (g.Point, LocationKey: $"{g.Info.City}{g.Info.Country}".ToLowerInvariant()))
406-
.ToList(),
407-
1 / _scale);
424+
groups.Select(g => (g.Point, LocationKey: BuildLocationKey(g.Info, g.Point))).ToList(),
425+
1 / _minScale);
408426

409427
_hopPoints = displayPoints;
410428

@@ -599,10 +617,34 @@ void SetHighlighted(bool highlighted)
599617

600618
private static bool IsSameLocation(IPGeolocationInfo a, IPGeolocationInfo b)
601619
{
620+
// A missing city (e.g. a rural hop the geolocation service could only place in a
621+
// country, not a specific city) must not compare equal to another missing city - two
622+
// such hops are otherwise treated as the "same" location purely because both are blank,
623+
// even when their actual coordinates are far apart. Fall back to comparing the resolved
624+
// coordinate directly instead.
625+
if (string.IsNullOrEmpty(a.City) || string.IsNullOrEmpty(b.City))
626+
return a.Lat.Equals(b.Lat) && a.Lon.Equals(b.Lon);
627+
602628
return string.Equals(a.City, b.City, StringComparison.OrdinalIgnoreCase) &&
603629
string.Equals(a.Country, b.Country, StringComparison.OrdinalIgnoreCase);
604630
}
605631

632+
/// <summary>
633+
/// Builds the key SpreadOverlappingPoints groups markers by. Same city/country logic as
634+
/// IsSameLocation - a missing city falls back to the projected point itself (rounded, so
635+
/// float noise doesn't split what's really the same point into different keys) rather than
636+
/// comparing two blank cities as equal, which would otherwise treat unrelated rural hops in
637+
/// the same country as repeat visits to one location and shift them away from their real
638+
/// coordinates.
639+
/// </summary>
640+
private static string BuildLocationKey(IPGeolocationInfo info, Point point)
641+
{
642+
if (!string.IsNullOrEmpty(info.City))
643+
return $"{info.City}{info.Country}".ToLowerInvariant();
644+
645+
return $"{info.Country}{Math.Round(point.X, 1)}{Math.Round(point.Y, 1)}".ToLowerInvariant();
646+
}
647+
606648
private static string BuildHopTooltip(List<TracerouteHopInfo> hops, IPGeolocationInfo info)
607649
{
608650
var firstHop = hops[0];
@@ -625,7 +667,7 @@ private static string BuildHopTooltip(List<TracerouteHopInfo> hops, IPGeolocatio
625667
var ipLine = firstHop.IPAddress?.ToString() ?? "-";
626668

627669
if (hops.Count > 1)
628-
ipLine += $" (+{hops.Count - 1} more)";
670+
ipLine += " " + string.Format(Strings.PlusXMore, hops.Count - 1);
629671

630672
lines.Add(ipLine);
631673

@@ -1212,6 +1254,14 @@ private void BorderHost_MouseLeftButtonUp(object sender, MouseButtonEventArgs e)
12121254
BorderHost.ReleaseMouseCapture();
12131255
}
12141256

1257+
// Mouse capture can be lost without a matching MouseLeftButtonUp (e.g. Alt+Tab while
1258+
// dragging) - without this, _isPanning would stay true and the next mouse move over
1259+
// BorderHost would keep panning the map even though the button is no longer held.
1260+
private void BorderHost_LostMouseCapture(object sender, MouseEventArgs e)
1261+
{
1262+
_isPanning = false;
1263+
}
1264+
12151265
private void ButtonResetView_Click(object sender, RoutedEventArgs e)
12161266
{
12171267
FitToHops(_hopPoints);

Website/docs/application/traceroute.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ The map is only shown if [Check IP geolocation](#check-ip-geolocation) and [Show
3838

3939
:::
4040

41-
Hover a marker or arrow to show its details (location, ISP/ASN, hostname, IP address and average round-trip time) in the top-left info panel.
41+
Hover a marker to show its details (location, ISP/ASN, hostname, IP address and average round-trip time) in the bottom-right info panel. Hovering an arrow shows just the source and destination location of that segment.
4242

4343
You can interact with the map:
4444

Website/docs/changelog/next-release.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ Release date: **xx.xx.2026**
2323

2424
**Traceroute**
2525

26-
- New **Map** view below the hop list, visualizing each resolved hop's geolocation on an offline world map. Consecutive hops are connected with curved, directional arrows; hovering a marker or arrow shows its location, ISP/ASN, hostname, IP address and average round-trip time. The map supports mouse-wheel zoom and drag-to-pan, and can be collapsed via a toggle button on the map itself, similar to the Profiles panel. The map is only shown if **Check IP geolocation** and the new **Show map** setting are both enabled, since hops need a resolved geolocation to be plotted. [#3520](https://github.com/BornToBeRoot/NETworkManager/pull/3520)
26+
- New **Map** view below the hop list, visualizing each resolved hop's geolocation on an offline world map. Consecutive hops are connected with curved, directional arrows; hovering a marker shows its location, ISP/ASN, hostname, IP address and average round-trip time, while hovering an arrow shows the source and destination location of that segment. The map supports mouse-wheel zoom and drag-to-pan, and can be collapsed via a toggle button on the map itself, similar to the Profiles panel. The map is only shown if **Check IP geolocation** and the new **Show map** setting are both enabled, since hops need a resolved geolocation to be plotted. [#3520](https://github.com/BornToBeRoot/NETworkManager/pull/3520)
2727

2828
## Improvements
2929

0 commit comments

Comments
 (0)