Skip to content
This repository was archived by the owner on Aug 8, 2023. It is now read-only.

Convert between predicate options and collator expressions - #12329

Merged
1ec5 merged 2 commits into
masterfrom
1ec5-darwin-collator-12269
Jul 9, 2018
Merged

1ec5 merged 2 commits into
masterfrom
1ec5-darwin-collator-12269

Conversation

@1ec5

@1ec5 1ec5 commented Jul 5, 2018

Copy link
Copy Markdown
Contributor

The [c] and [d] options are now supported within comparison predicates for case and diacritic insensitivity, respectively. Along the way, the mbgl code that detects expressions in filters had to be updated to allow a collator as the third argument to a comparison expression.

Fixes #12269. Working towards #11786.

/cc @ChrisLoer @fabian-guerra @anandthakker

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS Core The cross-platform C++ core, aka mbgl runtime styling labels Jul 5, 2018
@1ec5 1ec5 self-assigned this Jul 5, 2018
@1ec5 1ec5 added this to the ios-v4.2.0 milestone Jul 5, 2018
Comment thread src/mbgl/style/conversion/filter.cpp Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Apparently this code was ported from GL JS code that will also need to be updated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe slightly better would be:

length != 3 || (isArray(arrayMember(filter, 1)) || isArray(arrayMember(filter, 2)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The reasoning being that the default should be "this is an expression", and we should scope the fallback to legacy filters as tightly as possible?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The reasoning being that the default should be "this is an expression", and we should scope the fallback to legacy filters as tightly as possible?

Yeah, pretty much: sending as many inputs as possible through the expression parsing pipeline, since that will be more and more likely what users will be trying to write, and so expression-related validation messages are more likely to be useful

@1ec5 1ec5 added the GL JS parity For feature parity with Mapbox GL JS label Jul 5, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this means 'Québec' =[] 'Quebec' will use a collator (and thus the current locale) vs. 'Québec' = 'Quebec' doing non-locale-aware comparison?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As long as an option is specified in brackets, it’ll use a collator. To match Core Data behavior, I suppose we could default the locale to en-posix when c or d is specified unless l is also specified.

A literal =[] is invalid syntax; there has to be an option. NSPredicate will raise an exception immediately on initialization.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the current behavior sounds right. 👍

Comment thread platform/ios/CHANGELOG.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤔 Now that I see this, I wonder if it makes sense to treat the core feature ("support styles using the collator expression") as a separate changelog entry from the platform-specific wrapper...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That’s a good point; we normally mention the JSON equivalent when that’s also new.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the changelog entry.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To make sure I'm understanding: if you wanted to explicitly do locale-sensitive comparison with something other than the current locale, you'd have to manually craft your expression, because NSPredicate only supports a binary "locale-sensitive"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That’s correct. The syntax would look something like this:

NSPredicate(format: "MGL_FUNCTION('==', 'QUEBEC', 'Québec', MGL_FUNCTION('collator', %@))", ["case-sensitive": false, "locale": "fr-CA"])

There’s a couple things we could adjust at the SDK level, like allowing locale to be set to an NSLocale instead of a raw locale identifier. But my understanding is that we normally want developers to keep using the default collator for better performance, so maybe it doesn’t help much to simplify the locale-specific collator syntax.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I made some last-minute changes so that this predicate would work:

  • The collator options object is no longer wrapped in a literal expression when converting from NSExpression to JSON.
  • The MGL_FUNCTION() syntax is used instead of the ==[…] syntax when a locale is specified.

@fabian-guerra fabian-guerra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍🏼

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!

@1ec5
1ec5 force-pushed the 1ec5-darwin-collator-12269 branch 3 times, most recently from 421db04 to e992355 Compare July 7, 2018 01:56
1ec5 added 2 commits July 9, 2018 14:07
When detecting an expression used as a filter, relax the check for expressions to allow comparison expressions to contain a fourth member for the collator.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Core The cross-platform C++ core, aka mbgl GL JS parity For feature parity with Mapbox GL JS iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants