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

Feature request: Add anchor option to insert_image #117

Closed
mojaie opened this Issue Apr 8, 2014 · 16 comments

Comments

2 participants
@mojaie
Contributor

mojaie commented Apr 8, 2014

Hello,

I'm working with spreadsheets with images inserted by worksheet.insert_image().
It is inconvenient for me that moving and sorting image cells doesn't work correctly because the default worksheet setting of the 'editAs' option in 'twoCellAnchor' element is set to 'oneCell' (objects move with cells but do not resize).
So I'm going to add anchor option to the insert_image method so that 'editAs' option is set to 'twoCell' (objects move and resize with cells).

Thank you for your effort for this excellent library.

@mojaie

This comment has been minimized.

Show comment
Hide comment
@mojaie

mojaie Apr 8, 2014

Contributor

like this
7fcaed4

Contributor

mojaie commented Apr 8, 2014

like this
7fcaed4

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Apr 8, 2014

Owner

Hi @mojaie,

That looks useful.

Follow the guidelines in CONTRIBUTING.md.

Try to copy as many of the existing image comparison test cases as possible. Call them something like test_image_xxx_onecell.py.

If you need help creating Excel files to test against let me know. Or let me know if you have any other questions.

John

Owner

jmcnamara commented Apr 8, 2014

Hi @mojaie,

That looks useful.

Follow the guidelines in CONTRIBUTING.md.

Try to copy as many of the existing image comparison test cases as possible. Call them something like test_image_xxx_onecell.py.

If you need help creating Excel files to test against let me know. Or let me know if you have any other questions.

John

@mojaie

This comment has been minimized.

Show comment
Hide comment
@mojaie

mojaie Apr 12, 2014

Contributor

Hi,

I made some changes and wrote test cases. But the test was failed with following error message.
I used Excel 2007 for Windows to make xlsx file. Do you have any idea to fix it?

10b934d

======================================================================
FAIL: test_create_file (xlsxwriter.test.comparison.test_image01_absolute.TestCompareXLSXFiles)
Test the creation of a simple XlsxWriter file with image(s).
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/mojaie/XlsxWriter/xlsxwriter/test/comparison/test_image01_absolute.py", line 55, in test_create_file
    self.assertEqual(got, exp)
AssertionError: Lists differ: ['[Content_Types].xml', '_rels... != ['[Content_Types].xml', '_rels...

First differing element 8:
xl/styles.xml
xl/printerSettings/printerSettings1.bin

Second list contains 1 additional elements.
First extra element 13:
xl/worksheets/sheet1.xml

  ['[Content_Types].xml',
   '_rels/.rels',
   'docProps/app.xml',
   'docProps/core.xml',
   'xl/_rels/workbook.xml.rels',
   'xl/drawings/_rels/drawing1.xml.rels',
   'xl/drawings/drawing1.xml',
   'xl/media/image1.png',
+  'xl/printerSettings/printerSettings1.bin',
   'xl/styles.xml',
   'xl/theme/theme1.xml',
   'xl/workbook.xml',
   'xl/worksheets/_rels/sheet1.xml.rels',
   'xl/worksheets/sheet1.xml']
Contributor

mojaie commented Apr 12, 2014

Hi,

I made some changes and wrote test cases. But the test was failed with following error message.
I used Excel 2007 for Windows to make xlsx file. Do you have any idea to fix it?

10b934d

======================================================================
FAIL: test_create_file (xlsxwriter.test.comparison.test_image01_absolute.TestCompareXLSXFiles)
Test the creation of a simple XlsxWriter file with image(s).
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/mojaie/XlsxWriter/xlsxwriter/test/comparison/test_image01_absolute.py", line 55, in test_create_file
    self.assertEqual(got, exp)
AssertionError: Lists differ: ['[Content_Types].xml', '_rels... != ['[Content_Types].xml', '_rels...

First differing element 8:
xl/styles.xml
xl/printerSettings/printerSettings1.bin

Second list contains 1 additional elements.
First extra element 13:
xl/worksheets/sheet1.xml

  ['[Content_Types].xml',
   '_rels/.rels',
   'docProps/app.xml',
   'docProps/core.xml',
   'xl/_rels/workbook.xml.rels',
   'xl/drawings/_rels/drawing1.xml.rels',
   'xl/drawings/drawing1.xml',
   'xl/media/image1.png',
+  'xl/printerSettings/printerSettings1.bin',
   'xl/styles.xml',
   'xl/theme/theme1.xml',
   'xl/workbook.xml',
   'xl/worksheets/_rels/sheet1.xml.rels',
   'xl/worksheets/sheet1.xml']
@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Apr 12, 2014

Owner

Hi,

The additional printerSettings1.bin file is included when you access any of the printer settings from within Excel. It is a binary file and specific to each printer so it isn't included by XlsxWriter. If possible you should avoid accessing any printer preview or other options when creating the test files. However, in some cases that isn't possible because they may be the features that you are testing.

The comparison test framework has an option to ignore files and/or parts of files during comparison.

Include this at the top of the failing test case:

        self.ignore_files = ['xl/printerSettings/printerSettings1.bin',
                             'xl/worksheets/_rels/sheet1.xml.rels']
        self.ignore_elements = {'[Content_Types].xml': ['<Default Extension="bin"'],
                                'xl/worksheets/sheet1.xml': ['<pageMargins', '<pageSetup']}

See for example test_fit_to_pages01.

Before submitting the Pull Request can you message me once more and I'll do a quick code review (although as it stands it looks good).

After that can you squash or rebase your changes into one commit (if you know how to do that, if not leave it as is).

John

Owner

jmcnamara commented Apr 12, 2014

Hi,

The additional printerSettings1.bin file is included when you access any of the printer settings from within Excel. It is a binary file and specific to each printer so it isn't included by XlsxWriter. If possible you should avoid accessing any printer preview or other options when creating the test files. However, in some cases that isn't possible because they may be the features that you are testing.

The comparison test framework has an option to ignore files and/or parts of files during comparison.

Include this at the top of the failing test case:

        self.ignore_files = ['xl/printerSettings/printerSettings1.bin',
                             'xl/worksheets/_rels/sheet1.xml.rels']
        self.ignore_elements = {'[Content_Types].xml': ['<Default Extension="bin"'],
                                'xl/worksheets/sheet1.xml': ['<pageMargins', '<pageSetup']}

See for example test_fit_to_pages01.

Before submitting the Pull Request can you message me once more and I'll do a quick code review (although as it stands it looks good).

After that can you squash or rebase your changes into one commit (if you know how to do that, if not leave it as is).

John

@mojaie

This comment has been minimized.

Show comment
Hide comment
@mojaie

mojaie Apr 12, 2014

Contributor

Printer setting files were removed but there are still some properties automatically added by Excel.

35d8b87

Contributor

mojaie commented Apr 12, 2014

Printer setting files were removed but there are still some properties automatically added by Excel.

35d8b87

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Apr 13, 2014

Owner

There seems to be a little bit too many ignore options in use in those examples.

The mechanism is only intended for metadata that changes (like dates) or certain values that can't be replicated exactly (like some floating point values).

I think the best thing to do if for me to generate the test Excel files. Is it okay to mail them to you at the email address on your GitHub page?

Owner

jmcnamara commented Apr 13, 2014

There seems to be a little bit too many ignore options in use in those examples.

The mechanism is only intended for metadata that changes (like dates) or certain values that can't be replicated exactly (like some floating point values).

I think the best thing to do if for me to generate the test Excel files. Is it okay to mail them to you at the email address on your GitHub page?

@mojaie

This comment has been minimized.

Show comment
Hide comment
@mojaie

mojaie Apr 14, 2014

Contributor

Yes. Thank you for your assistance.

Contributor

mojaie commented Apr 14, 2014

Yes. Thank you for your assistance.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Apr 14, 2014

Owner

Could you change the interface a little.

I'd prefer to call the new property positioning rather than anchor (you can keep the anchor name internally in the module).

Also can you use the following values:

The ``positioning`` parameter can be used to control the object positioning
of the image::

    worksheet.insert_image('B3', 'python.png', {'anchor': 1})

Where ``positioning`` has the following allowable values

1. Move and size with cells.
2. Move but don't size with cells (the default).
3. Don't move or size with cells.

Internally map 1, 2, 3 to the values you are currently using.

Owner

jmcnamara commented Apr 14, 2014

Could you change the interface a little.

I'd prefer to call the new property positioning rather than anchor (you can keep the anchor name internally in the module).

Also can you use the following values:

The ``positioning`` parameter can be used to control the object positioning
of the image::

    worksheet.insert_image('B3', 'python.png', {'anchor': 1})

Where ``positioning`` has the following allowable values

1. Move and size with cells.
2. Move but don't size with cells (the default).
3. Don't move or size with cells.

Internally map 1, 2, 3 to the values you are currently using.

@mojaie

This comment has been minimized.

Show comment
Hide comment
@mojaie

mojaie Apr 19, 2014

Contributor

Thank you for your assistance.
Adding test cases and remapping values are done.
https://github.com/mojaie/XlsxWriter/compare/image_anchor_option

Contributor

mojaie commented Apr 19, 2014

Thank you for your assistance.
Adding test cases and remapping values are done.
https://github.com/mojaie/XlsxWriter/compare/image_anchor_option

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Apr 22, 2014

Owner

I'll make some comments in-line.

Owner

jmcnamara commented Apr 22, 2014

I'll make some comments in-line.

@mojaie

This comment has been minimized.

Show comment
Hide comment
@mojaie

mojaie Apr 24, 2014

Contributor

I made some changes based on your comments.
I'm sorry for my mistakes in git commit. I will do squash before sending pull request.

Contributor

mojaie commented Apr 24, 2014

I made some changes based on your comments.
I'm sorry for my mistakes in git commit. I will do squash before sending pull request.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Apr 24, 2014

Owner

Also all the tests need to be passing before I can merge.

Are you getting the emails from Travis when the tests fail after a push?

Like this: https://travis-ci.org/mojaie/XlsxWriter/builds/23653170

Owner

jmcnamara commented Apr 24, 2014

Also all the tests need to be passing before I can merge.

Are you getting the emails from Travis when the tests fail after a push?

Like this: https://travis-ci.org/mojaie/XlsxWriter/builds/23653170

@mojaie

This comment has been minimized.

Show comment
Hide comment
@mojaie

mojaie Apr 24, 2014

Contributor

Yes, the latest commit 5cd7535 passed the test.
https://travis-ci.org/mojaie/XlsxWriter/builds/23653964
I lost some files in local repository by mistake, and tried to delete old test files on github manually.

Contributor

mojaie commented Apr 24, 2014

Yes, the latest commit 5cd7535 passed the test.
https://travis-ci.org/mojaie/XlsxWriter/builds/23653964
I lost some files in local repository by mistake, and tried to delete old test files on github manually.

@mojaie

This comment has been minimized.

Show comment
Hide comment
@mojaie

mojaie Apr 29, 2014

Contributor

I missed one of your comment and fixed it now.

Contributor

mojaie commented Apr 29, 2014

I missed one of your comment and fixed it now.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Apr 29, 2014

Owner

Looks good. Squash it into one commit and submit it as a Pull Request.

Owner

jmcnamara commented Apr 29, 2014

Looks good. Squash it into one commit and submit it as a Pull Request.

jmcnamara added a commit that referenced this issue Apr 30, 2014

Merge pull request #119 from mojaie/master
Add positioning option to insert_image #117

@mojaie mojaie closed this May 1, 2014

@mojaie

This comment has been minimized.

Show comment
Hide comment
@mojaie

mojaie May 1, 2014

Contributor

Thank you for all your help!

Contributor

mojaie commented May 1, 2014

Thank you for all your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment