Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Refactor accessibility calculations #318

Merged
merged 7 commits into from
Jul 8, 2020

Conversation

julianrex
Copy link
Contributor

@julianrex julianrex commented Jun 15, 2020

This is part 2 of 3 PRs that port mapbox/mapbox-gl-native#15303 to this repo. (See also #317, #319.)

This PR, refactors the accessibility element calculations, caching the results. Previously these were needlessly being called continually. This PR addresses the performance concerns. In addition, I have seen fewer crashes.

Fixes #220.

@julianrex julianrex force-pushed the jrex/update-accessibility-calcs branch from b90b5df to 4e9878d Compare June 15, 2020 15:50
This was referenced Jun 15, 2020
@julianrex julianrex marked this pull request as ready for review June 15, 2020 16:17
@julianrex julianrex requested a review from a team June 15, 2020 16:17
Comment on lines 2940 to 2941
// TODO:
/*NSArray **/visiblePlaceFeatures = [self visibleFeaturesInRect:bounds inStyleLayersWithIdentifiers:[NSSet setWithArray:placeStyleLayerIdentifiers]];
Copy link
Contributor

@1ec5 1ec5 Jun 15, 2020

Choose a reason for hiding this comment

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

What’s the to-do item here? (I see that it’s addressed in #319, but I’m not following what the task was.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I lifted visiblePlaceFeatures above the if, so that I could use it further below.

@jmkiley jmkiley force-pushed the jrex/update-accessibility-calcs branch from c583c75 to fb30ebf Compare June 29, 2020 08:47
@jmkiley
Copy link
Contributor

jmkiley commented Jun 29, 2020

@julianrex Made some minor cleanup. I don't see the memory spike with these changes. Will ticket out tail-work around VoiceOver improvements.

Copy link
Contributor

@captainbarbosa captainbarbosa left a comment

Choose a reason for hiding this comment

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

Synced with @jmkiley and ran this locally. I'm seeing other unrelated flaky test failures with offline, and one accessibility-related failure with MGLTestAccessibilityDictionaryForElement within MGLQueryFeatureTests.

Copy link
Contributor

@captainbarbosa captainbarbosa left a comment

Choose a reason for hiding this comment

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

Per chat with @jmkiley we'll follow up on the test failure locally, since the actual implementation works and I didn't seem to find any noted performance degradation.

@jmkiley jmkiley force-pushed the jrex/query-tests branch from 105f0f6 to e5fcaa1 Compare July 7, 2020 22:11
@jmkiley jmkiley force-pushed the jrex/update-accessibility-calcs branch from fb30ebf to 08d4f21 Compare July 7, 2020 23:38
@jmkiley jmkiley merged commit a670461 into jrex/query-tests Jul 8, 2020
@jmkiley jmkiley deleted the jrex/update-accessibility-calcs branch July 8, 2020 00:17
jmkiley added a commit that referenced this pull request Jul 8, 2020
* Port tests from gl-native 14324

* Remove crud.

* Set baselines

* Add timing for dictionary generation

* Remove access token

* Use insert access token script to insert an access token

* Update accessing the access token

* Fix second accessToken

* Refactor accessibility calculations (#318)

* Port tests from gl-native 14324

* Port accessibility calcs

* Fix typo from merge.

* Match results from before optimization.

* Remove temp commented code

* Address some TODOs

* Include exception from failing test

Co-authored-by: jmkiley <jordan.kiley@mapbox.com>

* Update MGLAnnotationViewIntegrationTests.mm

Co-authored-by: jmkiley <jordan.kiley@mapbox.com>
Co-authored-by: Jordan Kiley <jmkiley@users.noreply.github.com>
@knov knov added this to the release-xoài milestone Jul 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants