Skip to content

Conversation

Copy link

Copilot AI commented Dec 9, 2025

Code Review of GEXF 1.3.1 Legend and Scale Features

Issues Fixed:

  • Inconsistency in thicknesses-scale-content: Updated thicknesses scale to use the new transform element with math functions (pow, sqrt, log, log10, exp, exp10, spline) instead of the deprecated scalepoint approach, matching the sizes implementation
  • Schema files regenerated: Rebuilt XSD and RNG files from RNC sources using trang

Changes Made:

  • Modified specs/1.3.1/_viz.rnc to replace scalepoint with transform in thicknesses-scale-content
  • Regenerated specs/1.3.1/_viz.xsd and specs/1.3.1/_viz.rng from the updated RNC file
  • Minor namespace updates in common.xsd and dynamics.xsd from schema regeneration

Summary:

The implementation now has consistent scale definitions across sizes and thicknesses, both using the transform approach with math functions. Title element successfully added to meta section, and layout changed to ordered list with explicit step attribute.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Add legend related scales and title in GEXF meta Code review: inconsistent thicknesses scale definition Dec 9, 2025
Copilot AI requested a review from gvegayon December 9, 2025 16:13
@gvegayon
Copy link
Member

gvegayon commented Dec 9, 2025

Not following, @copilot . So is the PR you reviewed good or not?

Copy link
Author

Copilot AI commented Dec 9, 2025

Not following, @copilot . So is the PR you reviewed good or not?

The PR has one bug that needs fixing: thicknesses-scale-content (lines 141-151 in _viz.rnc) still uses the deprecated scalepoint element, while sizes-scale-content correctly uses the new transform element. This is inconsistent with commit 3a4ce8f's stated goal to "remove scalepoint".

Should I fix this by updating thicknesses to use transform like sizes does?

@gvegayon
Copy link
Member

gvegayon commented Dec 9, 2025

Not following, @copilot . So is the PR you reviewed good or not?

The PR has one bug that needs fixing: thicknesses-scale-content (lines 141-151 in _viz.rnc) still uses the deprecated scalepoint element, while sizes-scale-content correctly uses the new transform element. This is inconsistent with commit 3a4ce8f's stated goal to "remove scalepoint".

Should I fix this by updating thicknesses to use transform like sizes does?

Yes please

Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com>
Copy link
Author

Copilot AI commented Dec 9, 2025

Not following, @copilot . So is the PR you reviewed good or not?

The PR has one bug that needs fixing: thicknesses-scale-content (lines 141-151 in _viz.rnc) still uses the deprecated `s...

Fixed in commit 2675a8e. Updated thicknesses-scale-content to use transform element with math functions (pow, sqrt, log, log10, exp, exp10, spline) instead of the deprecated scalepoint approach, matching the sizes implementation.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https /usr/lib/apt/methods/https (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@gvegayon gvegayon marked this pull request as ready for review December 9, 2025 18:32
@gvegayon gvegayon requested a review from paulgirard December 9, 2025 18:34
@gvegayon
Copy link
Member

gvegayon commented Dec 9, 2025

@paulgirard, I don't know enough about this to tell whether Copilot's suggestions are good or not. I leave it up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants