Feature/1260 Detecting and Cleaning Galleries #8119

autojeff · 2017-11-08T22:14:42Z

#1260 …detecting and cleaning galleries

Fixes #
Remove deprecated characters.count

To test:
Detection for gallery markup

Needs review: @aerych

…cting and cleaning galleries

houndci-bot · 2017-11-08T22:14:58Z

WordPress/Classes/ViewRelated/Views/WPRichText/WPRichTextFormatter.swift

@@ -332,12 +332,12 @@ class BlockquoteTagProcessor: HtmlTagProcessor {
        if !matched {
            return (parsedString, nil)
        }
-
+   


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-08T22:14:58Z

WordPress/Classes/ViewRelated/Views/WPRichText/WPRichTextFormatter.swift

@@ -6,7 +6,7 @@ import WordPressShared
 /// HTML tags that require special handling before the text is shown in a UITextView.
 ///
 class WPRichTextFormatter {
-
+ 


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-08T22:14:58Z

WordPress/Classes/ViewRelated/Views/WPRichText/WPRichTextFormatter.swift

@@ -332,12 +332,12 @@ class BlockquoteTagProcessor: HtmlTagProcessor {
        if !matched {
            return (parsedString, nil)
        }
-
+   


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-08T22:14:58Z

WordPress/Classes/ViewRelated/Views/WPRichText/WPRichTextFormatter.swift

@@ -6,7 +6,7 @@ import WordPressShared
 /// HTML tags that require special handling before the text is shown in a UITextView.
 ///
 class WPRichTextFormatter {
-
+ 


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

…, new ReaderGalleryCell for use in WPRichTextGallery

houndci-bot · 2017-11-12T21:57:07Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+    let cell : ReaderGalleryCell
+    let attachment : WPTextAttachment
+}
+


Trailing Newline Violation: Files should have a single trailing newline. (trailing_newline)

houndci-bot · 2017-11-12T21:57:07Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+
+struct GalleryMedia {
+    let cell : ReaderGalleryCell
+    let attachment : WPTextAttachment


Colon Violation: Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)

houndci-bot · 2017-11-12T21:57:07Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+}
+
+struct GalleryMedia {
+    let cell : ReaderGalleryCell


Colon Violation: Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)

houndci-bot · 2017-11-12T21:57:07Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+
+}
+
+extension WPRichTextGallery : UICollectionViewDelegateFlowLayout {


Colon Violation: Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)

houndci-bot · 2017-11-12T21:57:07Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+
+        collectionView.reloadData()
+    }
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-12T21:57:08Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+    func collectionView(_ collectionView: UICollectionView, layout collectionViewLayout: UICollectionViewLayout, sizeForItemAt indexPath: IndexPath) -> CGSize {
+        let rightInset = CGFloat(8.0)
+        let rightExtraContent = CGFloat(8.0)
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-12T21:57:08Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+}
+
+extension WPRichTextGallery : UICollectionViewDelegateFlowLayout {
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-12T21:57:08Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+
+        return cell
+    }
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-12T21:57:08Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+        }
+
+
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-12T21:57:08Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+            let media = GalleryMedia(cell: cell, attachment: attachment)
+            mediaArray.append(media)
+        }
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

autojeff · 2017-11-12T21:58:06Z

Gallery is now in the pull request.

Still to do: zoomed in gallery, pagination for inline gallery, clean up code, support for image subtitle.

…troller to work with gallery, delegate functions, changes to WPRichTextGallery to make it all work

more clean up, fix collectionview sizing a bit (needs better sizing for showing edges of past and previous cards and some attention to iPad)

houndci-bot · 2017-11-14T08:40:12Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+    }
+
+    func scrollViewDidEndDecelerating(_ scrollView: UIScrollView) {
+        scrollToPage(scrollView, withVelocity: CGPoint(x:0, y:0))


Colon Violation: Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)

houndci-bot · 2017-11-14T08:40:12Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+        page = max(page, 0)
+        let newOffset: CGFloat = CGFloat(page) * (cellWidth + cellPadding)
+
+        scrollView.setContentOffset(CGPoint(x:newOffset, y:0), animated: true)


Colon Violation: Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)

houndci-bot · 2017-11-14T08:40:12Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+
+    fileprivate var collectionView: UICollectionView
+    var captionLabel : UILabel?
+    var pageLabel : UILabel?


Colon Violation: Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)

houndci-bot · 2017-11-14T08:40:12Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+    }()
+
+    fileprivate var collectionView: UICollectionView
+    var captionLabel : UILabel?


Colon Violation: Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)

houndci-bot · 2017-11-14T08:40:13Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+    var contentURL: URL?
+    var linkURL: URL?
+
+    var cellSize : CGFloat = 10


Colon Violation: Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)

houndci-bot · 2017-11-14T08:40:13Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+
+        scrollView.setContentOffset(CGPoint(x:newOffset, y:0), animated: true)
+    }
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-14T08:40:13Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+        }
+        page = max(page, 0)
+        let newOffset: CGFloat = CGFloat(page) * (cellWidth + cellPadding)
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-14T08:40:13Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+    func scrollToPage(_ scrollView: UIScrollView, withVelocity velocity: CGPoint) {
+        let cellWidth: CGFloat = cellSize
+        let cellPadding: CGFloat = 8
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-14T08:40:13Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+    func scrollViewWillEndDragging(_ scrollView: UIScrollView, withVelocity velocity: CGPoint, targetContentOffset: UnsafeMutablePointer<CGPoint>) {
+        scrollToPage(scrollView, withVelocity: velocity)
+    }
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-14T08:40:13Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+    }
+
+    /* In case the user scrolls for a long swipe, the scroll view should animate to the nearest page when the scrollview decelerated. */
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

aerych

Howdy @autojeff 👋 You've been busy!
I've taken an early peek through the code so far. I've noted a few things and made a few suggestions.
I also had a couple of questions.
I did take a peek at the UI but I know there are things you're still looking at there so I'm holding my comments. :)
I'm happy to chat about any of the comments I've made or clarify anything that might not be clear. Just let me know.

aerych · 2017-11-14T23:29:42Z

WordPress/Classes/Extensions/Array.swift

@@ -12,4 +12,10 @@ extension Array {
            }
        }
    }
+
+    //Safely returns an index from an array.


Is //Safely is missing a / ?

@aerych Do you mean like this? "/// Safely"

aerych · 2017-11-14T23:38:53Z

WordPress/Classes/ViewRelated/Reader/ReaderGalleryCell.swift

+//
+//  Created by Jeff Jacka on 11/11/17.
+//  Copyright © 2017 WordPress. All rights reserved.
+//


We try to avoid this sort of header comment.

aerych · 2017-11-14T23:42:53Z

WordPress/Classes/ViewRelated/Reader/ReaderGalleryCell.swift

+
+    @IBOutlet weak var galleryImageView: UIImageView!
+
+    override func awakeFromNib() {


Maybe remove this overridden method if all it will do is call the super implementation.

aerych · 2017-11-14T23:43:32Z

WordPress/Classes/ViewRelated/Reader/WPGalleryPageViewController.swift

+//
+//  Created by Jeff Jacka on 11/12/17.
+//  Copyright © 2017 WordPress. All rights reserved.
+//


Same as before about file header comments.

aerych · 2017-11-14T23:49:54Z

WordPress/Classes/ViewRelated/Reader/WPGalleryPageViewController.swift

+        }
+
+        dataSource = self
+        // Do any additional setup after loading the view.


It would be nice if we could clean up boilerplate comments like this one.

aerych · 2017-11-15T03:53:07Z

WordPressShared/WordPressShared/Core/Utility/RichContentFormatter.swift

+
+        //Replace Div with Gallery tag
+        content = RegEx.galleryDivStart.stringByReplacingMatches(in: content, options: .reportCompletion, range: NSRange(location: 0, length: content.count), withTemplate: galleryStart)
+        content = RegEx.galleryDivEnd.stringByReplacingMatches(in: content, options: .reportCompletion, range: NSRange(location: 0, length: content.count), withTemplate: galleryEnd)


I'm curious about the use of regular expressions to handle string replacement rather than have it handled by the Scanner. Was there a particular reason that made regex a better choice?

My thinking behind this was to the keep the logic a bit more simple. This function scanForGalleryBy(galleryType : String, inString : String) only scans and identifies where a gallery should start and end. Then clean it up with the RegEx, rather than using another Scanner. It could be done in one go, but this felt like a safe, reliable option to me.

aerych · 2017-11-15T03:54:51Z

WordPressShared/WordPressShared/Core/Utility/RichContentFormatter.swift


        // Trailing BR Tags
        static let trailingBRTags = try! NSRegularExpression(pattern: "(\\s*<br\\s*(/?)\\s*>\\s*)+$", options: .caseInsensitive)
+
+
+


Maybe remove the newly added returns?

aerych · 2017-11-15T03:56:33Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+
+    func contentRatio() -> CGFloat {
+
+


We probably don't need the white space between the method definition and body here and above.

aerych · 2017-11-15T03:58:21Z

WordPressShared/WordPressShared/Core/Utility/RichContentFormatter.swift

+
+            if galleriesFound <= 0 {
+                //check for gallery
+                galleryScanner.scanUpTo(galleryType, into: &tempStr)


The if and else blocks share some similarity in the way the start. Maybe there is an opportunity for a small refactor to reuse some code?

aerych · 2017-11-15T04:03:10Z

WordPress/Classes/ViewRelated/Reader/WPImageViewController.m

@@ -102,8 +109,10 @@ - (void)viewDidLoad
    [tgr1 requireGestureRecognizerToFail:tgr2];
    [self.scrollView addGestureRecognizer:tgr1];

-    self.flingableViewHandler = [[FlingableViewHandler alloc] initWithTargetView:self.scrollView];
-    self.flingableViewHandler.delegate = self;
+    if (!self.galleryMode) {


Since galleryMode is only flagging whether the FlingableViewHandle should be used, it might be nice to rename it according to how its used. Then if we extend WPImageViewController in the future and add more inits, the flag could be reused in a new context. Make sense?

…, fix labels not showing up until swiped

…a bit better

houndci-bot · 2017-11-16T05:49:32Z

WordPress/Classes/ViewRelated/System/FlingableViewHandler.swift

+    fileprivate func handleSwipeGesture(_ recognizer: UISwipeGestureRecognizer) {
+
+        isActive = false
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T05:49:32Z

WordPress/Classes/ViewRelated/System/FlingableViewHandler.swift

+
+    @objc
+    fileprivate func handleSwipeGesture(_ recognizer: UISwipeGestureRecognizer) {
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T05:49:32Z

WordPress/Classes/ViewRelated/System/FlingableViewHandler.swift

        targetView.addGestureRecognizer(panGestureRecognizer)
        isActive = true
    }
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T05:49:32Z

WordPress/Classes/ViewRelated/System/FlingableViewHandler.swift

+        //Allow swipe gestures to pass through first
+        panGestureRecognizer.require(toFail: leftSwipe)
+        panGestureRecognizer.require(toFail: rightSwipe)
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T05:49:32Z

WordPress/Classes/ViewRelated/System/FlingableViewHandler.swift


        panGestureRecognizer = UIPanGestureRecognizer(target: self, action: #selector(handlePanGesture(_:)))
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T05:49:32Z

WordPress/Classes/ViewRelated/System/FlingableViewHandler.swift

+        let leftSwipe = UISwipeGestureRecognizer(target: self, action: #selector(handleSwipeGesture(_:)))
+        leftSwipe.direction = .left
+        targetView.addGestureRecognizer(leftSwipe)
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T05:49:33Z

WordPress/Classes/ViewRelated/System/FlingableViewHandler.swift

@@ -43,11 +43,31 @@ class FlingableViewHandler: NSObject {
        animator = UIDynamicAnimator(referenceView: targetView.superview!)

        super.init()
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

fix gallery padding so they align correctly

houndci-bot · 2017-11-16T09:56:37Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+}
+
+extension WPRichTextGallery: UICollectionViewDelegateFlowLayout {
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T09:56:38Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+
+        collectionView.showsHorizontalScrollIndicator = false
+    }
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T09:56:38Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+
+        setupCollectionView()
+        setupLabels()
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T09:56:38Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+
+    override open var frame: CGRect {
+        didSet {
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

frosty

I've just been reviewing this, but don't quite have time to go through it all right now – so I'll finish up later.

Functionally, this seems to work pretty well! The galleries are parsed out, I can view the images and flick through them, which is great. I've added bunch code comments - a lot of it is style or naming or organization, rather than fundamental issues. Hopefully it's all useful, but let me know if you have any questions!

Two functional issues I did see:

I think it'd be good if we set the background colour behind the gallery to black, otherwise you see it when you scroll to the edge:

Also the status bar is black-on-black there – again, you can see it when you scroll off. We should probably hide it altogether or make it lightContent style.

I think it's largely down to UIPageViewController, but you can see the images appear / disappear as you scroll, which is a little jarring. It'd be nice if those transitions happen offscreen:

frosty · 2017-11-16T13:43:16Z

WordPress/Classes/ViewRelated/Reader/ReaderDetailViewController.swift

@@ -1076,6 +1076,16 @@ extension ReaderDetailViewController: WPRichContentViewDelegate {

        present(controller, animated: true, completion: nil)
    }
+
+    func richContentView(_ richContentView: WPRichContentView, didReceiveGalleryAction image: [WPTextAttachment], indexTapped index: Int) {


The second parameter name here feels slightly confusing, I think. didReceiveGalleryAction doesn't really explain what the parameter contains, and the inner name is image but it looks like it contains an array of images (plural)? How about didReceiveGalleryActionWithImages images: [WPTextAttachment]?

frosty · 2017-11-16T13:47:20Z

WordPress/Classes/ViewRelated/Reader/WPGalleryPageViewController.swift

+    ///
+    /// - Return: A WPGalleryPageViewController instance.
+    ///
+    open class func controllerWithImages(_ images: [WPTextAttachment], selectedIndex: Int) -> WPGalleryPageViewController {


I think in Swift 3 / 4, this might be best named something like controller(with images: [WPTextAttachment], selectedIndex: Int)

In fact, we could probably add a default value for selected index (= 0?) to allow it to be omitted?

frosty · 2017-11-16T13:49:20Z

WordPress/Classes/ViewRelated/Reader/WPGalleryPageViewController.swift

+    override func viewDidLoad() {
+        super.viewDidLoad()
+
+        if let initalVC = imageViewControllerForIndex(index: initialIndex) {


Typo: initalVC -> initialVC

frosty · 2017-11-16T13:50:23Z

WordPress/Classes/ViewRelated/Reader/WPGalleryPageViewController.swift

+        dataSource = self
+    }
+
+    func imageViewControllerForIndex(index: Int) -> WPImageViewController? {


Swift's API guidelines (as far as I know) recommend against duplicating parameter names like this – something like imageViewController(for index: Int) or imageViewControllerFor(index: Int) would be a better fit, I think.

frosty · 2017-11-16T13:53:37Z

WordPress/Classes/ViewRelated/Reader/WPGalleryPageViewController.swift

+            return nil
+        }
+
+        let urlString = imageAttachment.attributes?["data-large-file"] ?? imageAttachment.src


It might be good to pull "data-large-file" out into a constant.

frosty · 2017-11-16T14:40:38Z

WordPress/Classes/ViewRelated/Views/WPRichText/WPRichContentView.swift

@@ -5,6 +5,7 @@ import WordPressShared

 @objc protocol WPRichContentViewDelegate: UITextViewDelegate {
    func richContentView(_ richContentView: WPRichContentView, didReceiveImageAction image: WPRichTextImage)
+    func richContentView(_ richContentView: WPRichContentView, didReceiveGalleryAction image: [WPTextAttachment], indexTapped index: Int)


See earlier comment about the name of this method.

frosty · 2017-11-16T14:41:59Z

WordPress/Classes/ViewRelated/Views/WPRichText/WPRichContentView.swift

@@ -283,6 +286,48 @@ extension WPRichContentView: WPTextAttachmentManagerDelegate {
        return img
    }

+    func galleryForAttachment(_ attachment: WPTextAttachment) -> WPRichTextGallery {
+
+        let width: CGFloat = attachment.width > 0 ? attachment.width : self.bounds.width


You can omit self. here.

frosty · 2017-11-16T14:43:47Z

WordPress/Classes/ViewRelated/Views/WPRichText/WPRichContentView.swift

+            let formatter = WPRichTextFormatter()
+            let (_, attachments) = formatter.processAndExtractTags(content)
+
+            gallery.imageAttachments = attachments.filter({ (attachment) -> Bool in


You could condense this if you like, to attachments.filter({ $0.tagName == "img" })

frosty · 2017-11-16T14:49:30Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+    let layout: UICollectionViewFlowLayout = {
+        let layout = UICollectionViewFlowLayout()
+        layout.scrollDirection = .horizontal
+        layout.minimumInteritemSpacing = 8.0


We could use a constant for these values – or should this be using cellPadding?

frosty · 2017-11-16T14:51:04Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+            }
+        }
+    }
+    var handleGalleryTapped : ((_ selectedIndex: Int, _ galleryImages: [WPTextAttachment]) -> ())?


Nitpick, but there shouldn't be a space before a : here. Also -> Void is preferred over -> ().

frosty

Okay, I added a few more comments! Let me know if you have any questions 👍

frosty · 2017-11-16T19:49:14Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+        let source = WPTableImageSource(maxSize: self.maxDisplaySize)
+        source?.delegate = self
+        source?.forceLargerSizeWhenFetching = false
+        source?.photonQuality = 65


We should have a constant for this – looks like WPRichContentView has one in its Constants struct.

frosty · 2017-11-16T19:49:33Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+    ///
+
+    fileprivate let galleryCellIdentifier = "ReaderGalleryCell"
+    fileprivate let contentUrlKey = "contentUrl"


These identifiers could be grouped together in a struct or enum.

frosty · 2017-11-16T19:51:28Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+        setupCollectionView()
+        setupLabels()
+
+        collectionView.isPrefetchingEnabled = true


Can this just go in setupCollectionView?

frosty · 2017-11-16T19:53:45Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+        aCoder.encode(footerStack, forKey: UIStackView.classNameWithoutNamespaces())
+
+        if let url = contentURL {
+            aCoder.encode(url, forKey: "contentURL")


We should use the constant keys defined at the top of the file here - contentUrlKey, etc.

frosty · 2017-11-16T19:55:50Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+extension WPRichTextGallery: UICollectionViewDelegateFlowLayout {
+
+    func setOffsetIfNeeded() {
+        //TODO: Still Needs Fixing


Is this still valid? What's the issue exactly?

Yes it is, you made a nice gif of the issue I'm having with the gallery UICollectionView so a few things need to be changed in this area. The problem I am having is that when my gallery gets it frame from the UIRichContentView/TextView it is respecting the contentInset from that textView, however this causes issues with the UICollectionView because from the design I have attempted to build it should probably be the full width of the app to prevent that disappearing issue you showed here (https://user-images.githubusercontent.com/4780/32897805-738be56a-cade-11e7-80b3-3d1aa4b5845a.gif). So I think I have a two options, change the design or stretch the frame to be the width of the screen, use contentInsets to handle some of the padding, this should make the disappearing issue go away. Would love some feedback/opinions on what the best options here might be from both a design and development standpoint.

frosty · 2017-11-16T19:58:34Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+    ///
+
+    func scrollViewWillEndDragging(_ scrollView: UIScrollView, withVelocity velocity: CGPoint, targetContentOffset: UnsafeMutablePointer<CGPoint>) {
+        scrollToPage(scrollView, withVelocity: velocity)


Ideally, rather than calling setContentOffset from this method, you should set targetContentOffset to the offset that you calculate – this will result in a smoother deceleration, instead of stopping in the wrong place and then animating to the correct one.

frosty · 2017-11-16T20:00:09Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+    }
+
+    func scrollViewDidEndDecelerating(_ scrollView: UIScrollView) {
+        scrollToPage(scrollView, withVelocity: CGPoint(x: 0, y: 0))


Is this necessary if scrollViewWillEndDragging is implemented and sets an adjusted offset?

frosty · 2017-11-16T20:01:16Z

WordPressShared/WordPressShared/Core/Utility/RichContentFormatter.swift

+    ///
+    /// - Returns: The formatted string.
+    ///
+    public class func normalizeGallery(_ string : String) -> String {


Nitpick, no space before :

frosty · 2017-11-16T20:02:14Z

WordPressShared/WordPressShared/Core/Utility/RichContentFormatter.swift

+        var tempStr: NSString? = ""
+
+        var divCounter = 0
+        var galleriesFound : Int = 0


Nitpick, no space before :

frosty · 2017-11-16T20:05:34Z

WordPressShared/WordPressShared/Core/Utility/RichContentFormatter.swift

+        return content
+    }
+
+    private class func scanForGalleryBy(galleryType : String, inString : String) -> String {


Are we able to break this method down at all, to make it a little easier to follow? Can we add some tests?

…tionary to store WPImageViewControllers in use

houndci-bot · 2017-11-16T23:37:19Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+
+        setupCollectionView()
+        setupLabels()
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T23:37:19Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+        }
+
+        aCoder.encode(viewHeight, forKey: viewHeightKey)
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T23:37:20Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+        if let url = linkURL {
+            aCoder.encode(url, forKey: linkUrlKey)
+        }
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T23:37:20Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+        if let url = contentURL {
+            aCoder.encode(url, forKey: contentUrlKey)
+        }
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T23:37:20Z

WordPress/Classes/ViewRelated/Views/WPRichTextGallery.swift

+
+        aCoder.encode(collectionView, forKey: UICollectionView.classNameWithoutNamespaces())
+        aCoder.encode(footerStack, forKey: UIStackView.classNameWithoutNamespaces())
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T23:37:20Z

WordPress/Classes/ViewRelated/Reader/WPGalleryPageViewController.swift

+extension WPGalleryPageViewController : UIPageViewControllerDelegate {
+
+    func pageViewController(_ pageViewController: UIPageViewController, didFinishAnimating finished: Bool, previousViewControllers: [UIViewController], transitionCompleted completed: Bool) {
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T23:37:21Z

WordPress/Classes/ViewRelated/Reader/WPGalleryPageViewController.swift

+}
+
+extension WPGalleryPageViewController : UIPageViewControllerDelegate {
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T23:37:21Z

WordPress/Classes/ViewRelated/Reader/WPGalleryPageViewController.swift

+        guard let index = imageViewControllers[imageVC] else {
+            return nil
+        }
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T23:37:21Z

WordPress/Classes/ViewRelated/Reader/WPGalleryPageViewController.swift

+        guard let imageVC = viewController  as? WPImageViewController else {
+            return nil
+        }
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

houndci-bot · 2017-11-16T23:37:21Z

WordPress/Classes/ViewRelated/Reader/WPGalleryPageViewController.swift

+        guard let imageVC = viewController  as? WPImageViewController else {
+            return nil
+        }
+


Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

aerych · 2017-11-27T22:57:47Z

Closing.

remove deprecated characters.count, create scanner and regex for dete…

9d73e2a

…cting and cleaning galleries

houndci-bot reviewed Nov 8, 2017

View reviewed changes

aerych added the [Status] DO NOT MERGE label Nov 8, 2017

autojeff changed the title ~~remove deprecated characters.count, create scanner and regex for dete…~~ Feature/1260 Detecting and Cleaning Galleries Nov 8, 2017

autojeff added 2 commits November 8, 2017 14:21

fix whitespace issues

c7335aa

new WPRichTextGallery, displays inline uicollectionview for a gallery…

8ad922e

…, new ReaderGalleryCell for use in WPRichTextGallery

houndci-bot reviewed Nov 12, 2017

View reviewed changes

autojeff added 5 commits November 12, 2017 20:29

new pageview controller for zoomed gallery, changes to wpimageviewcon…

975fb9b

…troller to work with gallery, delegate functions, changes to WPRichTextGallery to make it all work

clean up files/code no longer needed

9f94aca

clean up whitespace

23b4c60

gallery caption and page label

880fff1

clean up, work on gallery sizing

5f28c6f

more clean up, fix collectionview sizing a bit (needs better sizing for showing edges of past and previous cards and some attention to iPad)

houndci-bot reviewed Nov 14, 2017

View reviewed changes

run autocorrect, source credit

ef22dcf

aerych requested changes Nov 15, 2017

View reviewed changes

autojeff added 2 commits November 15, 2017 01:11

corrections from code review, refractoring, add support for alt title…

0298a00

…, fix labels not showing up until swiped

added gesture to flingable to make it work with uipageviewcontroller …

bdd8e66

…a bit better

houndci-bot reviewed Nov 16, 2017

View reviewed changes

autojeff added 2 commits November 15, 2017 21:51

clean up code

6128bb3

fix gallery padding

88d5b26

fix gallery padding so they align correctly

houndci-bot reviewed Nov 16, 2017

View reviewed changes

frosty requested changes Nov 16, 2017

View reviewed changes

autojeff added 2 commits November 16, 2017 13:56

remove index from wpimageviewcontroller

d92c1a4

remove index usage from WPImageViewController, clean up code, add dic…

1affd4b

…tionary to store WPImageViewControllers in use

houndci-bot reviewed Nov 16, 2017

View reviewed changes

clean up whitespace/autocorrect

3225d36

aerych closed this Nov 27, 2017


		}

		extension WPRichTextGallery : UICollectionViewDelegateFlowLayout {


		scrollView.setContentOffset(CGPoint(x:newOffset, y:0), animated: true)
		}

		}

		/* In case the user scrolls for a long swipe, the scroll view should animate to the nearest page when the scrollview decelerated. */


		@IBOutlet weak var galleryImageView: UIImageView!

		override func awakeFromNib() {


		// Trailing BR Tags
		static let trailingBRTags = try! NSRegularExpression(pattern: "(\\s<br\\s(/?)\\s>\\s)+$", options: .caseInsensitive)

		fileprivate func handleSwipeGesture(_ recognizer: UISwipeGestureRecognizer) {

		isActive = false


		@objc
		fileprivate func handleSwipeGesture(_ recognizer: UISwipeGestureRecognizer) {


		panGestureRecognizer = UIPanGestureRecognizer(target: self, action: #selector(handlePanGesture(_:)))

		@@ -43,11 +43,31 @@ class FlingableViewHandler: NSObject {
		animator = UIDynamicAnimator(referenceView: targetView.superview!)

		super.init()

		}

		extension WPRichTextGallery: UICollectionViewDelegateFlowLayout {


		aCoder.encode(collectionView, forKey: UICollectionView.classNameWithoutNamespaces())
		aCoder.encode(footerStack, forKey: UIStackView.classNameWithoutNamespaces())

		extension WPGalleryPageViewController : UIPageViewControllerDelegate {

		func pageViewController(_ pageViewController: UIPageViewController, didFinishAnimating finished: Bool, previousViewControllers: [UIViewController], transitionCompleted completed: Bool) {

Feature/1260 Detecting and Cleaning Galleries #8119

Feature/1260 Detecting and Cleaning Galleries #8119

Conversation

autojeff commented Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autojeff commented Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerych left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frosty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frosty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autojeff commented Nov 8, 2017 •

edited

Loading

autojeff commented Nov 12, 2017 •

edited

Loading