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

[reland] cleanup column width and font size setting #350

Merged
merged 2 commits into from
Oct 30, 2022

Conversation

JanMarvin
Copy link
Owner

@JanMarvin JanMarvin commented Sep 30, 2022

I have updated to font size settings to default to column width of 8.43 and a font size of 11 and to use the windows size setting. On Mac the column width will be slightly off, but otherwise we'd have to skip a couple of tests.

This adjusts a few functions to use default column width of 10 and default font size 12. This is what MS365 uses on Mac. It is changed, because otherwise column widths were slightly off. 9. something instead of 10. Adjusted to this new values, all calculations are correct for Calibri. Yet, I would not bet that our calculations are valid for all versions. Previously we were using 8.43 and font size 11 as default. Which might be correct for the MS365 webbrowser edition or certain Windows low dpi versions.

Maybe you can check the following on Windows @jmbarbone ? On Mac the column width is exactly 8.5 ... 17.5.

  wb <- wb_workbook()$
    add_worksheet()$
    set_col_widths(cols = 1:10, width = (8:17) + .5)$
    add_data(x = rbind(8:17), colNames = FALSE)$
    open()

Orignal PR was #341 which was accidentally merged and reverted

Copy link
Collaborator

@jmbarbone jmbarbone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JanMarvin, I'm seeing an extra 0.25 on Windows. Column widths are appearing as 8.75, 9.75, etc.

@JanMarvin
Copy link
Owner Author

Thanks @jmbarbone , I have modified their formula here:

widths <- trunc((as.numeric(col_width) * mdw + 5 + 2) / mdw * 256) / 256

The +2 is my fix for Mac. Could you see if it works without the +2? I'm not sure if the width relies on the OS or DPI of the screen somehow. I will check with the web browser version of MS365.

@jmbarbone
Copy link
Collaborator

@JanMarvin , removing the +2 seems to do the trick. All the columns are at the appropriate x.5 widths.

@JanMarvin
Copy link
Owner Author

So their formula actually works on Windows. Maybe we should stick with that.
My initial hope was that the value in the XML file would match the value Excel shows. Though that is not the case and the value we are currently setting was off when applied on Mac. This PR is an improvement, though the default width seems to be varying across Excel and OS versions. MS365 on Mac has a default width of 10. My work Excel 2013 has 10.71 and MS365 in the browser was 8.43. Do you have any preference? (The calculated value is depending on the default width as well.)
We could go with the 10 and this PR and formula for Windows. This way if someone sets a column width the result would match on one OS.

@JanMarvin JanMarvin mentioned this pull request Oct 13, 2022
5 tasks
@JanMarvin JanMarvin force-pushed the update_col_widths branch 3 times, most recently from 52260b3 to d227345 Compare October 16, 2022 21:54
@JanMarvin JanMarvin changed the title [reland] change default col size to 10 [reland] cleanup font size setting Oct 16, 2022
@JanMarvin JanMarvin changed the title [reland] cleanup font size setting [reland] cleanup column width and font size setting Oct 16, 2022
@JanMarvin
Copy link
Owner Author

@jmbarbone If you find the time, could you please give this another feature test and run the check again? Just want to make sure that we set the column width correctly for Windows Excel. Unfortunately the width values are different on Mac and I haven't found a way to please both Excels. Might be a general issue: link to ms forum

This changes the column width setting to calculate the values, whereas previously we simply set a value which was not identical to whatever Excel was using. This aligns a few functions to return width 8.43 and Calibri 11 font. I'd like to include this with 0.3.1 due to the more expected column widths. Prior to this PR you would set the width to 10 and would end up with something different, now you end up with column width 10 on Windows and 9.8 or so on Mac.

@jmbarbone
Copy link
Collaborator

@JanMarvin , with the current build the column widths seem fine when set to integers or integer-like values. Decimals seem to have some random offset:

devtool::load_all()
wb <- wb_workbook()
wb$add_worksheet()
wb$add_data(x = rbind(8:17), colNames = FALSE)
wb$set_col_widths(cols = 1:10, width = (8:17) + seq(0.1, 1, .1))
wb$open()
width offset actual difference
8 0.1 8.14 0.04
9 0.2 9.14 -0.06
10 0.3 10.29 -0.01
11 0.4 11.43 0.03
12 0.5 12.43 -0.07
13 0.6 13.57 -0.03
14 0.7 14.71 0.01
15 0.8 15.86 0.06
16 0.9 16.86 -0.04
17 1.0 18.00 0.00

@JanMarvin
Copy link
Owner Author

Thanks. Might be related to the use of round. With .2 it is -0.06 and with .8 it's 0,06. Do you feel like this is reason enough to hold the pull request back? I'd say we can merge it the way it is and improve from there on (if we ever have to). After all it is already much better than the previous implementation.

@jmbarbone
Copy link
Collaborator

@JanMarvin , if this is an improvement from the current version, then it's probably good to include it

Might just need a disclaimer:

column width setting has been improved but may not be exact on Windows

@JanMarvin JanMarvin merged commit 0a25378 into main Oct 30, 2022
@JanMarvin JanMarvin deleted the update_col_widths branch October 30, 2022 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants