Skip to content
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

add inline comment support #145

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

guoweis-work
Copy link
Contributor

@guoweis-work guoweis-work commented Dec 12, 2021

This is the output from cloud

    {
      "id": "2320499078",
      "type": "comment",
      "status": "current",
      "title": "Re: something",
      "macroRenderedOutput": {
      },
      "extensions": {
        "location": "inline",
        "inlineProperties": {
          "originalSelection": "“abc",
          "markerRef": "d9f34bb0-1cd3-462f-9541-1a93893f458a"
        },
        "_expandable": {
          "resolution": ""
        }
      },
      "_expandable": {
        "container": "/rest/api/content/2315944161",
        "metadata": "",
        "restrictions": "/rest/api/content/2320499078/restriction/byOperation",
        "history": "/rest/api/content/2320499078/history",
        "body": "",
        "version": "",
        "descendants": "/rest/api/content/2320499078/descendant",
        "space": "/rest/api/space/~951868684",
        "childTypes": "",
        "operations": "",
        "schedulePublishDate": "",
        "children": "/rest/api/content/2320499078/child",
        "ancestors": ""
      },
      "_links": {
        "webui": "/spaces/~951868684/pages/2315944161/something?focusedCommentId=2320499078#comment-2320499078",
        "self": "https://xxxxx.atlassian.net/wiki/rest/api/content/2320499078"
      }
}

@guoweis-work
Copy link
Contributor Author

guoweis-work commented Dec 12, 2021

@kovetskiy how did the ' become ‘? The text when it comes to MergeComments has become ‘ but the htnl escape string doesn't do that. So I missed couple escaping

@kovetskiy
Copy link
Owner

Hi, I've checked the draft version, the problem with ‘ comes from the fact that Confluence replaces ' with ’ on the server side when we push HTML via API.

@kovetskiy
Copy link
Owner

Also found a problem:

Original markdown:

aaa **bold** bbb

Then select the entire paragraph and comment, OriginalSelection of an inline comment structure will be aaa bold bbb so MergeComments() func will not be able to replace the content since in the original content the phrase bold was wrapped up in two asterisks.

Actually have no idea how to come up with string replacement in such scenarios which will be quite frequent I suppose.

@guoweis-work
Copy link
Contributor Author

guoweis-work commented Dec 12, 2021

good catch. I tried the case you mentioned. In MergeComments, on the input, I got aaa <strong>bold</strong> bbb, what if we just remove the <strong> and probably with some space handling, this might work?

@guoweis-work
Copy link
Contributor Author

i'll create some test cases for these.

@kovetskiy
Copy link
Owner

kovetskiy commented Dec 12, 2021

good catch. I tried the case you mentioned. In MergeComments, on the input, I got aaa \<strong\>bold\</strong\> bbb, what if we just remove the \<strong\> and probably with some space handling, this might work?

That's strange since I've got OriginalSelection to be aaa bold bbb without <b> or <strong> tags. I'm surprised that they don't pinpoint at a specific location like line/column or at least the number of byte where the comment starts.

I'm not sure if string replacement is really a good way to handle the issue.

@guoweis-work
Copy link
Contributor Author

guoweis-work commented Dec 12, 2021

the alternative I thought about was xml path, however, since everything starts with <p>, it wasn't unique enough... Not sure what else can be done other than string replacement here, but I'm open to any other approach. I'll keep going with this to see if the end result is satisfactory.

@guoweis-work
Copy link
Contributor Author

That's strange since I've got OriginalSelection to be aaa bold bbb without <b> or <strong> tags

i also got OriginalSelection as aaa bold bbb from the inlineComment. My mentioning of aaa <strong>bold</strong> bbb is the body when it comes to MergeComment

@guoweis-work
Copy link
Contributor Author

the other approach is to download the original body.storage, and compare with the current one to inject the marker, but that is much harder, as it's diffing 2 xml documents...

@guoweis-work
Copy link
Contributor Author

guoweis-work commented Dec 15, 2021

@kovetskiy i think about this more. yes, there is no way to fix this ONLY by string comparison. Fundamentally, there is no way we could solve this accurately as the user's edition is a lossy info to us. I think there should be 2 approaches together

  1. we could have some heuristics, based on the string content as well as the original doc structure to determine which comment should go where
  2. for really motivated users, i wonder if we should invent a syntax so they could specify the comment in the exact location of their doc? Yes, this will be complicated as they will need to fetch the refID, etc...

Thoughts?

@kovetskiy
Copy link
Owner

@guoweis-outreach I didn't really come up to any ideas that wouldn't require a lot of work.

Maybe we could just lock page for comments so people would need to comment the entire page in the comments section instead?

@guoweis-work
Copy link
Contributor Author

guoweis-work commented Jan 8, 2022

@kovetskiy i think this one is important. I used mark to sync, but whenever someone comments on the page, I get afraid to update the page with mark as I'm worry that I'll lose it. Turning off the ability to comment inline can work but it causes lot of inconvenience.

I wonder as the first step, does it make sense to create a way to sync the comment from confluence to the markdown? ie. If we could create a syntax to mark the inline comment in the markdown file, and safely preserve it, then at least people won't feel worry about losing valuable comments.

something like this

Some text with <ac:comment id="xxxxxxx">comment</ac:comment>

Thoughts?

@Skeeve
Copy link
Contributor

Skeeve commented Jun 10, 2022

I wonder as the first step, does it make sense to create a way to sync the comment from confluence to the markdown? ie. If we could create a syntax to mark the inline comment in the markdown file, and safely preserve it, then at least people won't feel worry about losing valuable comments.
[…]
Thoughts?

Tbh: I'm not a hard-core confluence user and never will be. So maybe I miss the point of inline comments. Maybe it would be best to not allow a push when comments are in the page mark wants to update.

Maybe the comments have to be integrated somehow into the markdown and removed from the page first? I'm thinking here about comments which point to mistakes in the page or missing stuff. But as said: Maybe I have too less a clue of how confluence is intended to be used.

@andymac4182
Copy link

I have started on this similar problem in my project. Might provide some inspiration for you when looking at how to find where to put comments. https://github.com/markdown-confluence/markdown-confluence/blob/main/packages/lib/src/AdfProcessing.ts#L285-L500

I am using https://en.wikipedia.org/wiki/Levenshtein_distance calculation on the text around the comment to find the "best" place to put it back. Need to add in the same for when you modify the comment text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants