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

fix qif export to SDCard #419

Merged
merged 1 commit into from
Nov 5, 2015
Merged

Conversation

fefe982
Copy link
Contributor

@fefe982 fefe982 commented Nov 4, 2015

No description provided.

@codinguser
Copy link
Owner

👍 thanks :)

@rivaldi8
Copy link
Collaborator

rivaldi8 commented Nov 4, 2015

@fefe982, beware this PR reverts the changes that fixed #357. Also, it's not considered good practice to have a method do more than what it's expected from its name (this confused me a lot while reading the code to fix the bug).

I think we should be careful with SD card operations. It seems there's some inconsistency in how different devices handle it's access. That may be the cause of the export issues.

@codinguser
Copy link
Owner

@rivaldi8 so does it mean that sharing multiple files (split into QIF) is what causes the sharing file to fail? If so how can we fix that? Or was the problem with the SD card as temporary storage?

At the moment, we rely on the Android getExternalStorageDirectory API. The assumption is that this will return us a location where can safely store the files regardless of the device.

In the future I'm thinking of changing that to use the cache for the temporary internal export before we move the file to the location desired by the user. Hopefully that will be more reliably.

@codinguser
Copy link
Owner

@rivaldi8 I've already merged this request.
I was just thinking, since you identified the cause of the problem with file sharing to 3rd party services, maybe we can fix it without breaking QIF exports. Any ideas?

@codinguser codinguser merged commit 6d625ac into codinguser:develop Nov 5, 2015
@rivaldi8
Copy link
Collaborator

rivaldi8 commented Nov 5, 2015

@codinguser The problem with file sharing is that copyFile() doesn't copy the file src to dst, if src is a Qif. Instead, it calls splitQif(), which splits src into multiple files with dst as file name suffix.

The general problem, I think, is that splitQif() should be only called once as part of the process of exporting to QIF. Then, all export options should deal with multiple files per export.

@fefe982 Could you describe the issue you are fixing with this PR so I can try to reproduce it?

@codinguser
Copy link
Owner

This issue being fixed is that QIF file was not being split into the
different currencies. Or at least, if it was, then only the original
unsplit version was given to the user as the result of the export.

@rivaldi8 Does this mean that the problem of not being able to export is
again present on your device after this patch?

If so then maybe we can do it as you suggest and treat all exports as a
list of files. Which would be 1 for XML and multiple for QIF.
I'll look into it.
On Nov 5, 2015 17:58, "Àlex Magaz Graça" notifications@github.com wrote:

@codinguser https://github.com/codinguser The problem with file sharing
is that copyFile() doesn't copy the file src to dst, if src is a Qif.
Instead, it calls splitQif(), which splits src into multiple files with
dst as file name suffix.

The general problem, I think, is that splitQif() should be only called
once as part of the process of exporting to QIF. Then, all export options
should deal with multiple files per export.

@fefe982 https://github.com/fefe982 Could you describe the issue you
are fixing with this PR so I can try to reproduce it?


Reply to this email directly or view it on GitHub
#419 (comment)
.

@rivaldi8
Copy link
Collaborator

rivaldi8 commented Nov 5, 2015

Yes, it's failing again.

@rivaldi8
Copy link
Collaborator

rivaldi8 commented Nov 9, 2015

@codinguser, are you working on this? If not, I can look into fixing it.

@codinguser
Copy link
Owner

@rivaldi8 please go ahead and look into it.
Won't have time the next few days
Thanks

@codinguser
Copy link
Owner

@rivaldi8 have you been able to look into it yet?

@rivaldi8
Copy link
Collaborator

Yes, I'm working on it.

@rivaldi8
Copy link
Collaborator

@codinguser It have taken more time than I thought but I've finally finished with the fix. I just need some more time to review the changes, do some further clean up, and testing. I'll send a pull request tomorrow.

@codinguser
Copy link
Owner

@rivaldi8 great! looking forward to it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants