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

[blocks] fix equal spacing in battery & sound & networkmanager #923

Merged
merged 2 commits into from
Nov 12, 2020

Conversation

Stunkymonkey
Copy link
Contributor

@Stunkymonkey Stunkymonkey commented Nov 10, 2020

related to #900 .

If the battery if fully charged, then the percentage does not have an additonal space.

Same if the volume if muted (and percentage not printed)

@Stunkymonkey Stunkymonkey changed the title [blocks] fix spacing in battery & sound [blocks] fix equal spacing in battery & sound Nov 10, 2020
@Stunkymonkey Stunkymonkey changed the title [blocks] fix equal spacing in battery & sound [blocks] fix equal spacing in battery & sound & networkmanager Nov 10, 2020
@Stunkymonkey
Copy link
Contributor Author

@ammgws any thoughts?

@ammgws
Copy link
Collaborator

ammgws commented Nov 11, 2020

On mobile now so hard to review, however if your battery config was "{time}%<space>{percentage}" instead of "{percentage}%<space>{time}" as mentioned in #900, does it still behave as expected?

@Stunkymonkey
Copy link
Contributor Author

On mobile now so hard to review, however if your battery config was "{time}%<space>{percentage}" instead of "{percentage}%<space>{time}" as mentioned in #900, does it still behave as expected?

this does not touch the formatting directly. In #900 I wrote, that rewriting the formatting would be ideal. But this is a lot of work. Maybe it is worth it, but the solution i found is quiet good I think.

So i decided to look at the current code and find out how to fix it, with the already given functions. So I adjusted the Spacing of the blocks, that I use and are wrongly aligned.

@ammgws
Copy link
Collaborator

ammgws commented Nov 11, 2020

Ah ok, IIRC you wrote that it solved all your issues so I was trying to figure out how this PR solved that particular one.

@Stunkymonkey
Copy link
Contributor Author

Stunkymonkey commented Nov 11, 2020

the fix is done in a different way, then discussed in the issue. Sorry for the confusion.

I discovered, that we would had to rewrite all blocks, which would definitely introduce bugs. Using the Spacing-Enum is fine for me, and all the blocks i am using are only displaying a single space at the start & end (what is wanted by default).

The only uneven spacing in-between happens if ethernet and wifi are used at the same time. But I consider this a minor issue (could be addressed in another PR).

@ammgws
Copy link
Collaborator

ammgws commented Nov 12, 2020

I tested with the below config but the output was the same before and after this PR. What am I not seeing?

[[block]]
block = "battery"
driver = "sysfs"
device = "CMB1"
format = "{percentage}% {time}"
allow_missing = true
hide_missing = true

after PR
with font awesome
"full_text":"  43% 1:49 "
"full_text":"  43% 1:14 "

no icons
"full_text":" BAT 44% 99:15 "
"full_text":" CHG 44% 1:17 "

before PR
with font awesome
"full_text":"  44% 2:57 "
"full_text":"  44% 1:23 "

no icons
"full_text":" BAT 46% 99:15 "
"full_text":" CHG 46% 1:11 "

@ammgws
Copy link
Collaborator

ammgws commented Nov 12, 2020

The sound block one I was able to confirm:
after PR
"full_text":" MIC MUTED<space>"

before PR
"full_text":" MIC MUTED<space><space>"

Copy link
Collaborator

@ammgws ammgws left a comment

Choose a reason for hiding this comment

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

Sound block one confirmed fixed

@ammgws
Copy link
Collaborator

ammgws commented Nov 12, 2020

OK I see why I couldn't reproduce the battery block one. Apparently "Not charging" is different to "Discharging" but I thought they were the same

   /// Query the device status. One of `"Full"`, `"Charging"`, `"Discharging"`,
    /// or `"Unknown"`. Thinkpad batteries also report "`Not charging`", which
    /// for our purposes should be treated as equivalent to full.

@ammgws
Copy link
Collaborator

ammgws commented Nov 12, 2020

My battery is taking too long to charge so I hardcoded it to 100% and confirmed the spacing bug is fixed:

With this PR: "full_text":" FULL<space>"
Before this PR: "full_text":" FULL<space><space>"

@ammgws
Copy link
Collaborator

ammgws commented Nov 12, 2020

I'll take your word for the networkmanager one.

Thanks for your work!

@ammgws ammgws merged commit 4361dd3 into greshake:master Nov 12, 2020
@Stunkymonkey
Copy link
Contributor Author

by interest: when is the next release planned?

@Stunkymonkey Stunkymonkey deleted the equal-spacing branch November 12, 2020 09:04
@GladOSkar
Copy link
Contributor

@Stunkymonkey From my experience a release happens about every 3-4 months depending on the amount of changes. Since the last one was just 2 weeks ago that might still be a while.

@ammgws
Copy link
Collaborator

ammgws commented Nov 12, 2020

I've starting putting together the release notes but I want to give it at least another couple of weeks or so (some people run the -git version so if there are any issues with recent changes then we should find out soon).

@Stunkymonkey
Copy link
Contributor Author

no problem at all. Testing is always good. I was just curious. Thanks for all your awesome work 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants