search

pleonex / atom-autocomplete-xml

Autcomplete XML package for Atom editor.
https://atom.io/packages/autocomplete-xml
MIT License
13 stars 8 forks source link

Fix issue #29 #31

Closed akukuq closed 6 years ago

akukuq commented 7 years ago

This seems to fix the issue for me, but I cannot guarantee that my changes did not introduce new problems (as I don't know coffee script). I would advise reviewing the changes I have made before intergrating. ;) Also, these are my first steps using Git and GitHub, so I'm pretty new to this ;)

pleonex commented 7 years ago

Thanks for the fix! I think that this approach is almost correct. Let me know if you need help with the changes.

akukuq commented 7 years ago

I have now addressed your comments by adding a new variable waitingStartCDATA and additional else if statements. I have also corrected a typo in the code (but perhaps it doesn't matter - I don't know if Coffee Script is case-sensitive)

akukuq commented 7 years ago

Well, actually, this is still not good enough. As it is, a sequence like <![CDATA[ --> ]]> will detect the comment end tag inside CDATA, resulting in all preceding XML structure being ignored.

I think I see how this could be fixed, but I am a bit confused by the reverse order of the regexp search. Is there a specific reason to do it this way?

akukuq commented 7 years ago

After the commit https://github.com/pleonex/atom-autocomplete-xml/pull/31/commits/43fd3095792cef8737578ff26ddcae52ac224bd9, all markup inside CDATA sections and comments will be ignored, including comments and CDATA markup, respectively. This will work only if the cursor is outside of these structures. When the cursor is inside CDATA or a comment, the contents of this CDATA or a comment are still parsed as XML. In some special scenarios, this might result in part of the file incorrectly not being parsed. For example:

<outer>
<!-- everything from this line... -->
  <inner>
    <![CDATA[
      ...all the way to this line will be treated as a comment... -->
      ...if the cursor is placed after the "end of comment" markup above,
      but still within this CDATA section. As a result, the XPath printed in
      the status bar will read "outer" instead of "outer/inner".
    ]]>
  </inner>
</outer>

This behavior, however, has more to do with the overall design of the reverse ordered regexp search than with my changes. It might qualify for a separate issue, though.

pleonex commented 6 years ago

Sorry I forgot about this issue :sweat_smile: Your changes looks good! I will merge this. We can improve how to handle xml inside CDATA in the future if someone really needs it.