Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for navPlace element #86

Merged
merged 5 commits into from
Nov 21, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions iiif-presentation.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,5 @@ Gem::Specification.new do |spec|
spec.add_dependency 'json'
spec.add_dependency 'activesupport', '>= 3.2.18'
spec.add_dependency 'faraday', '~> 2.7'
spec.add_dependency 'geo_coord'
end
1 change: 1 addition & 0 deletions lib/iiif/v3/presentation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
choice
collection
manifest
nav_place
resource
image_resource
sequence
Expand Down
98 changes: 98 additions & 0 deletions lib/iiif/v3/presentation/nav_place.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
require 'geo/coord'

module IIIF
module V3
module Presentation
class NavPlace < IIIF::V3::AbstractResource
Rect = Struct.new(:coord1, :coord2)

def initialize(coordinate_texts:, base_uri:)
@coordinate_texts = coordinate_texts
@base_uri = base_uri
end

def build
return if coordinates.nil? || coordinates.empty?

{
id: "#{base_uri}/feature-collection/1",
type: 'FeatureCollection',
features:
}
end

private

attr_reader :coordinate_texts, :base_uri

def coordinates
@coordinates ||= coordinate_texts.map do |coordinate_text|
coordinate_parts = coordinate_text.split(%r{ ?--|/})
case coordinate_parts.length
when 2
coord_for(coordinate_parts[0], coordinate_parts[1])
when 4
rect_for(coordinate_parts)
end
end.compact
end

COORD_REGEX = /(?<hemisphere>[NSEW]) (?<degrees>\d+)[°⁰*] ?(?<minutes>\d+)?[ʹ']? ?(?<seconds>\d+)?[ʺ"]?/

def coord_for(long_str, lat_str)
long_matcher = long_str.match(COORD_REGEX)
lat_matcher = lat_str.match(COORD_REGEX)
return unless long_matcher && lat_matcher
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to silently fail here, or would an argument exception make it clear that what you are doing is not valid?

Copy link
Collaborator Author

@peetucket peetucket Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I am the correct person to answer this, as this was just a refactor PR to move existing code in PURL to here....but my guess is that by silently failing, we prevent the method from blowing up if there is geo data (in the MODs) but it is not formatted correctly. If we throw an exception, in these cases I'd think we would hit errors in PURL until the data is remediated? Or perhaps there can be multiple formats of geo data in MODs and we only handle one case here?

this is the path where the data is coming from: https://github.com/sul-dlss/purl/pull/821/files#diff-01029093f0ab5a19645c20b7e361bc54e695ff29144aacf226a61bca8dfdd554R353

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you ought to check if you have usable data before calling this method. I think this should let you know when you give it the wrong data. Otherwise it becomes a mystery you have to track down.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:

  1. Add a new method to this class to validate coordinates -- it returns true/false
  2. Caller (currently only PURL), calls this first and then only calls build if valid, otherwise noop (or if you really want to raise in PURL, we can do that too, but I will note that is not what is happening in production at the moment and for this refactor work, I'd suggest preserving functionality as is).
  3. build method raises an exception if invalid coordinates are sent in anyway.

Is that what you are thinking

Copy link
Collaborator

@jcoyne jcoyne Nov 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm only thinking of 3 for this gem. You, as the user of the gem can determine how you want to ensure what you're passing in is "good". That could be validating the data before you store it, or filtering out the data you have before you call this method.

If doing 1, also helps you acchieve step 3, then go for it. It's probably better that there is a separate method that validates anyways.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did both 1 & 3 (added a .valid? method and used it to determine if we raise an exception on .build?). PURL can use this .valid? method to keep current functionality as is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the validate method be checking the COORD_REGEX though?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I see it does that.


Geo::Coord.new(latd: lat_matcher[:degrees], latm: lat_matcher[:minutes], lats: lat_matcher[:seconds], lath: lat_matcher[:hemisphere],
lngd: long_matcher[:degrees], lngm: long_matcher[:minutes], lngs: long_matcher[:seconds], lngh: long_matcher[:hemisphere])
end

def rect_for(coordinate_parts)
coord1 = coord_for(coordinate_parts[0], coordinate_parts[2])
coord2 = coord_for(coordinate_parts[1], coordinate_parts[3])
return if coord1.nil? || coord2.nil?

Rect.new(coord1, coord2)
end

def features
coordinates.map.with_index do |coordinate, index|
peetucket marked this conversation as resolved.
Show resolved Hide resolved
{
id: "#{base_uri}/iiif/feature/#{index + 1}",
type: 'Feature',
properties: {},
geometry: coordinate.is_a?(Rect) ? polygon_geometry(coordinate) : point_geometry(coordinate)
}
end
end

def point_geometry(coord)
{
type: 'Point',
coordinates: [format(coord.lng), format(coord.lat)]
}
end

def polygon_geometry(rect)
{
type: 'Polygon',
coordinates: [
[
[format(rect.coord1.lng), format(rect.coord1.lat)],
[format(rect.coord2.lng), format(rect.coord1.lat)],
[format(rect.coord2.lng), format(rect.coord2.lat)],
[format(rect.coord1.lng), format(rect.coord2.lat)],
[format(rect.coord1.lng), format(rect.coord1.lat)]
]
]
}
end

def format(decimal)
peetucket marked this conversation as resolved.
Show resolved Hide resolved
decimal.truncate(6).to_f.to_s
end
end
end
end
end
44 changes: 44 additions & 0 deletions spec/unit/iiif/v3/presentation/nav_place_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
describe IIIF::V3::Presentation::NavPlace do
let(:subject) { described_class.new(coordinate_texts:, base_uri:) }
let(:base_uri) { 'https://purl.stanford.edu' }

describe '#build' do
context 'when coordinates are present' do
let(:coordinate_texts) do
["W 23°54'00\"--E 53°36'00\"/N 71°19'00\"--N 33°30'00\"",
'E 103°48ʹ/S 3°46ʹ).',
'X 103°48ʹ/Y 3°46ʹ).',
'In decimal degrees: (E 138.0--W 074.0/N 073.0--N 041.2).']
end
let(:nav_place) do
{ id: 'https://purl.stanford.edu/feature-collection/1',
type: 'FeatureCollection',
features: [{ id: 'https://purl.stanford.edu/iiif/feature/1',
type: 'Feature',
properties: {},
geometry: { type: 'Polygon',
coordinates: [[['-23.9', '71.316666'],
['53.6', '71.316666'],
['53.6', '33.5'],
['-23.9', '33.5'],
['-23.9', '71.316666']]] } },
{ id: 'https://purl.stanford.edu/iiif/feature/2',
type: 'Feature',
properties: {},
geometry: { type: 'Point', coordinates: ['103.8', '-3.766666'] } }] }
end

it 'returns navPlace' do
expect(subject.build).to eq nav_place
end
end

context 'when coordinates are not present' do
let(:coordinate_texts) { [] }

it 'returns nil' do
expect(subject.build).to be nil
end
end
end
end