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

wofi: specify a unit (pt) for font size #552

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jezcope
Copy link
Contributor

@jezcope jezcope commented Sep 11, 2024

Without a unit specified in wofi's style.css, it emits a warning and defaults to using pixels, which (on my screens anyway) results in the font being larger than intended. This explicitly sets the unit to pt.

trueNAHO added a commit to jezcope/stylix that referenced this pull request Sep 11, 2024
@trueNAHO
Copy link
Collaborator

IIRC, rofi, tofi, and wofi are pretty much the same and might have the same API.

Can you verify whether d31d392 resolves the same issue in those applications? NOTE: I did not test this.

@cgahr
Copy link

cgahr commented Sep 12, 2024

@trueNAHO I did not try the referenced commit. However, I manually added a pt to the config file. As far as I can tell, is the same thing your commit does. This fixes the issue @jezcope describes, I seem to have the same one.

Also, with pt added, wofi does not emit a warning anymore that a unit is missing.

Copy link

@cgahr cgahr left a comment

Choose a reason for hiding this comment

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

This change fixes the issue with wofi (I did not try this commit, I added pt by hand).
Using the head of https://github.com/jezcope/stylix/tree/fix/wofi-font-size fixes the issue

@cgahr
Copy link

cgahr commented Sep 12, 2024

Can you verify whether d31d392 resolves the same issue in those applications? NOTE: I did not test this.

PS: the referenced commit is not the correct one

@jezcope
Copy link
Contributor Author

jezcope commented Sep 12, 2024

Just tried tofi (love it, by the way, it's replaced wofi for me) and it does not want pt specified: it errors if you add that to the font size with [ERROR]: Failed to parse "14pt" as unsigned int.

I haven't used rofi for a while but skimming through its docs it looks like its syntax for styling fonts is slightly different and only needs the number. Example taken from rofi's own theming docs:

* {
    background-color:      Black;
    text-color:            White;
    border-color:          White;
    font:            "Times New Roman 12";
}

So AFAICT there was no issue with stylix's configs for tofi and rofi, only for wofi.

trueNAHO pushed a commit to jezcope/stylix that referenced this pull request Sep 12, 2024
Link: danth#251
Link: danth#552

Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
Copy link
Collaborator

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

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

Changelog

v2: 205c3d2

  • Do not specify font size unit as pt for rofi
  • Do not specify font size unit as pt for tofi
  • Simplify commit header
  • Link: to related issue
  • Link: to GitHub PR
  • Add Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com> tag

v1: d31d392

  • Specify font size unit as pt for rofi
  • Specify font size unit as pt for tofi

v0: c8d4912

@trueNAHO
Copy link
Collaborator

@cgahr, if you want we can add your Tested-by tag to the commit. If so, should we use the information from https://patch-diff.githubusercontent.com/raw/cgahr/latexplotlib/pull/38.patch?

@cgahr
Copy link

cgahr commented Sep 12, 2024

I'm not really sure what you are talking about. What does latexplotlib have to do with that?

@trueNAHO
Copy link
Collaborator

I'm not really sure what you are talking about.

Just adding metadata to the commit that you successfully tested it.

What does latexplotlib have to do with that?

I was referring to the whether the name and email should be the one from the From: <NAME> <EMAIL> line in the linked patch.

@cgahr
Copy link

cgahr commented Sep 12, 2024

Ah I see. Please use
26804763+cgahr@users.noreply.github.com
Thanks! (Also I just learned more about privacy with github)

@trueNAHO
Copy link
Collaborator

Should I use the name from the linked PR or just your GitHub account name?

@cgahr
Copy link

cgahr commented Sep 12, 2024

Github please

@trueNAHO
Copy link
Collaborator

I can also just not include your Tested-by tag, if you prefer. Sorry for bothering.

@cgahr
Copy link

cgahr commented Sep 12, 2024

Don't worry! Feel free to include it

Link: danth#251
Link: danth#552

Tested-by: cgahr <26804763+cgahr@users.noreply.github.com>
Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
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.

3 participants