Feature/rename #102
Feature/rename #102
Conversation
Codecov Report
@@ Coverage Diff @@
## master #102 +/- ##
==========================================
- Coverage 23.31% 21.98% -1.33%
==========================================
Files 57 58 +1
Lines 4877 5172 +295
==========================================
Hits 1137 1137
- Misses 3740 4035 +295
Continue to review full report at Codecov.
|
(1)Renaming to a blank name should not be allowed. current behaviour: if you rename a item to blank, an alert is shown with message "already exists", so that the name of the file is "\", same as the webdav name of the folder. expected behaviour: rename to blank not allowed. For example, by disabling "Done" option. tested with iPhoneX v11.4 |
(2)Look at these steps (SDK involved @felix-schwarz )
Current: When the second renaming is performed, the source item does not already exist (was renamed before), so an alert is displayed with the initial file name. The final name of the item is the one in step 2. Maybe confusing for user who expects to get the name of the latest renaming. |
(3)Characters like "\" and "/" are forbidden in the name of the items. Server shows the message App does not behaves correctly is such cases. Displays error or gives no feedback about the issue. |
(4)The following one is not a correct user-use-case, pay attention: Rename a image file by renaming the extension (f.ex. pic.jpg -> pic.pdf) Thumbnail of the image is shown in list file. If you swipe to rename, in the renaming view appears the pdf icon. Maybe, this is not the regular behaviour but if renaming removes the extension (f.ex pic.jpg -> pic), same issue appears in renaming view. |
(5)The rename view shows the option to confirm in different way in iPhone and iPad: iPhone: Done Both should display the same wording |
|
|
|
About #102 (comment): I use the standard iOS UIBarButtonItem init method: convenience init(barButtonSystemItem systemItem: UIBarButtonItem.SystemItem, target: Any?, action: Selector?)so the done button is initialized as: doneButton = UIBarButtonItem(barButtonSystemItem: .done, target: self, action: #selector(doneButtonPressed))I have tested it with iOS 12 beta 5 and the iPad button now say Done so it seems that in iOS 11.* Apple has some diffs in the naming for this kind of |
There was a problem hiding this comment.
Well done!
Please see the comments for the few changes I think should be made.
The only issue not mentioned there: right now, the layout doesn't seem to adapt to the space needed by the software keyboard - and likewise doesn't vertically center the views when a hardware keyboard is used. I'm not sure if we should deal with that right away, or - in the name of faster progress - leave it as a TODO.
| thumbnailImageView.image = item.icon(fitInSize: thumbnailSize) | ||
|
|
||
| if item.thumbnailAvailability != .none { | ||
| let displayThumbnail = { (thumbnail: OCItemThumbnail?) in |
There was a problem hiding this comment.
Since displayThumbnail is only used in the retrieveHandler, maybe inline that code there rather than creating a separate block for it.
| private var thumbnailHeightAnchorConstraint: NSLayoutConstraint | ||
|
|
||
| private var stackViewLeftAnchorConstraint: NSLayoutConstraint? | ||
| private var stackviewRightAnchorConstraint: NSLayoutConstraint? |
There was a problem hiding this comment.
Small typo here, you likely meant to start both with either stackview or stackView.
| import UIKit | ||
| import ownCloudSDK | ||
|
|
||
| class RenameViewController: UIViewController { |
There was a problem hiding this comment.
Since we can use the same kind of UI for different operations (like naming newly created folders f.ex.), would be great to give this a different name and make it reusable for other purposes.
Thanks to the use of a completionHandler, we're already there, but for truly general purpose use of the view controller, this is still missing:
- a block that allows validation of the entered string (like the part where you currently check for forbidden characters)
- the
itemToRenameproperty should be renamed to something neutral - it should be possible to provide a string for the text field independent from the item (in case there is no item yet, like f.ex. when creating a new folder). Ideally the text field's content and that additional string property are kept in sync.
- this view controller should also be passed to the completionHandler, so the completionHandler has access to all of its properties
|
|
||
| @objc private func doneButtonPressed() { | ||
| if nameTextField.text!.contains("/") || nameTextField.text!.contains("\\") { | ||
| let controller = UIAlertController(title: "Forbiden Characters".localized, message: "File name cannot contain / or \\".localized, preferredStyle: .alert) |
There was a problem hiding this comment.
Small typo here: should probably read "Forbidden characters."
| }) | ||
| } | ||
|
|
||
| _ = core!.retrieveThumbnail(for: item, maximumSize: self.thumbnailSize, scale: 0, retrieveHandler: { (error, _, _, thumbnail, _, progress) in |
There was a problem hiding this comment.
Be careful not to capture self strongly here (I think the displayThumbnail block captures it, and this captures displayThumbnail), since that block can be kept around for an extended amount of time.
| self.core = core | ||
| self.completion = completion | ||
|
|
||
| blurView = UIVisualEffectView.init(effect: UIBlurEffect(style: .regular)) |
There was a problem hiding this comment.
The view controller currently doesn't support theming. For this to really work well, however, it should also be possible to request the blur effect from the theme as well, so it fits in.
Since both is best done together, and the theme just isn't there yet, please just leave a TODO for theming support for now, so we can deal with it in the future.
| let renamevc = RenameViewController(with: item, core: self.core, completion: { newName in | ||
| var summary : ProgressSummary = ProgressSummary(indeterminate: true, progress: 1.0, message: nil, progressCount: 1) | ||
| summary.message = NSString(format: "Renaming to %@".localized as NSString, newName) as String | ||
| self.queryProgressSummary = summary |
There was a problem hiding this comment.
The queryProgressSummary is reserved for usage by the OCQuery driving the ClientQueryViewController. To add progress information for the rename, please use the Progress object returned by OCCore.move and add it to the ClientQueryViewController's progressSummarizer via startTracking. The ProgressSummarizer will automatically remove the progress object once it indicates that it is done, so that's all. Like for deletion, the SDK currently doesn't provide progress information for moving, but this will change in a future SDK version.
There was a problem hiding this comment.
I've seen the changes you made in the feature/delete pull request and I'll change it to that way!
| @@ -80,7 +80,7 @@ extension NSObject { | |||
| let navigationController : UINavigationController = (self as? UINavigationController)! | |||
|
|
|||
| navigationController.navigationBar.applyThemeCollection(collection, itemStyle: itemStyle) | |||
| navigationController.view.backgroundColor = collection.tableBackgroundColor | |||
| //navigationController.view.backgroundColor = collection.tableBackgroundColor | |||
There was a problem hiding this comment.
Why did you comment out this line?
There was a problem hiding this comment.
I've left this line commented because when you launch a ThemeNavigationController with a RootViewController that has some transparency or blur view, this navigation controller background color interferes with the background color of your previous view you only see this color or this color with a blur. I've checked the app with the different themes we already support and I didn't notice any change.
Is there another better solution taking advantage of our Theme support mechanism?
There was a problem hiding this comment.
Good point that was off my radar! Did you notice any regressions or visual glitches anywhere by leaving this line out?
| @@ -23,4 +23,8 @@ extension String { | |||
| var localized: String { | |||
| return NSLocalizedString(self, comment: "") | |||
| } | |||
|
|
|||
| func fileExtension() -> String? { | |||
| return NSURL(fileURLWithPath: self).pathExtension | |||
There was a problem hiding this comment.
You can achieve them same with less overhead with return (self as NSString).pathExtension. Since this method is only called once in the code, I would also suggest inlining it in the sole place that it is called from.
There was a problem hiding this comment.
Maybe I can change the location of this method to OCItem+Extension.swift renaming it to item extension to be able to reuse it in the future if it's needed instead of coding it again. Or even create a new property in OCItem+Extension.swift like:
var extension: String {
return (name as NSString).pathExtension
}What do you think?
There was a problem hiding this comment.
extension, of course, would be best, but I'm afraid of colliding with what Apple might provide in the future. What do you think about fileSuffix? That's recognizable for us, but not a reserved Swift keyboard and not what Apple could likely choose in the future (pathExtension).
pablocarmu
force-pushed
the
feature/rename
branch
from
c554eec to
e423db9
Compare
|
About the reported issues: (2) and (4) are adressed to the SDK On the other hand: layout in landscape is OK and suggestions bar correctly removed. |
(6)UX improvement:
Current: Alert is shown, because the name already exists. |
|
All stuff reviewed from my side and it is OK. |
- Removed the background color for UINavigationController in NSObject+Theme.swift to prevent errors with blur backgrounds. - Coded a trailing swipe in ClientQueryViewController.swift with the rename action.
…extension remains untouched for consistency/security.
pablocarmu
force-pushed
the
feature/rename
branch
from
4bd0832 to
b261881
Compare
- also call completionHandler if cancel is hit - make use of tuple result easier to read/maintain
Description
This pull request provides a way to rename items.
Related Issue
#21
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
BUGS & improvements