Issue/8045 aztec image links #786

jim-rhoades · 2017-10-27T15:45:40Z

This is the Aztec related work needed to implement adding links to images in posts from its detail editor.

One small detail to note is that in the example app, the linkURL is automatically set to the same thing as the file URL after the image is added in EditorDemoController.

Doing it that way, rather than automatically setting it within ImageAttachment, leaves it up to the developer integrating AztecEditor into their app to decide whether or not they want to automatically link images to themselves.

To test:

run the example app and tap "Empty Editor Demo"
tap the text view to bring up the format bar
tap the "+" button in the lower left corner to add an image
once you've added the image, tap on it to show "Tap for options"
tap the image again, then tap "Media Details"
note that the link URL is shown (change it to something else if you want)
tap "Done"
bring up the format bar again and tap the </> button to show that the link URL is shown within the HTML (with an <a href></a> containing the URL around the img tag)
if you want, edit the URL within the HTML
tap the </> button again to show that the HTML is properly converted back to an NSAttributedString... and that the image displays properly
bring up the image media details again, and observe that the link URL is still set (because it was set after converting back from HTML)

…ibuted string. Fixed issue with converting a link to HTML.

… is added

diegoreymendez · 2017-10-27T15:48:12Z

@jim-rhoades - Can you provide some sample testing steps? You can check out some of our other PRs for examples.

jim-rhoades · 2017-10-27T15:57:26Z

Testing steps added!

diegoreymendez

@jim-rhoades - I started with a logic and style check and added a few comments.

Let me know if there's anything.

diegoreymendez · 2017-10-27T15:57:37Z

Aztec/Classes/NSAttributedString/Conversions/AttributedStringSerializer.swift

-                let childContent = serialize(child, inheriting: childAttributes)
-                content.append(childContent)
+            // if the element is of type '.a' and contains a '.img'
+            if element.isNodeType(.a) && element.children.count == 1 {


Readability / Maintainability:

From this point onward we have 2 if conditions that could be reduced to a single method call for clarity. Something along the lines of:

if isLinkedImage(element) { // calculate content here }

diegoreymendez · 2017-10-27T16:02:23Z

Aztec/Classes/NSAttributedString/Conversions/AttributedStringSerializer.swift

+                if let imgElement = element.children.first as? ElementNode, imgElement.isNodeType(.img) {
+
+                    // get the link URL and assign it to the image attachment
+                    if let linkText = element.stringValueForAttribute(named: "href") {


Logic: If this is false, we're losing content.

We should actually set linkText to "" when href is missing.

diegoreymendez · 2017-10-27T16:08:41Z

Aztec/Classes/NSAttributedString/Conversions/AttributedStringSerializer.swift

+                    // get the link URL and assign it to the image attachment
+                    if let linkText = element.stringValueForAttribute(named: "href") {
+                        let imgAttributes = self.attributes(for: imgElement, inheriting: attributes)
+                        if let attachment = imgAttributes[NSAttachmentAttributeName] as? ImageAttachment,


Logic: There's a problem here since if this is false, we're losing content.

The truth is that the code we currently have doesn't support this scenario very well, so I'd recommend to deal with the other comments, and once that's done let's discuss this part of the code in Slack to try and provide a nice solution for it.

…dified code for clarity.

jim-rhoades · 2017-10-27T18:38:33Z

I've made some changes to address the first two issues you mentioned, which also fixed the problem with one of the tests not passing.

diegoreymendez

Nice changes. I particularly like that you documented the new method.

I've added a few extra comments to try and improve both readability and maintainability.

Once we're done with these we chat to figure out the remaining logic.

diegoreymendez · 2017-10-27T18:52:08Z

Aztec/Classes/NSAttributedString/Conversions/AttributedStringSerializer.swift

+            // if the node is of type '.a' with one child of type '.img', retrieve the '.img' node
+            if let imgElement = linkedImageElementFor(element) {
+                // get the link URL text
+                var linkText = ""


In order to avoid having linkText be a variable, you can reduce this and the next three lines into:

let linkText = element.stringValueForAttribute(named: "href") ?? ""

diegoreymendez · 2017-10-27T18:54:54Z

Aztec/Classes/NSAttributedString/Conversions/AttributedStringSerializer.swift

+    ///
+    /// - Returns: the child '.img' node if there is one
+    ///
+    fileprivate func linkedImageElementFor(_ element: ElementNode) -> ElementNode? {


This is really minor, but to align better with the rest of our code we'd normally declare this method as:

fileprivate func linkedImageElement(for element: ElementNode) -> ElementNode? {

diegoreymendez · 2017-10-27T18:55:41Z

Aztec/Classes/NSAttributedString/Conversions/AttributedStringSerializer.swift

+            return nil
+        }
+
+        guard let imgElement = element.children.first as? ElementNode, imgElement.isNodeType(.img) else {


You can group this up with the above guard, like so:

guard element.isNodeType(.a) && element.children.count == 1, let imgElement = element.children.first as? ElementNode, imgElement.isNodeType(.img) else { return nil }

diegoreymendez · 2017-10-27T18:58:40Z

Aztec/Classes/NSAttributedString/Conversions/AttributedStringSerializer.swift

-                            content.append(attributedString)
-                        }
-                    }
+            // if the node is of type '.a' with one child of type '.img', retrieve the '.img' node


We should remove this method since it describes internal details of linkedImageElementFor() that are already documented in the method.

diegoreymendez · 2017-10-27T19:04:03Z

Aztec/Classes/NSAttributedString/Conversions/AttributedStringSerializer.swift

-                    }
+            // if the node is of type '.a' with one child of type '.img', retrieve the '.img' node
+            if let imgElement = linkedImageElementFor(element) {
+                // get the link URL text


We'd normally avoid explaining in comments what the code does, since the code itself is readable enough to describe it.

In cases where the code is not easy to read, it normally means some effort can be put towards making it more readable (like you did with the method linkedImageElementFor()).

diegoreymendez

Added some comments.

diegoreymendez · 2017-10-30T13:28:27Z

Aztec/Classes/NSAttributedString/Conversions/AttributedStringParser.swift

            element.updateAttribute(named: key, value: .string(finalValue))
        }

+		// if the image attachment has a linkURL set,


Remove the comments that explain what the code does, since that should be evident by just reading the code.

diegoreymendez · 2017-10-30T14:04:30Z

Aztec/Classes/NSAttributedString/Conversions/AttributedStringSerializer.swift

+            if let imgElement = linkedImageElement(for: element) {
+                let linkText = element.stringValueForAttribute(named: "href") ?? ""
+                let imgAttributes = self.attributes(for: imgElement, inheriting: attributes)
+                if let attachment = imgAttributes[NSAttachmentAttributeName] as? ImageAttachment,


The specific problem we have at this point is that if this condition fails, we're losing content, since the element has no representation in the attributed string.

Upon inspecting the code in more depth I realized both of these conditions should succeed if we're dealing with an image element. If they ever failed it would mean we have broken logic somewhere.

The ideal solution would be to reengineer our formatters, but that'd be too big of a project for the scope of the trial.

In order to avoid a silent failure, we could use something like this:

let attachment = imgAttributes[NSAttachmentAttributeName] as! ImageAttachment let attributedString = implicitRepresentation(for: imgElement, inheriting: imgAttributes)!

PS: I know it can look counter-intuitive but it's basically a decision between silently ignoring an error situation vs letting it "explode" and become more noticeable (so we can fix it).

Normally I would try to avoid force unwrapping optionals, but I can see how "letting it explode" would be better in this case - to make the failure noticeable. Will make that change momentarily.

diegoreymendez

Working as described.

We'll wait for the second review before merging.

frosty

Looking good! I ran through your testing steps and everything worked as expected. I've just left a couple of quite small code style comments. I'll mark the PR as approved, as I don't think they should hold it up, but one or two of them might be nice to quickly address before merging.

frosty · 2017-10-30T18:04:09Z

Aztec/Classes/NSAttributedString/Conversions/AttributedStringParser.swift


+        if let linkText = attachment.linkURL?.absoluteString {
+            let hrefValue = Attribute.Value(withString: linkText)
+            let hrefAttribute = Attribute(name: "href", value: hrefValue)


I'm not very familiar with the Aztec codebase, but "href" here feels like something we could use a constant for, instead of using string literals.

You're right, and I see there's already a HTMLLinkAttribute enum with a constant for that. I'll change it.

frosty · 2017-10-30T18:07:53Z

Aztec/Classes/NSAttributedString/Conversions/AttributedStringSerializer.swift

        }
+
+        if let imgElement = linkedImageElement(for: element) {
+            let linkText = element.stringValueForAttribute(named: "href") ?? ""


See earlier comment about "href"

frosty · 2017-10-30T18:14:31Z

Example/Example/AttachmentDetailsViewController.swift


    var attachment: ImageAttachment?
-    var onUpdate: ((ImageAttachment.Alignment, ImageAttachment.Size, URL, String?) -> Void)?
+    var onUpdate: ((ImageAttachment.Alignment, ImageAttachment.Size, URL, URL?, String?) -> Void)?


This closure is getting a little unwieldy, particularly for the urls and strings. I wonder if we should either add a comment that clarifies what each parameter does, or simply add names to the parameters:

var onUpdate: ((_ alignment: ImageAttachment.Alignment, _ size: ImageAttachment.Size, _ imageURL: URL, _ linkURL: URL?, _ altText: String?) -> Void)?

The rest of the code seems to favor making things self-explanatory, rather than adding comments... so I think adding names to the parameters is a good idea. Making that change.

Unfortunately that doesn't make the point where you're using the closure any more readable since it's still:
onUpdate(alignment.toAttachmentAlignment(), size.toAttachmentSize(), url, linkURL, alt)

(because "Function types cannot have argument labels" ? meh)

Right! But I guess at least in the sample you game, the variables you’re passing in all have explanatory names :)

frosty · 2017-10-30T18:16:57Z

Example/Example/AttachmentDetailsViewController.swift

-        onUpdate(alignment.toAttachmentAlignment(), size.toAttachmentSize(), url, alt)
+        var linkURL: URL?
+        if let linkText = linkURLTextField.text {
+            linkURL = URL(string: linkText)


We could condense this like:

var linkURL = URL(string: linkURLTextField.text ?? "")

If you pass in an empty string, you get back nil.

diegoreymendez

Checked the latest changes and all is fine. Merging.

Nice work @jim-rhoades :)

jim-rhoades added 7 commits October 26, 2017 09:34

Initial work to handle links with image attachments

cb12701

Fixed issues with converting an image with a link from HTML to NSAttr…

8e4cb9f

…ibuted string. Fixed issue with converting a link to HTML.

Moved image/link related logic to make everything more futureproof

2eccaba

Cleaner more concise way to extract the image attachment

4181818

Small change to avoid code duplication

befd4a0

Replaced tab indents with spaces

3ee3d43

Example app now sets the linkURL of an image attachment when an image…

d3d1638

… is added

diegoreymendez requested review from diegoreymendez and frosty October 27, 2017 15:48

diegoreymendez suggested changes Oct 27, 2017

View reviewed changes

Fixed issue where content could be lost if there was no link text. Mo…

c4aad53

…dified code for clarity.

diegoreymendez suggested changes Oct 27, 2017

View reviewed changes

jim-rhoades added 2 commits October 27, 2017 15:33

Improvements to code readability/maintainability

e0be042

Minor code documentation fixes

015d52f

diegoreymendez reviewed Oct 30, 2017

View reviewed changes

Force unwrap certain optionals to make a failure noticeable

72815b9

diegoreymendez added the [Type] Enhancement label Oct 30, 2017

diegoreymendez added this to the Beta 14 (next stable) milestone Oct 30, 2017

diegoreymendez self-assigned this Oct 30, 2017

Moved handling of linked images into implicitRepresentation

035b2d5

diegoreymendez approved these changes Oct 30, 2017

View reviewed changes

frosty approved these changes Oct 30, 2017

View reviewed changes

jim-rhoades added 4 commits October 30, 2017 15:09

Uses a constant instead of 'href'. Minor code improvements.

a82d5a9

Fixed issue when converting linked image back from HTML

4808b93

Added unit test for linked image encoding/decoding

88c6124

Merged in changes from upstream.

02ddd21

diegoreymendez approved these changes Nov 2, 2017

View reviewed changes

diegoreymendez merged commit a1fa190 into wordpress-mobile:develop Nov 2, 2017

Issue/8045 aztec image links #786

Issue/8045 aztec image links #786

Uh oh!

Conversation

jim-rhoades commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diegoreymendez commented Oct 27, 2017

Uh oh!

jim-rhoades commented Oct 27, 2017

Uh oh!

diegoreymendez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jim-rhoades commented Oct 27, 2017

Uh oh!

diegoreymendez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegoreymendez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegoreymendez Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegoreymendez left a comment

Choose a reason for hiding this comment

Uh oh!

frosty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegoreymendez left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jim-rhoades commented Oct 27, 2017 •

edited

Loading

diegoreymendez Oct 30, 2017 •

edited

Loading