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

A cleaner pr for cgeo/cgeo#4427 #4428

Merged
merged 6 commits into from Oct 22, 2014

Conversation

Projects
None yet
4 participants
@yulin2
Contributor

yulin2 commented Oct 21, 2014

This is a cleaner version of pr cgeo/cgeo#4427

@marco-dev

This comment has been minimized.

Show comment
Hide comment
@marco-dev

marco-dev Oct 21, 2014

Contributor

@yulin
You saw that the image you now write asynchronously is read right after it is written? Won't this task handling keep the preview from loading?

Contributor

marco-dev commented Oct 21, 2014

@yulin
You saw that the image you now write asynchronously is read right after it is written? Won't this task handling keep the preview from loading?

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Oct 21, 2014

Contributor

do you talk about the second task in ImageSelectActivity? but I don't get what you mean. The imagePreview will be set once the image is loaded.

Contributor

yulin2 commented Oct 21, 2014

do you talk about the second task in ImageSelectActivity? but I don't get what you mean. The imagePreview will be set once the image is loaded.

@marco-dev

This comment has been minimized.

Show comment
Hide comment
@marco-dev

marco-dev Oct 21, 2014

Contributor

@yulin2
You are right, I didn't get the full view on my mobile.

Contributor

marco-dev commented Oct 21, 2014

@yulin2
You are right, I didn't get the full view on my mobile.

@samueltardieu

This comment has been minimized.

Show comment
Hide comment
@samueltardieu

samueltardieu Oct 21, 2014

Member

@yulin2 Did all the tests pass? Did you try by hand the features you have modified?

Member

samueltardieu commented Oct 21, 2014

@yulin2 Did all the tests pass? Did you try by hand the features you have modified?

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Oct 22, 2014

Contributor

I did manual test and it looks good.
There're five test cases fails:
SettingsTest.testSettings,
LogTrackableActivityTest.testInsertNameExists, LogTrackableActivityTest.testInsertNumberNotExists,
WaypointsTest.testDownloadWaypoints,
ProcessUtilsTest.testIsInstalled.
But these five tests also fails for f1aadad (the latest commit before I sent the pr), so this pr shouldn't be the reason leads to test failure.
I do all tests on a samsung galaxy note.

Contributor

yulin2 commented Oct 22, 2014

I did manual test and it looks good.
There're five test cases fails:
SettingsTest.testSettings,
LogTrackableActivityTest.testInsertNameExists, LogTrackableActivityTest.testInsertNumberNotExists,
WaypointsTest.testDownloadWaypoints,
ProcessUtilsTest.testIsInstalled.
But these five tests also fails for f1aadad (the latest commit before I sent the pr), so this pr shouldn't be the reason leads to test failure.
I do all tests on a samsung galaxy note.

@rsudev

This comment has been minimized.

Show comment
Hide comment
@rsudev

rsudev Oct 22, 2014

Contributor

The failing tests are probably due to a configuration issue on your testing device (some tests require but not ensure certain settings for static map download and such).
I cross-checked locally and all tests are green.

Contributor

rsudev commented Oct 22, 2014

The failing tests are probably due to a configuration issue on your testing device (some tests require but not ensure certain settings for static map download and such).
I cross-checked locally and all tests are green.

@samueltardieu

This comment has been minimized.

Show comment
Hide comment
@samueltardieu

samueltardieu Oct 22, 2014

Member

I rather suspect that @yulin2 is not a geocaching.com premium member (which is required for proper testing).

Member

samueltardieu commented Oct 22, 2014

I rather suspect that @yulin2 is not a geocaching.com premium member (which is required for proper testing).

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Oct 22, 2014

Contributor

Thanks for the confirmation @rsudev. I did use a basic geocaching.com account to run tests.

Contributor

yulin2 commented Oct 22, 2014

Thanks for the confirmation @rsudev. I did use a basic geocaching.com account to run tests.

@rsudev

This comment has been minimized.

Show comment
Hide comment
@rsudev

rsudev Oct 22, 2014

Contributor

Hmm...
I would assume more failures in this case, but maybe

Contributor

rsudev commented Oct 22, 2014

Hmm...
I would assume more failures in this case, but maybe

@samueltardieu samueltardieu merged commit 58551a2 into cgeo:master Oct 22, 2014

@samueltardieu

This comment has been minimized.

Show comment
Hide comment
@samueltardieu

samueltardieu Oct 22, 2014

Member

Thanks for your contribution.

Member

samueltardieu commented Oct 22, 2014

Thanks for your contribution.

@samueltardieu samueltardieu self-assigned this Oct 22, 2014

@samueltardieu samueltardieu added this to the Master branch milestone Oct 22, 2014

@samueltardieu samueltardieu added the Bug label Oct 22, 2014

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Oct 22, 2014

Contributor

You're very welcome.

Contributor

yulin2 commented Oct 22, 2014

You're very welcome.

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