-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
upload a gcode file received via iOS/iPadOS share feature #598
base: master
Are you sure you want to change the base?
Conversation
In the meantime the share issue on slicer side is fixed. Sharing to octopod works like a breeze now. |
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.
Thanks for the PR. It looks very good. I haven't tested it locally but left some comments before taking it for a local spin/test and merge.
Regards,
Gaston
@@ -152,7 +152,7 @@ class FilesTreeViewController: UIViewController, UITableViewDataSource, UITableV | |||
if let imageView = cell.viewWithTag(50) as? UIImageView { | |||
imageView.image = UIImage(named: file.isModel() ? "Model_48" : "GCode_48") | |||
if let thumbnailURL = file.thumbnail { | |||
octoprintClient.getThumbnailImage(path: thumbnailURL) { (data: Data?, error: Error?, response: HTTPURLResponse) in | |||
octoprintClient.getThumbnailImage(path: thumbnailURL) { (data: Data?, _: Error?, _: HTTPURLResponse) in |
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.
As coding style I like to be explicit about parameters for easier maintenance and readability. Would you be ok reverting this change for both params? Thanks
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.
Reverting is ok, but then the compiler complains with a warning. Any idea, how to resolve this ?
func tableView(_ tableView: UITableView, trailingSwipeActionsConfigurationForRowAt indexPath: IndexPath) -> UISwipeActionsConfiguration? { | ||
let deleteAction = UIContextualAction(style: .destructive, title: NSLocalizedString("Delete", comment: "Delete action"), handler: { (action, view, completionHandler) in | ||
self.showConfirm(message: NSLocalizedString("Do you want to delete this file?", comment: "")) { (UIAlertAction) in | ||
let deleteAction = UIContextualAction(style: .destructive, title: NSLocalizedString("Delete", comment: "Delete action"), handler: { _, _, completionHandler in |
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.
same here (explicit param name)
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.
Reverting is ok, but then the compiler complains with a warning. Any idea, how to resolve this ?
let deleteAction = UIContextualAction(style: .destructive, title: NSLocalizedString("Delete", comment: "Delete action"), handler: { (action, view, completionHandler) in | ||
self.showConfirm(message: NSLocalizedString("Do you want to delete this file?", comment: "")) { (UIAlertAction) in | ||
let deleteAction = UIContextualAction(style: .destructive, title: NSLocalizedString("Delete", comment: "Delete action"), handler: { _, _, completionHandler in | ||
self.showConfirm(message: NSLocalizedString("Do you want to delete this file?", comment: "")) { _ in |
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.
same here (explicit param name)
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.
Reverting is ok, but then the compiler complains with a warning. Any idea, how to resolve this ?
// Delete selected file | ||
self.deleteRow(forRowAt: indexPath) | ||
self.tableView.reloadData() | ||
completionHandler(true) | ||
} no: { (UIAlertAction) in | ||
} no: { _ in |
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.
same here (explicit param name)
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.
Reverting is ok, but then the compiler complains with a warning. Any idea, how to resolve this ?
// MARK: - Button actions | ||
|
||
// Initialize SD card if needed and refresh files from SD card | ||
@IBAction func refreshSDCard(_ sender: Any) { | ||
octoprintClient.refreshSD { (success: Bool, error: Error?, response: HTTPURLResponse) in | ||
octoprintClient.refreshSD { (success: Bool, _: Error?, response: HTTPURLResponse) in |
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.
same here (explicit param name)
if success { | ||
// SD Card refreshed so now fetch files | ||
self.loadFiles(delay: 1) | ||
} else if response.statusCode == 409 { | ||
// SD Card is not initialized so initialize it now | ||
self.octoprintClient.initSD(callback: { (success: Bool, error: Error?, response: HTTPURLResponse) in | ||
self.octoprintClient.initSD(callback: { (success: Bool, _: Error?, _: HTTPURLResponse) in |
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.
same here (explicit param name)
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.
Reverting is ok, but then the compiler complains with a warning. Any idea, how to resolve this ?
@@ -457,8 +464,8 @@ class FilesTreeViewController: UIViewController, UITableViewDataSource, UITableV | |||
} | |||
} | |||
// Load all files and folders (recursive) | |||
octoprintClient.files { (result: NSObject?, error: Error?, response: HTTPURLResponse) in | |||
var newFiles: Array<PrintFile> = Array() | |||
octoprintClient.files { (result: NSObject?, error: Error?, _: HTTPURLResponse) in |
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.
same here (explicit param name)
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.
Reverting is ok, but then the compiler complains with a warning. Any idea, how to resolve this ?
// Delete from server (if failed then show error message and reload) | ||
octoprintClient.deleteFile(origin: printFile.origin!, path: printFile.path!) { (success: Bool, error: Error?, response: HTTPURLResponse) in | ||
octoprintClient.deleteFile(origin: printFile.origin!, path: printFile.path!) { (success: Bool, _: Error?, response: HTTPURLResponse) in |
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.
same here (explicit param name)
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.
Reverting is ok, but then the compiler complains with a warning. Any idea, how to resolve this ?
fileprivate func deleteRow(forRowAt indexPath: IndexPath) { | ||
let printFile: PrintFile! | ||
if searching { | ||
printFile = searchedFiles[indexPath.row] | ||
// Remove file from UI | ||
searchedFiles.remove(at: indexPath.row) | ||
files.removeAll { (someFile: PrintFile) -> Bool in | ||
return someFile == printFile | ||
someFile == printFile |
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.
was the removal of the 'return' keyword intentional?
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.
actually this has been done by SwiftFormat, because the return is not needed in this special case
Apparently all the issues are due to coding styles. If a format ing tool is used, then endless discussions about different flavors can be avoided. SwiftFormat is nicely integrated in Xcode, that‘s why it was applied to the changed files. On Linux, swift-format is an alternative. If you prefer to have explicit parameter names, then how to get rid of the compiler warnings ? Just keep explicit parameter names and tolerating compiler warnings, appears not to be a good solution. Anyway, whatever coding style you like to apply in your project is up to you. So all those changes proposed can be applied. |
Thanks for taking the time to review the PR. |
In reference to #597: This PR presents a quick implementation for the share feature to quickly upload a gcode file. This works in these steps:
From the slicer, it does not work yet, but this may be slicer related or still some wrong definitions. For sure the whole UTTypeIdentifier/Document/Imported Type Identifiers is not self-explaining. So there may be issues on either side.
Two additional notes:
Please comment