Conversation
There was a problem hiding this comment.
Pull Request Overview
Introduces support for embedding custom UI (RibbonX) XML into generated workbooks.
- Adds Workbook.add_custom_ui() API and wires packaging to include customUI files and relationships.
- Adds documentation and an example-driven test validating ribbons together with a signed VBA project.
- Includes sample customUI XML fixtures used by the new test.
Reviewed Changes
Copilot reviewed 9 out of 13 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| xlsxwriter/workbook.py | Adds add_custom_ui() API and internal storage for custom UI entries. |
| xlsxwriter/packager.py | Copies customUI XML into the package and adds relationships in root .rels. |
| xlsxwriter/relationships.py | Allows MS package relationship schema to vary by “version”. |
| xlsxwriter/test/comparison/test_macro05.py | New comparison test that exercises custom UI and a signed VBA project. |
| xlsxwriter/test/comparison/xlsx_files/customUI-01.xml | Test fixture for pre-2010 RibbonX namespace. |
| xlsxwriter/test/comparison/xlsx_files/customUI14-01.xml | Test fixture for 2010+ RibbonX namespace. |
| dev/docs/source/working_with_customui_elements.rst | New documentation for adding custom RibbonX to workbooks. |
| dev/docs/source/index.rst | Adds the new docs page to the index. |
| dev/docs/source/contents.rst | Adds the new docs page to the TOC. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if not self.in_memory: | ||
| # In file mode we just copy the xml files. | ||
| os_filename = self._filename("customUI/%s" % xml_custom_ui_name) | ||
| copy(custom_ui, os_filename) |
There was a problem hiding this comment.
In file mode, the copied customUI XML is never added to self.filenames, so it won’t be written into the zip. Append the file, similar to other assets, e.g. self.filenames.append((os_filename, f'customUI/{xml_custom_ui_name}', False)).
| copy(custom_ui, os_filename) | |
| copy(custom_ui, os_filename) | |
| self.filenames.append( | |
| (os_filename, "customUI/%s" % xml_custom_ui_name, False) | |
| ) |
| # For in-memory mode we read the custom ui into a stream. | ||
| custom_ui_file = open(custom_ui, mode="r") | ||
| custom_ui_data = custom_ui_file.read() | ||
| os_filename = StringIO(custom_ui_data) | ||
| custom_ui_file.close() | ||
|
|
||
| self.filenames.append( | ||
| (os_filename, "customUI/%s" % xml_custom_ui_name, True) | ||
| ) |
There was a problem hiding this comment.
In-memory branch appends the XML stream with is_binary=True but uses a text StringIO, which will be treated as bytes later and can cause incorrect encoding. Either set is_binary=False for XML text streams or switch to BytesIO by reading the file in binary mode.
| if self.workbook.custom_properties: | ||
| rels._add_document_relationship("/custom-properties", "docProps/custom.xml") | ||
|
|
||
| for custom_ui, version in self.workbook.custom_uis: | ||
| xml_custom_ui_name = os.path.split(custom_ui)[1] | ||
| rels._add_ms_package_relationship( | ||
| "/ui/extensibility", "customUI/%s" % xml_custom_ui_name, version=version | ||
| ) |
There was a problem hiding this comment.
Once the relationships method is updated to accept only {2006, 2010}, validate or normalize version here before passing it through (e.g., map 2007→2010 for backward compatibility or raise a descriptive warning).
|
@zindy, are you still working on this topic? It seems like everything is done except for resolving the merge conflicts. |
|
Hello @tsillus I need to get this sorted, you're absolutely right. Can you give me your opinion on the
I was thinking of using the keyword I will look at all the changes proposed over the week-end. |
|
I would not go for a boolean value here, because once you have 3 options, you would have to change the API, which would be a breaking change, which then would require some deprecation period. More work for the person maintaining the code and more work for the people using the code (because they have to make changes to their own code as well). If you want to limit the options in an extensible way, you could think of an Enum, or have some constants defined that people can use instead of magic numbers, but I think writing documentation explaining the parameter should be enough. |
I think that the version should be parsed from the XML so that the user doesn't need to know anything about it. That was a change that I was going to make after merge but maybe it is better to do it now if you are going to change it anyway. |
I really like the simplicity of this! |
Add custom UI (ribbons) to XlsxWriter Polish work from previous PRs and integrate into main. Co-authored-by: Andreas Rueckle <[email protected]> Co-authored-by: Egor Zindy <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
The PR code is now up to date with main. I implemented pulling the excel version straight from the XML file namespace as @jmcnamara suggested. I also resolved all but three of copilot's suggested changes as I don't understand what they imply. |
|
Thanks @zindy . I will probably merge this up over the weekend. I may change things around a bit before release and I will need to do an implementation in Rust as well. Thanks for the effort here. You can ignore the failing lint tests. I'll fix everything up after merge. |
This PR introduces custom UI ribbons to XlsxWriter, allowing users to define and add custom ribbon elements to Excel workbooks as discussed in #1086.
Pull request details:
add_custom_ui()method to XlsxWriter.Author and Co-authors:
This feature builds on previous PRs, consolidating ribbon functionality into a single, well-tested commit.
Testing:
Tested with
pytest xlsxwriter\test\comparison\test_macro05.py:All existing tests still pass: