-
Notifications
You must be signed in to change notification settings - Fork 82
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
ios: fix exporting files (CSV exports, log export, notes export) #2939
base: master
Are you sure you want to change the base?
Conversation
Ping @Beerosagos in case you missed it |
class GoEnvironment: NSObject, MobileserverGoEnvironmentInterfaceProtocol, UIDocumentInteractionControllerDelegate { | ||
func getSaveFilename(_ fileName: String?) -> String { | ||
let tempDirectory = URL(fileURLWithPath: NSTemporaryDirectory(), isDirectory: true) | ||
let fileURL = tempDirectory.appendingPathComponent(fileName!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a guard for fileName
, like the one used in systemOpen
for the urlString
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure how filename could actually be nil here, any clue? That's why I just used fileName!
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it should not happen, but from what I could understand, the app would crash if that happened. I just felt that in the future we may exploit this method somewhere else without realizing that it is not safe against null values. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it, but I still disagree 👅 this is in the GoEnv interface, so it will only ever be called by the Go backend, and there is no "nil" string in Go. I am unsure why gomobile even types this as optional, it seems almost like a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah now I see your point! I didn't consider that strings are not pointers in Go 😇 Then I agree with you. Feel free to put it back to how it was initially. Maybe it would be nice to have a comment explicitly stating why we don't need the guard, tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
return scene.windows.first(where: { $0.isKeyWindow })?.rootViewController | ||
} | ||
|
||
func systemOpen(_ urlString: String?) throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function doesn't throw any exception, I think you can remove the throws
keyword
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can remove it - this whole class adheres to MobileserverGoEnvironmentInterfaceProtocol
, i.e. it follows the Environment interface in backend.go. I assume gomobile
translates a Go error return to throws
:
bitbox-wallet-app/backend/backend.go
Line 150 in c4867b8
SystemOpen(string) error |
5302525
to
212d401
Compare
Similar to Android, we set `ExportsDir()` to `""` and let the backend environment `getSaveFilename()` construct the full path. Mobile handling of the dirs/files is a big hack and works on implicit assumptions, e.g. that `getSaveFilename()` is always called with `filepath.Join(exportsDir, filename)`, which is only the filename on mobile as `ExportsDir()` returns `""`. This should be cleaned up, but for now iOS is made to behave like Android in this regard. We use a temp directory for these files, so the files will be cleaned up automatically after a while.
Similar to Android, we set
ExportsDir()
to""
and let the backend environmentgetSaveFilename()
construct the full path.Mobile handling of the dirs/files is a big hack and works on implicit assumptions, e.g. that
getSaveFilename()
is always called withfilepath.Join(exportsDir, filename)
, which is only the filename on mobile asExportsDir()
returns""
. This should be cleaned up, but for now iOS is made to behave like Android in this regard.