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

Replace textsize with fontsize everywhere #2387

Merged
merged 4 commits into from
Nov 23, 2022

Conversation

jkrumbiegel
Copy link
Member

@jkrumbiegel jkrumbiegel commented Oct 31, 2022

Description

We had both textsize and fontsize in use for quite a while, which was always confusing.
With this PR, textsize is removed everywhere.

I've added error messages in two central places, plot recipes and Block creation functions, which should catch all uses of the keyword and prompt users to search and replace. With that, migration should not take more than a few seconds so I believe this breaking change is worth it in terms of API clarity.

We can discuss if the error policy is too drastic because it will also apply to user-defined recipes and Blocks. However, I'd argue that it's an improvement for the ecosystem if all uses of textsize are purged, unless we want it to stick around as a ghost for years.

The PR also makes text inherit fontsize from the main theme setting by default, which it arguably should have done for a while.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Added an entry in NEWS.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.

@MakieBot
Copy link
Collaborator

MakieBot commented Oct 31, 2022

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running:

using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(display(fig))
using create display create display
GLMakie 31.64s (31.46, 31.92) 0.19+- 19.04s (18.68, 19.52) 0.28+- 17.87s (17.45, 18.33) 0.31+- 14.46ms (14.13, 14.72) 0.19+- 56.57ms (55.81, 58.27) 0.82+-
master 31.39s (31.21, 31.64) 0.16+- 19.08s (18.57, 19.57) 0.37+- 17.84s (17.64, 18.20) 0.20+- 14.44ms (14.22, 14.83) 0.24+- 56.91ms (56.32, 57.54) 0.46+-
evaluation +0.79%, 0.25s slower X (1.40d, 0.02p, 0.18std) -0.20%, -0.04s invariant (-0.12d, 0.83p, 0.32std) +0.18%, 0.03s invariant (0.13d, 0.82p, 0.25std) +0.11%, 0.02ms invariant (0.07d, 0.89p, 0.22std) -0.60%, -0.34ms invariant (-0.51d, 0.36p, 0.64std)
CairoMakie 29.50s (29.21, 29.77) 0.20+- 20.53s (20.25, 20.79) 0.17+- 2.92s (2.89, 2.96) 0.02+- 15.59ms (15.53, 15.77) 0.09+- 1.47ms (1.45, 1.49) 0.01+-
master 29.64s (29.27, 29.91) 0.22+- 20.37s (20.18, 20.58) 0.16+- 2.93s (2.89, 2.98) 0.04+- 15.43ms (15.27, 15.60) 0.12+- 1.47ms (1.46, 1.49) 0.01+-
evaluation -0.48%, -0.14s invariant (-0.66d, 0.24p, 0.21std) +0.79%, 0.16s invariant (0.99d, 0.09p, 0.16std) -0.44%, -0.01s invariant (-0.42d, 0.45p, 0.03std) +1.06%, 0.16ms slower X (1.55d, 0.01p, 0.11std) -0.29%, -0.0ms invariant (-0.36d, 0.51p, 0.01std)
WGLMakie 30.76s (30.57, 31.19) 0.21+- 25.56s (25.34, 26.09) 0.25+- 40.90s (40.41, 41.50) 0.34+- 16.37ms (15.85, 17.50) 0.59+- 134.13ms (118.46, 205.38) 31.57+-
master 30.51s (30.39, 30.79) 0.14+- 25.38s (25.18, 25.67) 0.15+- 41.37s (41.01, 41.78) 0.29+- 15.07ms (14.68, 15.45) 0.28+- 88.21ms (77.76, 96.73) 6.82+-
evaluation +0.80%, 0.25s slower X (1.38d, 0.03p, 0.18std) +0.70%, 0.18s invariant (0.88d, 0.13p, 0.20std) -1.16%, -0.48s faster ✓ (-1.50d, 0.02p, 0.32std) +7.96%, 1.3ms slower❌ (2.82d, 0.00p, 0.43std) +34.23%, 45.92ms slower❌ (2.01d, 0.01p, 19.20std)

@Moelf
Copy link
Contributor

Moelf commented Oct 31, 2022

add breaking tag for this PR?

@jkrumbiegel jkrumbiegel added the breaking a PR with breaking changes label Oct 31, 2022
@himcraft
Copy link

Will this be merged into 0.18.4 with the Rich Text PR?

@jkrumbiegel
Copy link
Member Author

This should go into a new breaking version with rich text, so probably 0.19

@jkrumbiegel jkrumbiegel changed the base branch from master to jk/release-v0.19 November 23, 2022 11:49
@jkrumbiegel jkrumbiegel merged commit 1fecf4a into jk/release-v0.19 Nov 23, 2022
@jkrumbiegel jkrumbiegel deleted the jk/remove-textsize branch November 23, 2022 13:58
@jkrumbiegel jkrumbiegel mentioned this pull request Dec 27, 2022
t-bltg pushed a commit to t-bltg/Makie.jl that referenced this pull request Dec 31, 2022
* remove textsize for fontsize everywhere

* add previous fontsize to test

* fix one more text test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking a PR with breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants