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

Media Sharing - Latitude Column Fix #11072

Merged
merged 4 commits into from
Feb 22, 2020
Merged

Conversation

jd-alexander
Copy link
Contributor

@jd-alexander jd-alexander commented Jan 12, 2020

Fixes #10843

This crash is occurring because, in API 29, they depreciated the latitude column.
So the query is currently calling for all columns including the depreciated column even though it isn't used.

Solution

To specify the column that is needed by the query so that the depreciated columns are no longer accessed.

Testing

  1. Share a video with the WordPress app.
  2. Add it to a new post.
  3. Thumbnail should appear and there should be no crash.

Note: I haven't reproduced the fix as yet. I am resolving an API error so this hasn't been tested.

Reviewing

Only 1 reviewer is needed but anyone can review.

Submitter Checklist

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.
  • If it's feasible, I have added unit tests.

a15de7604a Merge pull request #25 from wordpress-mobile/deprecate-buggy-date-time-utils-methods
5026dc0904 Mark DateTimeUtils.localDateToUTC and DateTimeUtils.nowUTC as deprecated
58cd5bf239 Mark DateTimeUtils.localDateToUTC and DateTimeUtils.nowUTC as deprecated
4360e5d41f Merge pull request #23 from wordpress-mobile/update-bintray-config
48277981d8 Bump version to 1.23
81f224bd56 Fix Javadoc lint build error when releasing to Bintray
240f2492fa Update Bintray plugin to latest
b6abdcae2a Merge pull request #22 from wordpress-mobile/merge-wpa
8260cdce57 Update style and lint configs
7c08103ff2 Update Gradle wrapper version
9666183e69 Merge commit 'a4a756b60bcbf144ed8527f693c4e974fa6e365d' into subtree-updates-v3
cdeb3eb7f1 Merge pull request #9215 from wordpress-mobile/feature/update-support-lib-28
e8dbd8f404 Revert update of targetSdkVersion
f7014365ea Update supportLib version to 28.0.0
b3ad4c795e Fix low hanging deprecation warnings
b028488305 Remove all instances of LOCATION
bd194edb49 Merge pull request #9114 from wordpress-mobile/issue/8177-applog-testing
e081d898e7 Merge pull request #9044 from wordpress-mobile/gradle-4-10
2ba6f52024 Move fake AppLog(the one for testing) from utils to the main project
3f74b509d0 Create copy of AppLog for unit testing purposes
78d9bfbf13 fixed merge conflict
b3b6665bfd fixed merge conflict
c2fa689153 Update to Gradle 4.10.3/Android Gradle plugin 3.2.1
62b03547dc Merge remote-tracking branch 'origin/develop' into feature/master-site-creation
f55c4c33b1 Removed duplicated dots from image URL
818c1691c7 Rename NewSiteCreationService props
ab051f9083 removed unused imports
0ea8d2aab7 moved unused getDate() method from MediaUtils to getFormattedDateForLastModified() in PostUtils
6e4cecb37b Make the progress of NewSiteCreationService indeterminate
4434057372 Merge branch 'develop' into feature/master-site-creation
e7e89ad67e Fix SiteCreation segment icon color tint
2259033733 Merge branch 'develop' into feature/deep-link-main-activity
0671724495 Merge remote-tracking branch 'origin/develop' into feature/deep-linking-improvements
8f5238e5ef Reorder repositories in build.gradle to fix Gradle
4f18db4eb3 Reorder repositories in build.gradle to fix Gradle
e695cb4a6b Refactor isDeepLinking to be outside MyProfileActivity
d85b6dbb16 Remove unused WPImageGetter
44d63ede88 Add a new type of log tag for pages
5b4a4f09ae Move the logging wrapper class to FluxCUtils for convenience
c7c1ff782a Add Crashlytics logging to make sure we're keeping track of problems uploading media
d12f32a4a7 Remove coroutines and replace them with standard event driven architecture
5580689d26 Remove unused code - ImageUtils.getThumbnail

git-subtree-dir: libs/utils
git-subtree-split: a15de7604a34a9e989884fb84b8c9adfaeeb97b0
@peril-wordpress-mobile
Copy link

You can test the changes on this Pull Request by downloading the APK here.

@jkmassel
Copy link
Contributor

We're freezing 14.2 today, so this PR is being bumped to 14.3. If you'd like it to ship with 14.2, please merge it into release/14.2 and ping me – I'll be happy to cut a new beta release.

@jkmassel jkmassel modified the milestones: 14.2, 14.3 Feb 10, 2020
@oguzkocer
Copy link
Contributor

Hey @jd-alexander! It looks like this PR hasn't been reviewed yet and there hasn't been an update on it for some time. Do you mind following up on it when you get a chance?

@jd-alexander
Copy link
Contributor Author

Hey @jd-alexander! It looks like this PR hasn't been reviewed yet and there hasn't been an update on it for some time. Do you mind following up on it when you get a chance?

Hey Oguz! Thanks for the ping. It's going to get looked into in groundskeeping as noted here #10843 (comment)

I didn't figure out how to reproduce it as yet so during the review that will happen. Is it being marked as a draft sufficient enough? Let me know. Thanks!

@oguzkocer
Copy link
Contributor

Thanks for the update @jd-alexander! If @khaykov can't get to it, please feel free to ping me for a review.

@khaykov khaykov self-assigned this Feb 21, 2020
@khaykov
Copy link
Member

khaykov commented Feb 21, 2020

Hey @jd-alexander 👋 Looks like there are some changes not related to the issue mixed in?

Comment on lines +173 to +174
Cursor cursor = context.getContentResolver().query(uri, new String[]{OpenableColumns.DISPLAY_NAME},
null, null, null);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This the only line that matters for the solution @khaykov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This crash is occurring because, in API 29, they depreciated the latitude column.
So the query is currently calling for all columns including the depreciated column even though it isn't used.

@jd-alexander
Copy link
Contributor Author

Hey @jd-alexander 👋 Looks like there are some changes not related to the issue mixed in?

yes, this happened because the subtree needed updating.

What's the better way to go about this without including those other changes? I would have to take another look at it. I don't remember exactly why it happened since I haven't worked with subtrees alot.

@khaykov
Copy link
Member

khaykov commented Feb 21, 2020

@jd-alexander Subtrees is the bane of my existence. Yes, looks like there are some changes in utils repo that did not made it into wpandroid yet. Usually, it's another way around :) It's no biggie, and we can merge this one with them.

I was not able to replicate the issue but googling revealed that it's pretty common in various photo-pickers, and your fix makes sense. Could you add some description to the PR so we can proceed?

@jd-alexander
Copy link
Contributor Author

@jd-alexander Subtrees is the bane of my existence. Yes, looks like there are some changes in utils repo that did not made it into wpandroid yet. Usually, it's another way around :) It's no biggie, and we can merge this one with them.

Okay awesome!

I was not able to replicate the issue but googling revealed that it's pretty common in various photo-pickers, and your fix makes sense. Could you add some description to the PR so we can proceed?

Great! Just did.

@jd-alexander jd-alexander marked this pull request as ready for review February 21, 2020 20:42
Copy link
Member

@khaykov khaykov left a comment

Choose a reason for hiding this comment

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

Looks good 👍 Hope it will fix the issue :)

@khaykov khaykov merged commit 0c64406 into develop Feb 22, 2020
@khaykov khaykov deleted the issue-10843/latitude_fix_api_29 branch February 22, 2020 04:20
mzorz added a commit that referenced this pull request Jun 12, 2020
1a43e016c6 Merge pull request #29 from wordpress-mobile/issue/28-empty-img-tag-crash-fix
a4c24373e5 Updated the release version to 1.24
a33296f4d6 Check if img source is null before checking for image smileys
1dd7406ad5 Merge pull request #27 from wordpress-mobile/combined-updates-from-wpandroid
b918b23ac5 Check if announcement is available in App Settings before showing option. Updated AppLog to include announcement related tracker.
94e23e28c1 Rename build.gradle fields with invalid name syntax
d0be1240ca Make sure url utils recognizes atomic image proxy as valid image url.
9bdbdff3a9 Merge branch 'feature/support-private-at-sites' of https://github.com/wordpress-mobile/WordPress-Android into feature/support-private-at-sites
fcc5c99e37 Updated Photon utils to utilize new url for private AT proxy.
130195092f Gutenberg/integrate release 1.25.0 with dark mode (#11580)
a25096c291 Restoring Utils lib version to 1.24
edfef1e58c Merge branch 'develop' into issue/11249-media-not-load-jetpack
60d516266e Merge pull request #11536 from wordpress-mobile/add/log-persistence
1aceca9950 Make the linter happy
18c8447fe3 Change provider visibility
dac4e5ce38 Fixes to LogFileHelpersTest
76f9f75f5d Variablize recent created files test
a4d0a773e7 Small fixes for LogFileCleanerTest
24ac53fabf Make the queue private
64ea6a8cdf Use correct hungarian notation for static member
1c978ca61a Refactor tests to match project refactoring
565b88903c Persist log files in a background thread
3ae86d48e4 Refactor LogEntry.toString method
b900bdda83 Refactor LogFileHelpers to not use context and be a LogFileProvider
225e1db645 Upgrading lib version.
6fd47e8eb6 Using the assertThat pattern.
0948a93056 Adding connected testing.
dc5c2e3cd6 Refactor LogFileWriter to use LogFileProvider instead of context
2d5d0b65c0 Refactor LogFileCleaner to not need context and use a LogFileProvider instead
84b7a8a87d Minor changes in LogFileWriter to make it more idiomatic
8cfa0950aa Merge branch 'develop' into issue/11249-media-not-load-jetpack
31a14daeb4 Bump the utils minSdkVersion to 18
f65fd78414 Add log persistence
fe3960d6b7 Updating ssl logic for photon addresses.
1b4b31bf88 Merge pull request #11524 from wordpress-mobile/fix/wordpressutils-tests
6c80419806 Fix WordPressUtils tests
def5be4e88 Revert "Feature/material theme and Dark Theme support (#11469)" (#11486)
16540e85f1 Feature/material theme and Dark Theme support (#11469)
0ae297bbd3 Adding ssl parameter to query for Photon https.
89725645a6 made method name more meaningful.
76343d3d6d Added util function to check if a Uri is a content one.
75790052ef Merge pull request #11072 from wordpress-mobile/issue-10843/latitude_fix_api_29
981eb82158 Merge branch 'develop' into try/media-upload-completion-processor
8799a19ec4 removes safe check due to nature of RecyclerView
7906f07645 Fixed latitude missing column issue on api 29
882a64faf3 updated gradle wrapper of utils project.
b012d38c3f resolved conflicts and merged utils subtree
e8aae6d282 Merge branch 'develop' into try/media-upload-completion-processor
da19376391 Add MediaFile method to generate attachment page url
e35a9e0cd8 Used delegate safe method for focused accessibility event listener
de02499434 Merge remote-tracking branch 'origin/develop' into issue/10894-aztec-media-talkback
1812393e22 Added accessibility heading
30aa9a3132 Added a safe way to set accessibility delegates.
870518ff07 Moved method to util class that's more appropriate for it's behavior
a998f6e877 added annotations to event listener method
43013a5a79 Moved behavior to Utils so that it can be replicated in other areas.
54ca204429 Removed unused imports.
373db7c13f Removed unneeded whitespace and added necessary whiteline
785da6617b WP Media Picker now announces when an image is selected.
75f0913259 "Done" button now has a content description of "Cancel"
42ec1fb96c Disabled hint announcements and applied accessibility headings
916e2b0d96 Add comment to downloadExternalMedia method
573ba7f5e8 Clean up AddMediaToEditor logic
dda3a1ebb8 Merge pull request #10565 from wordpress-mobile/clean-up-after-legacy-editor
d84fb1f61f Upgrade Gradle to 5.4.1, gradle plugin to 3.5.1 and fix various errors
0511354994 Remove WPEditText
19d3f4aaf0 Optimize AppLog
b4401cd84f Fix simple date format concurrency issue
15ee1b27d0 Remove usages of buggy DateTimeUtils methods
e9f01e5e3c Fixed loop.
47eec63356 Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into issue/9815-email-verification-reminder
25d1310312 Fix AndroidX import order
a21c7120d8 Fix import ordering for androidx
35b99bc73e Migrate to AndroidX
d40e8773bb Merge branch 'issue/9815-create-call-to-action' into issue/9815-add-reminder-message
892eacce49 Make getPath private again
ae6291c612 Use existing method instead of creating a new one in MediaUtils
10b59f8d06 Implement comments from PR
f1b21d0acc Add logging for the failing cases
702881b26f A crash fix for uploaded docs
cc1c0334a8 Added reminder message toast after domain has been registered
9a14564929 Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into issue/9452-domain-suggestions
a4d8873a2e Update style config from style-config-android
1c10935b8c Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into issue/9452-domain-suggestions
971cf76d96 Remove unused get snackbar duration method from accessibility utils class
28b0685ae7 Update is accessibility enabled method in accessibility utils for simplicity
a418cb7dc8 Update get snackbar duration method in accessibility utils for indefinite constant
e0dd4824c9 Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into issue/9452-domain-suggestions
506f9883ce refactored ToastUtils to be able to pass gravity as param. Also showing toast after back-dating a scheduled post on top anchor now
e164cd7a61 Fixing merge conflicts from domain register branch.
586222ec51 Merge pull request #8164 from usamahamid/feature/domain-register
50de29e20a Introduced Domain Suggestion Network request in ViewModel

git-subtree-dir: libs/utils
git-subtree-split: 1a43e016c63bd9dc19f3e27d39b8dc1cba7c5263
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when sharing a video with the app
5 participants