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.

Sign up for GitHub

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

Jump to bottom

When selecting font, attempt to use system-based font before internally supplied version #4594

Merged
merged 1 commit into from
Mar 12, 2022
Merged

When selecting font, attempt to use system-based font before internally supplied version #4594

merged 1 commit into from
Mar 12, 2022

Conversation

ddb4github
Copy link
Contributor

@ddb4github ddb4github commented Mar 3, 2022 •
edited
Loading

  • Delete buildin DejaVu font files
  • Use OS provided DejaVu font files, append default for RHEL/CentOS/SLES/Ubuntu

@ddb4github ddb4github changed the title Use OS provided DejaVu font for RHEL/CentOS/SLES/Ubuntu Use OS provided DejaVu font file for RHEL/CentOS/SLES/Ubuntu Mar 3, 2022
@ddb4github ddb4github changed the title Use OS provided DejaVu font file for RHEL/CentOS/SLES/Ubuntu Delete buildin DejaVu font file and use OS provided for RHEL/CentOS/SLES/Ubuntu Mar 4, 2022
@TheWitness
Copy link
Member

@netniV, what do you think? Does not consider Windows for sure.

netniV
netniV requested changes Mar 4, 2022
View reviewed changes
Copy link
Member

@netniV netniV left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think using the system supplied font is useful, though if it is behind our version of the font, this may cause issues. However, since the packaged versions of cacti will remove fonts in favour of the system supplied ones, I think people like @paulgevers would cheer this change on. It doesn't apply to windows currently, as we seem to hard code to the Arial font on windows, though we could use a font_paths array and a font_names array to combine them into a search path.

Also, if this is related to an issue, I don't recall seeing one and there should be an issue tracker number and a CHANGELOG

lib/functions.php Outdated Show resolved Hide resolved
lib/functions.php Outdated Show resolved Hide resolved
lib/rrd.php Outdated Show resolved Hide resolved
@ddb4github ddb4github requested a review from netniV March 6, 2022 00:25
@TheWitness
Copy link
Member

Let's make sure Windoz is covered.

@netniV
Copy link
Member

netniV commented Mar 12, 2022

I would also like to keep the currently included font as backup.

@ddb4github ddb4github force-pushed the removedejavufile branch 2 times, most recently from ded1eb1 to b8664ca Compare March 12, 2022 03:34
@TheWitness
Copy link
Member

Jing, make the Cacti supplied Dejavu don't the last choice in the array. Did you check that windows actually includes this font. Maybe not by default. I'll look after I'm fully alert.

@TheWitness
Copy link
Member

Okay, I must not be awake. This is perfect now. 👍

@TheWitness
Copy link
Member

Everything looks right. Thanks for your contribution Jing!

@TheWitness TheWitness merged commit 7d48258 into Cacti:1.2.x Mar 12, 2022
@ddb4github ddb4github deleted the removedejavufile branch March 12, 2022 22:19
@netniV netniV changed the title Delete buildin DejaVu font file and use OS provided for RHEL/CentOS/SLES/Ubuntu When selecting font, attempt to use system-based font before internally supplied version Apr 3, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Nov 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@netniV netniV Awaiting requested review from netniV

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ddb4github @TheWitness @netniV