tbl: add row height getter and setter

[wich]

Added functionality to get and set the height of table rows

[wich]

Added a behave feature to test row height

[wich]

@scanny Any chance you can have a quick look at this?

[scanny]

@wich - Looks pretty good, just needs unit tests:

In tests.test_table.Describe_Row:

it_knows_its_height():
it_can_change_its_height():

[wich]

@scanny just added the mentioned unit tests and both make accept and make test pass.

[wich]

Hmmn, weird travis CI fails on some unintended changes, but make test went fine...
I'll commit amend and force push

[wich]

@scanny anything else I need to do?

[wich]

@scanny Can this be integrated?

[scanny]

Hi @wich, apologies, just getting back to this now.

Yes, this looks like it has all the bits it needs, I'll work on the merge here, possibly this evening :)

[scanny]

@wich I've taken a closer look and started a feature branch for this here:
https://github.com/python-openxml/python-docx/commits/feature/rowheight

I started an enhancement proposal page here and you'll find it as the last commit on that branch. In future you'll need to provide that yourself, preferably before starting work so we can agree on the API/calling protocol.

It looks like there's a property missing, named .HeightRule in the the MS API, along with the enumeration WdRowHeightRule (mapping to .height_rule and WD_ROW_HEIGHT_RULE respectively in python-docx). These two properties share the same element, so I don't see how we can implement one without the other. The good news is that it's a modest addition.

It's unclear to me what the behavior is if .height_rule is set without assigning anything to .height. I'm expecting it doesn't cause a load error, but that should be verified with an XML edit. opc-diag is helpful for that, basically create a small document, unzip it with opc-diag, edit the XML to set the hRule attribute but not the val attribute, repackage it with opc-diag, and then try loading it. There are other ways of accomplishing that but this is what I usually do.

If, while you were at it, you could work out what the default value of w:trHeight{@hRule} is that would be handy. I would guess that Word behaves the same when it's blank and when it's 'auto', but I've been surprised on that sort of thing before :) I'm not sure it would change the behavior we implement, but it would reduce the likelihood of surprises once it was all merged in.

If you can add an acceptance test for .height_rule I think that's the best first step. That may require adding the WD_ROW_HEIGHT_RULE enumeration. Then I can get the acceptance tests merged in and we can get on to the first step in the implementation. I expect I'll have some feedback on that but probably best to wait until we get to that step.

Let me know what you think after you've had a chance to look it over.

[wich]

@scanny Thanks. Should the pull request target be changed from master to that feature branch?

I also have some things ready for vertical alignment in tables and for cell fill. The latter is also a bit more complex as cells can support a lot more in their shading than only a fill. But probably it will be better to do it bit by bit, start with what people actually need.

That's also why I didn't really bother with the heightRule. I would guess that most people just want exact anyway if they explicitly set the height, so that's what it does now. I figured we can easily add the complete logic for heightRule later. But i guess it's simple enough to get into now.

I looked at the microsoft documentation, it has the following to say on the matter of missing attributes:

For the height rule:
If this attribute is omitted, then its value shall be assumed to be auto.

For the height value:
If this attribute is omitted, then its value shall be assumed to be 0.

So I guess that's simple enough. I will work on adding the height rule enumeration, the acceptance tests and adding the property to the CT_TblHeight class.

What is your intent by the way? do you strictly want to adhere to the microsoft API or not? In the microsoft API the properties are called heightType and val, instead of heightRule and height. Personally I would expect setting a "height" property to have an immediate effect and not needing to set some (to most) obscure heightRule property. My feeling is that it would be good to have some kind of convenience method/property that will immediately set a height as expected by a user. What do you think?

[scanny]

I think rebasing onto the feature branch is a good idea. That way you can incorporate changes to a merged commit when the process goes to multiple steps (as it always seems to do :).

The vertical alignment and cell fill will be welcome. You can start a separate pull request for each of those, or might even make sense to wait until we finish this up so you've had a cycle through the process and know what to expect.

I generally model the API after the Microsoft Word VBA API, but not always:
https://msdn.microsoft.com/en-us/library/office/ff837519.aspx

Note this is distinct from the Open XML SDK you linked to, which manipulates the XML directly.

In general the API names are different than the XML element names. There is a place for each as python-docx has API methods using the API names and internal oxml objects and methods that use the XML names.

So the API property would be .height_rule while the attribute names of CT_TblHeight would be heightType and val as you mention.

A convenience method can occasionally be a good idea. If you want to make a specific proposal we can think it through.

[wich]

@scanny can you please have a look at the changes made for height_rule?

[scanny]

@wich I'm on holidays just at the moment so away from my "merging" machine. I'll get started on this once I get back on Thursday :)

[scanny]

Hi @wich, I've pushed a refactored .feature file here: https://github.com/python-openxml/python-docx/commits/feature/rowheight.

Next step will be for you to get the steps etc. working with it. Here is a summary of the changes I made:

• acceptance tests for Row.height are separate from those for Row.height_rule. This is the main thing. This makes them more compact and independent, the general principle being that tests should test one thing only. The practical aspect is you don't have to test all the combinations and permutations, so they're also shorter. And of course they're easier to read and reason about.
• The corpus of acceptance tests embody a (my) learning curve in which I've refined the style to a compact and direct wording. So unfortunately not all the acceptance tests are as good an example as others :) This gives an example of the current style. I haven't gone back to update the existing ones, maybe one day :)

If you can adjust the steps and the .pptx test file to work with the new feature file we'll get this committed and move on from there.

[madphysicist]

There are a number of great features in work on this project, and this is definitely one of them. One of the things I especially like about PRs here is that I can almost always find a temporary workaround to use until the feature gets incorporated.

In that spirit, I wrote the following function to temporarily hack together a row height setting based on the code in this PR:

def setRowHeight(row, height):
    """
    Sets the height of a table row.

    `Row` is a `docx.table._Row` object. `height` is the desired height in EMU.
    """
    trHeight = OxmlElement('w:trHeight')
    trHeight.set(qn('w:val'), str(height.twips))

    trPr = OxmlElement('w:trPr')
    trPr.append(trHeight)

    row._tr.append(trPr)

[wich]

@scanny, sorry for the delay in response. Other priorities have kept me occupied the past months.

The source branch of the pull request is now updated according to your changes. Please let me know if the current state is acceptable for integration.

[scanny]

Glad to see you again @wich :) No worries, I'll have a look this evening and see where we stand :)

[madphysicist]

I think the word certain is superfluous here.

[madphysicist]

I hope this gets merged in. I really look forward to using it.

[scanny]

@wich Okay, this is all merged on feature/rowheight here: https://github.com/python-openxml/python-docx/commits/feature/rowheight

I left some comments for future reference, but I just made those tweaks as I was working through the commits.

I suppose I'll go ahead and push a new incremental release as there's no other active development nearing completion. Was there something else you wanted to work on? I can hold off if you have something else coming, just let me know :)

[wich]

@scanny awesome, great news. I can't fully commit to it, but I would like to address two more table related features (vertical alignment and cell fill color) in the next couple of weeks. I'll leave it up to you to decide whether you want to wait and see how progress on those go.

[scanny]

Sounds great, we'll wrap them all up into the next release then :)

[scanny]

The feature/rowheight branch has been merged with develop and will go out with the next release.