Skip to content
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

add_style overwrites all previous styles #14

Closed
westonganger opened this issue Jul 3, 2016 · 18 comments

Comments

Projects
None yet
2 participants
@westonganger
Copy link
Contributor

commented Jul 3, 2016

I had already defined row styles through plain axlsx but I was applying a column style using add_style and noticed it was overwriting all previous styles.

Is there any way we can get it to only add on to the other styles?

@sakovias

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2016

Originally, I wasn't going to support this. Let me re-evaluate and see what we can do.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2016

Correct me if i'm wrong since your the author but I think I found the code that needs to be changed. Its the set_style_index method of Workbook.

If this is the offending code it looks like it should be achievable to accomplish this task.

@sakovias

This comment has been minimized.

Copy link
Collaborator

commented Jul 6, 2016

Haven't looked into this, but I think it's not going to be a quick fix. If that could be done without patching axlsx, that would be perfect. Feel free to investigate!

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2016

Another problem here is when using add_border as it removes all styles that were applied to it previously by axlsx.

I investigated that method which didn't work out. However I have delved deep into axlsx lately and I think I may have found a way to do this. I would need to patch the add_style method of Axlsx::Styles though if that is acceptable.

@sakovias

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2016

Sounds good. Is this change required for Spreadsheet Architect or it's more of nice-to-have?

I'm a bit hesitant about a change like this since all styles can be added with add_style and there's a big risk of introducing some bugs, while I'm not an active user of this gem. I think this change will need a lot of tests before the release. I'll be away at the end of July, but will be happy to review / contribute in August.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2016

Well for any proper border support and better behaviour of range styles this will be required.

August is fine though I will try to make a PR before then. Basically I want to add another optional argument to this method which accepts the cells xf object which we can extract existing styles form and merge into the new one being created by this method. So unless that argument is present the functionality will remain the same.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2016

I was able to make this happen over at my fork https://github.com/westonganger/axlsx_styler.

It does monkey patch two method in Axlsx::Styles however it only adds an optional argument to each to provide the improved functionality so that the original method behaviour is still maintained. You can see these additional patches in lib/axlsx_styler.rb. I think the monkey patches should be moved to seperate file(s) as they are starting to pollute this main file.

Also I had to make changes to the Axlsx::Workbook#set_style_index method in lib/axlsx_styler/axlsx_workbook.rb to make this work because the original implementation had no regard for styles set by plain axlsx.

I think this is a huge improvement for this library. Let me know your thoughts. I will add tests and make a PR if you are happy with it.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2016

Have you had a chance to look at this yet?

@sakovias

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2016

Looks good. Thanks for your work!

There are a couple of things I notice. Most of the tests are failing, producing more styles than expected in the style index. I will need to look more thoroughly to understand the reasons. Obviously, for changes like this it's expected to get broken tests 😄

Correct me if I'm wrong, but my second observation is that styles are not getting merged properly when I mix old syntax and that brought over from axlsx. Here's an example.

require 'axlsx_styler'

axlsx = Axlsx::Package.new
workbook = axlsx.workbook

bold = workbook.styles.add_style b: true

workbook.add_worksheet do |sheet|
  sheet.add_row ['A1', 'B1'], style: [nil, bold]
  sheet.add_row ['A2', 'B2']
  sheet.add_style 'B1:B2', bg_color: 'ff0000'
end

axlsx.serialize 'test.xlsx'

The bold style in cell B1 gets overridden by red background color.

axlsx-merging-problem

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2016

Yes there will be increased styles because in order to "merge" it instead creates a new style object. I changed the implementation of Axlsx::Workbook#set_style_index. We should probably ensure we are doing all we can to use less style objects. Now that we are monkey patching add_styles we can probably add logic for style_index and raw_style in this method.

I just made a commit that should fix the styles merging.

@sakovias

This comment has been minimized.

Copy link
Collaborator

commented Aug 10, 2016

I pulled the latest, and it looks like the same manual test above is failing. The cell B1 is not getting bold face.

I also tried another test, where I apply border that covers one of cells with bold face

require 'axlsx_styler'

axlsx = Axlsx::Package.new
workbook = axlsx.workbook

bold = workbook.styles.add_style b: true

workbook.add_worksheet do |sheet|
  sheet.add_row ['A1', 'B1'], style: [nil, bold]
  sheet.add_row ['A2', 'B2']
  sheet.add_border 'A1:B2'
end

axlsx.serialize 'bold_vs_border.xlsx'

No luck unfortunately:

bold_vs_border

Ideally these tests should be automated.

As far as the tests go, the number of styles in the index should not change. I'm noticing that on your fork you've disabled style indexing (that guarantees uniqueness of styles)

      def set_style_index(cell)
        self.style_index ||= {}

        index_item = style_index.select { |_, v| v == cell.raw_style }.first
        if index_item
          cell.style = index_item.first # this one
        else
        # ...

If a spreadsheet has too many styles, Excel (at least older versions) can crash or perform very poorly. That's the issue I was facing in earlier iterations of the gem where a style was created for almost every cell.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2016

Thanks for the comments and help testing. I will have to revisit this on to get it merging correctly, try to utilize the style_index caching as best possible, and will add some tests to automate this.

@sakovias

This comment has been minimized.

Copy link
Collaborator

commented Aug 10, 2016

Sounds good! I may spend some time over the weekend to understand how it works. One of the options is to put it as a PR (so we can make comments) and iterate on it.

@sakovias

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2016

Just checked that when we call

workbook.styles.add_style <style hash>

the original style hash is broken down into a bunch of objects: border, numeric format, font ID, etc.

Here's an example

bold = wb.styles.add_style b: true
red  = wb.styles.add_style fg_color: 'ff0000'
# => 3
# => 4

wb.styles.cellXfs[bold]
wb.styles.cellXfs[red]
# <Axlsx::Xf:0x000000035f01b0 @borderId=0, @numFmtId=0, @fontId=1, @fillId=0, @applyNumberFormat=false, @applyFill=false, @applyFont=true, @applyBorder=false, @applyAlignment=false, @applyProtection=false>
# <Axlsx::Xf:0x000000035ee1f8 @borderId=0, @numFmtId=0, @fontId=2, @fillId=0, @applyNumberFormat=false, @applyFill=false, @applyFont=true, @applyBorder=false, @applyAlignment=false, @applyProtection=false>

wb.styles.fonts.to_a
# [
#     [0] #<Axlsx::Font:0x000000035f5818 @name="Arial", @sz=11, @family=1>,
#     [1] #<Axlsx::Font:0x000000035f09d0 @name="Arial", @sz=11, @family=1, @b=true>,
#     [2] #<Axlsx::Font:0x000000035eebd0 @name="Arial", @sz=11, @family=1, @color=#<Axlsx::Color:0x000000035ee810 @rgb="FFFF0000">>
# ]

If I understand it correctly, you were trying to convert the styles given by Ruby hashes into Axlsx::Xf instances, which is one approach that requires manipulating this long list of properties @borderId, @numFmtId, @fontId, @fillId... Another approach would be to convert Axlsx::Xf back into Ruby hash, to make merging styles similar to merging hashes. I think patching axlsx should not be required in either of approaches, as it provides a bunch of getter methods.

Of course, this is doable, but I'm not sure if what you're trying to achieve in SpreadsheetArchitect is not entirely possible with what axlsx_styler or axlsx already provide. A concrete example to demonstrate your pain points is appreciated :)

@sakovias

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2016

Actually, there's another solution I can think of.

  1. Find out which arguments are passed to Styles#add_style, i.e. figure out what the original style Hash is before compilation (e.g. { b: true })
  2. Find which styles cells receive during the sheet.add_row operation
  3. Merge the obtained styles using AxlsxStyler::Axlsx::Cell#add_to_raw_style, which should be a small modification to axlsx_styler
@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2016

Your last solution helped me come up with the proper implementation. I just created a PR for this. Interested to get your feedback.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2016

Now that this is implemented can we get a new release?

@sakovias

This comment has been minimized.

Copy link
Collaborator

commented Sep 18, 2016

Thanks for the reminder @westonganger! I just pushed the gem.

I'm hoping CI pipelines won't get busted 😜

@sakovias sakovias closed this Sep 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.