GradientFill stops with specified position

#163 Merged at 8140add
Repository
jnothman
Branch
2.5
Repository
openpyxl
Branch
2.5
Author
  1. jnothman
Reviewers
Description

Fixes #771.

This is the start of an implementation of positioned stops for GradientFill. GradientFill.stop may now be specified as a list of:

  • (color, position) pairs
  • colors
  • Stop instances

However, I can't seem to get all of the Serializable magic working; the default to_tree doesn't seem to be giving me what I expect. (Getting <stop val="&lt;openpyxl.styles.fills.Stop object&gt; ....) Please help, @CharlieC!

(PS: I'm failing to correctly use hg branches, with apologies)

Comments (52)

  1. CharlieC

    What problems are you having with hg branches? Looks okay at the moment. Important is not to be working on the default branch. I think this is where openpyxl differs from most projects but we have a reason for not making a default checkout bleeding edge.

  2. jnothman author

    Could you please comment on why the serialization is not working? TestGradientFill.test_serialize fails with

    E         Got:
    E           <fill>
    E             <gradientFill bottom="4" degree="90" left="1" right="2" top="3" type="linear">
    E               <stop val="&lt;openpyxl.styles.fills.Stop object&gt;
    E         Parameters:
    E         position=0.0, color=&lt;openpyxl.styles.colors.Color object&gt;
    E         Parameters:
    
  3. jnothman author

    Despite that, I've fixed up documentation (not checked rendering yet) and used a different method for filling unspecified positions.

  4. CharlieC

    The tests should be based around real world examples of the two different types. This will make documentation and a convenience API easier.

  5. jnothman author

    I have no idea what real world examples for the 'path' type are except perhaps to create some kind of border...?

    The case I was considering for 'linear' was using this as a bar chart, i.e. [BLUE, (RED, .7), (WHITE, .7), WHITE] for a value of 70%. I don't understand how this helps as a unit test, though, rather an end-to-end test.

    I admit I do not know other contexts where a multi-part gradient is useful in excel.

    1. CharlieC

      Borders can be created with the edge attributes: the specification contains an example. Your suggested example is fine but I would suggest that the code initially be fill.stop = [Stop(BLUE, 0), Stop(RED, ,7), Stop(WHITE, .7), Stop(WHITE, 1)] I'm guessing at the positions but this will help us get the API (the way we want client code to work) right.

  6. jnothman author

    Fine.

    Although I've just tested my Excel for Mac installation and found that two identical positions does not work. So you need to do (0, .7, .7001, 1). My Excel also allows stops to be specified in any order (which is one reason why they must be distinct), and it will do the sort.

    1. CharlieC

      From the "implementers notes":

      [MS-OI29500: Microsoft Office Implementation Information for ISO/IEC-29500 Standard Compliance]
      
      18.8.38. stop (Gradient Stop)
      
      (a)
       The standard specifies @position as defined by the XML Schema double datatype.
      
      
       Office requires @position to be a double between 0 and 1 inclusive.
      

      It's can be difficult to enforce constraints as this level so I wouldn't worry about them too much. It might be easier to have some kind of "gradient factory" that client code can use to get values right. But we'll cross that bridge when we come to it.

  7. jnothman author

    Charlie, i understand how to make this work and provide the interface you consider appropriate, i think. I have not understood how to make it backwards compatible, automatically enumerating positions where none are supplied, if not in the descriptor. Do you not care for such things?

    1. CharlieC

      As I've repeatedly noted, backwards compatibility, can and should be done after the code implements the spec correctly and the documentation is improved. If necessary I can do this myself. But I do also think that the desire to support (BLACK, (WHITE, 0.7), RED, BLUE (0.4)) is probably too ambitious and the Stop definition must be robust enough to reject invalid XML. Client code can either provide proper Stops or a list of colours. This makes everything a lot clearer. The code that calculates stop positions is a utility that client code can call with potentially incomplete definitions though I'm not sure if it's actually necessary. But then I don't understand the position stuff very well. I will also submit a bug report to the ECMA WG regarding the constraints

      1. jnothman author

        Okay. I'm not sure when I'll get to this, but I intend to (or someone else can take up the baton):

        • provide you with a sample rendering of a few examples of linear and path gradients with 1, 2, 3 stops, following the spec, and a few examples that fail to render in my Office instance, upon which tests will be based
        • make the GradientFill.stop setter accept either a list of 1 or 2 colours, or a list of Stops of arbitrary length
        • we may later extend GradientFill.stop's setter to support (colour, position) pairs (as this is how I would prefer to describe a gradient), or use a convenience openpyxl.styles.fills.make_gradient_stops() helper
        • we may later extend handling of multiple unpositioned colours to support more than two colours with equal lengths between, e.g. for five colours (0, .25, .5, .75, 1) (but you will have to allow tests to account for numerical imprecision)
        • we could outlaw duplicate positions either in the GradientFill.stop setter or in its to_tree or we could not handle this case and just let the user deal with it
  8. jnothman author

    OpenXML gradient test bed: https://drive.google.com/file/d/0B11T_r1dvhbfSkRwQVBWY1VIQVU/view?usp=sharing

    There I manually entered some variants on gradientFill. I got read errors upon opening and still haven't managed to diagnose what is producing the error. But the only cell that didn't render is that with stop position > 1.

    Screen Shot 2017-04-14 at 12.33.15 pm.png

    The path gradients have type="path" left="0.2" right="0.8" top="0.2" bottom="0.8" as at https://msdn.microsoft.com/en-us/library/documentformat.openxml.spreadsheet.gradientfill(v=office.14).aspx, but it seems that in my Excel (15.33 for Mac), the left, right, top, bottom attributes are ignored: the gradient is equal in all directions (proportional to height and width).

    1. CharlieC

      Thanks for the example. Difficult to tell why Excel doesn't like the file: the validator flags up all the attributes as being wrong but I guess it's really the ones where position > 1 that cause problems. The implementer notes mention the constraint but I suspect that there should also be a remark noting that stops and positional attributes are mutually exclusive.

      I will create bug report for the OOXML to get the spec updated.

      1. jnothman author

        I don't know what you mean about stops and positional attributes being mutually exclusive. All stops have a position attribute

              1. jnothman author

                by default in path, the gradients end in the centre. if left, etc are specified, they are compressed into the specified proportion in each direction, and maintain the final colour. I think that's what you're seeing in windows

                1. CharlieC

                  Okay. I guess it doesn't really matter too much as these fills, as we can see, are not universally supported. But useful for the documentation. The important thing for the library is that they are correctly preserved in existing files.

  9. jnothman author

    Do you think it is fair for me to initially support either: a list of colours, which will be positioned with equal spacing on the line from 0 to 1? a list of Stops with their positions specified?

        1. jnothman author

          And if you want more in this PR (i.e. specification by two colours with implied position), how do you suggest it be implemented? Perhaps inheriting from Sequence and overriding __set__?

          1. CharlieC

            Looks pretty good. The logic for dealing with a list of colours is more or less there in the existing _serialise_stop() function. This can be modified to accept a sequence of Stops or Colours and calculate the position as necessary. It could then be used by the __set__ method of the descriptor. fill_unspecified_stops() looks superfluous now, right?

              1. CharlieC

                Can you create a function that does what you want to be able to do? ie. pass in a mix of colours and stops and have it create a list of valid Stops? Let me worry about the descriptor side.

              1. CharlieC

                It looks okay but difficult to test. As most of the code touches neither the descriptor (self) nor the GradientFill (obj), it would be better as a (private?) function. This would make testing a lot easier.

                1. jnothman author

                  Could you please describe what is lacking in the current tests that would be much better served by a separate function and would not still need testing via a descriptor?

                  1. CharlieC

                    It can be a separate function that gets called by the descriptor: testing just the function without needing a Stop or a GradientFill is easier. The ValueError at least is not tested.

                      1. CharlieC

                        Thanks, I missed that. Still I'd prefer a standalone function that calculates the stops and just gets called by the descriptor. And I thought you wanted to be able to mix stops and colours in the API?

                        1. jnothman author

                          I think this subset of the previously proposed functionality is appropriate. It's much better on the explicit better than implicit front. I can't make more changes right now

    1. CharlieC

      I've just added a test for the total position: I thought this wasn't allowed to exceed 1?

          total = sum(stop.position for stop in values)
          if total > 1:
              raise ValueError('Total position may not exceed 1')
      
      1. jnothman author

        no, sum of positions may be any positive number in theory. I've got a test case there with positions 0,.5,1 so clearly your test contradicts that.

  10. jnothman author

    The only real constraint on position is that it not be below 0 or above 1. In practice, duplicate positions could be outlawed because they seem to be handled arbitrarily.