watermark feature for iOS App #1420
Conversation
|
|
|
@blicknix THX for your contribution - please sign the CLA as pointed out by the bot above. THX |
| "ar" : { | ||
| "stringUnit" : { | ||
| "state" : "translated", | ||
| "value" : "أنا أفهم" |
There was a problem hiding this comment.
For the future: translations will happen on https://app.transifex.com/owncloud-org/owncloud-ios/dashboard/
Any change done in the source code will be overwritten on a daily basis once merged.
There was a problem hiding this comment.
Thanks for the contribution.
Please see my comments regarding specific code snippets.
On a more general note, I'd like to add that modularity is an important aspect in this project and additions like yours should be as modular as possible. Right now, your changes are woven in directly in many places that don't have any direct contextual relationship to the added features.
To fit in the project, I'd therefore like to ask you to change that structure:
- please create a new folder
ownCloudAppShared/User Interface/Watermarkand put your code additions in that folder as far as possible - please put your code in files of their own rather than appending it to existing source code files
- the approach to add watermark views in every
UIViewControllersubclass that might show content has a number of drawbacks and weaknesses:- new views controllers can emerge where this is not thought of or can't be accounted for
- new subclasses of already modified classes may/will add views on top
- the approach generally fails if a modified view controller dynamically adds and removes views (as the Watermark will then no longer be the front most view and be covered by the views that were added after it)
- the code changes spread to unrelated sourcecode, weakening modularity
- that said,
ThemeWindow(containing all view controllers) or especiallyBrowserNavigationViewController(containing all (content) view controllers) strike me as good, central places to apply a watermark without having to modify so many different view controllers and micromanage their views - ideally the watermark feature should be removable from the app by deleting a folder and otherwise removing 1-3 lines of code elsewhere
Other than that:
- please run SwiftLint and correct the warnings in your code (there are many)
- please read and accept the CLA
| moreViewController.watermark( | ||
| username: core.bookmark.userName, | ||
| userMail: core.bookmark.user?.emailAddress | ||
| ) |
There was a problem hiding this comment.
This and similar code is repeated several times across in different places. It would require changes if
- the location of the extracted userName or emailAdress changes
- the data that should be extracted from the Bookmark changes
Passing the bookmark directly and extracting the desired information in thewatermarkmethod would be a more flexible and less repetitive pattern.
| static let watermarkFontSize = OCClassSettingsKey("watermark-font-size") | ||
| static let watermarkText = OCClassSettingsKey("watermark-text") | ||
| static let watermarkShowMail = OCClassSettingsKey("watermark-show-mail") | ||
| static let watermarkShowDate = OCClassSettingsKey("watermark-show-date") |
There was a problem hiding this comment.
Since the watermark options here are so numerous, related and for a specific feature only, it would be good to move them to their own category (classSettingsIdentifier) and class rather than adding them to the broader VendorServices / app category.
| .watermarkText: "", | ||
| .watermarkShowMail: true, | ||
| .watermarkShowDate: true, | ||
| .showScreenshotNotification: true, |
There was a problem hiding this comment.
There's a trailing comma here that should be removed.
| @@ -17,6 +17,8 @@ | |||
| */ | |||
|
|
|||
| import UIKit | |||
| import ownCloudApp | |||
There was a problem hiding this comment.
This change is not needed and should be removed.
| @@ -48,3 +49,273 @@ public extension UIViewController { | |||
| return true | |||
| } | |||
| } | |||
|
|
|||
| public extension UIViewController { | |||
| func observeScreenshotEvent() { | |||
There was a problem hiding this comment.
Please take a different approach to observe for UIApplication.userDidTakeScreenshotNotification. Setting up and removing an observer in every view controller has several issues:
- if a view controller already presents another view controller when this notification is observed, the alert will not be shown
- there can (and likely will) be several active observers for screenshots, which could result in several alerts being shown for a single notification
- the app also presents system-provided
UIViewControllers, which, for this approach to work, would need all be subclassed and all calling sites be replaced. And even then, the system presenting additional view controllers of its own on those subclassed view controllers would still not be covered. - structurally, these methods have nothing to do with
UIViewControllerand don't belong here.
A solution to all of the above would be to create a new class (f.ex. as a singleton), centrally managing the observation for UIApplication.userDidTakeScreenshotNotification and - if the event occurs - dynamically determining the front-most UIViewController and presenting the alert on that. See AppRootViewController.presentAlertAsCard() for an example on how to do that.
| username: username, | ||
| userMail: userMail | ||
| ) | ||
| view.addSubview(watermark) |
There was a problem hiding this comment.
This method creates and adds a new Watermark view to the UIViewController even if the watermark is not enabled. To avoid unnecessary usage of CPU and memory resources, this should not be done.
| watermark.leadingAnchor.constraint(equalTo: view.leadingAnchor), | ||
| watermark.trailingAnchor.constraint(equalTo: view.trailingAnchor), | ||
| watermark.bottomAnchor.constraint(equalTo: view.safeAreaLayoutGuide.bottomAnchor) | ||
| ]) |
There was a problem hiding this comment.
The whole segment of code with addSubview(), translatesAutoresizingMaskIntoConstraints and NSLayoutConstraint.activate can be replaced with a single line call of UIView.embed(toFillWith:).
| username: String?, | ||
| userMail: String? | ||
| ){ | ||
| let watermark = Watermark( |
There was a problem hiding this comment.
To be consistent with the rest of the code, that variable should end with View, so f.ex. be called watermarkView.
| class Watermark: UIView { | ||
|
|
||
| var angle: CGFloat = 45.0 { | ||
| didSet { updateView() } |
There was a problem hiding this comment.
All of the didSets here are superfluous as:
- the setters are not called anywhere but from
init(), from wheredidSetis generally not called - (if they were, the base class would not yet have been initialized - and even if it were, the size would be 0 x 0 and it wouldn't make sense to layout for that view size)
| } | ||
| } | ||
|
|
||
| class Watermark: UIView { |
There was a problem hiding this comment.
This view is implemented by filling it with further UILabel views, which are manually removed, re-added and repositioned on every change, with change detection hinging on an - as you write yourself "not ideal" - method override of updateConstraints() with the inherent risk of causing issues like infinite loops or degraded performance in the future.
Possible solutions to this:
- using the layout constraint system to layout the subviews (as in the rest of the app) automatically
- overriding
drawRect()and using the iOS graphics system to actually draw the strings in question
If watermarks are meant to be shown more densely, requiring more UILabels to be added as subviews, the latter option should be a lot more performant and - for that reason - would be my first choice for adding such a feature.
|
@blicknix rebase is also necessary as we just added GitHub workflows - THX |
There was a problem hiding this comment.
Some general thoughts about this feature:
This feature isn’t secure if it’s possible to open files from third-party apps. For instance, if you can open the same file via File Provider with another app or Files.app and take a screenshot, this feature isn’t secure.
If this feature is enabled, I suggest not delivering files via File Provider or showing an error message if the user wants to open or copy the file with a hint. Instead, open the ownCloud app to view the file. Additionally, it shouldn’t be possible to share files from the app.
Another idea to consider is an option instead of watermarking, where you can completely hide the content or disallow taking screenshots (perhaps with an overlay view or DRM content view).
|
@blicknix please add a detailed description about the MDM parameters to the PR and add meta data to the class settings. |
Description
We implemented a watermark feature which can be enabled with MDM Tools.
Also we implemented a feature which allows to add a notification if a screenshot was made in the app.
Related Issue
Motivation and Context
For audit reasons we want to add watermarks to each screen which shows personal data.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: