Skip to content

Ordering bug with deleted text in a list #25

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

Merged
merged 9 commits into from
May 21, 2013
Merged

Ordering bug with deleted text in a list #25

merged 9 commits into from
May 21, 2013

Conversation

jlward
Copy link
Contributor

@jlward jlward commented May 17, 2013

My first attempt at making a test for #23 resulted in finding an ordering bug. Lovely.

@ghost ghost assigned jlward May 17, 2013
@@ -342,6 +342,8 @@ def _get_children(el):
has_descendant_with_tag = True
if child.has_descendant_with_tag('drawing'):
has_descendant_with_tag = True
if child.has_descendant_with_tag('delText'):
has_descendant_with_tag = True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is not very clear. From the comment, it looks like what we're actually doing is only grabbing items if they at least have a child that might have some text. Is that correct? Can we move this logic to a separate method called maybe _get_children_with_content?

I would also recommend creating a constant like TAGS_CONTAINING_CONTENT that's a list of drawing, t, pict etc. instead of just a bunch of if statements with some tag names that seem to come from out of the blue.

Also, is there a tag that corresponds to delText like insText or something like that? Does that matter? I'm also a bit confused on why something with just deltText would break things here. If all it contained was deleted text, do we want to include its parent element at all?

<html><body>
<ol data-list-type="decimal">
<li>AAA<br/>
<span class='insert' author='' date=''>BBB</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the author and date stuff coming from? Those aren't valid HTML attributes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm surprised by the <br /> here. If I had change tracking on and typed BBB after the AAA, I would actually be trying to type AAABBB on one line. If I typed BBB I would be trying to type AAA BBB on the same line. Is there something I'm not understanding about the way Word records the track changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is existing behaviour and has nothing to do with this ticket. #30 will deal with that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yuck. "Existing behavior" is not a great justification for adding another bug, but I understand wanting to break it up. Seems odd to do for such a small ticket, though. Your call, so this would be done if you're cool with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not certain how complicated it will be, and it would be much simpler to handle it as a separate issue.

jlward added a commit that referenced this pull request May 21, 2013
Ordering bug with deleted text in a list
@jlward jlward merged commit 7f78ad4 into master May 21, 2013
jlward pushed a commit that referenced this pull request Mar 14, 2014
jlward pushed a commit that referenced this pull request Mar 14, 2014
jlward pushed a commit that referenced this pull request Mar 14, 2014
jlward pushed a commit that referenced this pull request Mar 14, 2014
jlward added a commit that referenced this pull request Mar 14, 2014
Ordering bug with deleted text in a list
jlward pushed a commit that referenced this pull request Mar 14, 2014
jlward pushed a commit that referenced this pull request Mar 14, 2014
jlward pushed a commit that referenced this pull request Mar 14, 2014
jlward pushed a commit that referenced this pull request Mar 14, 2014
jlward added a commit that referenced this pull request Mar 14, 2014
Ordering bug with deleted text in a list
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants